Re: [PATCH] platform/x86: think-lmi: Fix attribute name usage for non-compliant items

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5702 bytes --]

On Mon, 19 May 2025, Mark Pearson wrote:

> Hi Ilpo,
> 
> Thanks for the review
> 
> On Mon, May 19, 2025, at 8:56 AM, Ilpo Järvinen wrote:
> > On Fri, 16 May 2025, Mark Pearson wrote:
> >
> >> A few, quite rare, WMI attributes have names that are not compatible with
> >> filenames. e.g. "Intel VT for Directed I/O (VT-d)".
> >
> > filenames. -> filenames,
> 
> OK.
> 
> >
> >> For these cases the '/' gets replaced with '\' for display, but doesn't
> >> get switched again when doing the WMI access.
> >> 
> >> Fix this by keeping the original attribute name and using that for sending
> >> commands to the BIOS
> >> 
> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> >
> > Fixes tag?
> 
> This has been wrong since the very first implementation. I considered 
> adding a fixes for the first commit, but wasn't sure that was 
> reasonable.  Let me know if it should be added - I was a bit worried 
> about triggering other work for the stable maintainers. 

Please point Fixes into the first commit then. The first commit is no 
different or special in that sense and can fixed like any other commit.

If it fails to apply, stable maintainers just drop the patch and send a 
notification to you and you can provide the backported patch (AFAIK, they 
won't track that, so if you don't provide a backport, nothing happens 
beyond that). They won't try to backport them themselves if a patch 
conflicts.

-- 
 i.

> >>  drivers/platform/x86/think-lmi.c | 27 +++++++++++++++------------
> >>  drivers/platform/x86/think-lmi.h |  1 +
> >>  2 files changed, 16 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> >> index 0fc275e461be..be01085055a1 100644
> >> --- a/drivers/platform/x86/think-lmi.c
> >> +++ b/drivers/platform/x86/think-lmi.c
> >> @@ -137,6 +137,7 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
> >>   * You must reboot the computer before the changes will take effect.
> >>   */
> >>  #define LENOVO_CLEAR_BIOS_CERT_GUID  "B2BC39A7-78DD-4D71-B059-A510DEC44890"
> >> +
> >>  /*
> >>   * Name: CertToPassword
> >>   * Description: Switch from certificate to password authentication.
> >
> > An unrelated change.
> 
> Ah - my bad. I've been working on another patch (unrelated to this) and 
> this change crept in when I removed those changes. 
> 
> >
> >> @@ -1061,8 +1062,8 @@ static ssize_t current_value_store(struct kobject *kobj,
> >>  			ret = -EINVAL;
> >>  			goto out;
> >>  		}
> >> -		set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
> >> -					new_setting, tlmi_priv.pwd_admin->signature);
> >> +		set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->name,
> >> +				    new_setting, tlmi_priv.pwd_admin->signature);
> >>  		if (!set_str) {
> >>  			ret = -ENOMEM;
> >>  			goto out;
> >> @@ -1092,7 +1093,7 @@ static ssize_t current_value_store(struct kobject *kobj,
> >>  				goto out;
> >>  		}
> >>  
> >> -		set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> >> +		set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->name,
> >>  				    new_setting);
> >>  		if (!set_str) {
> >>  			ret = -ENOMEM;
> >> @@ -1120,11 +1121,11 @@ static ssize_t current_value_store(struct kobject *kobj,
> >>  		}
> >>  
> >>  		if (auth_str)
> >> -			set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
> >> -					new_setting, auth_str);
> >> +			set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->name,
> >> +					    new_setting, auth_str);
> >>  		else
> >> -			set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
> >> -					new_setting);
> >> +			set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->name,
> >> +					    new_setting);
> >>  		if (!set_str) {
> >>  			ret = -ENOMEM;
> >>  			goto out;
> >> @@ -1629,9 +1630,6 @@ static int tlmi_analyze(struct wmi_device *wdev)
> >>  			continue;
> >>  		}
> >>  
> >> -		/* It is not allowed to have '/' for file name. Convert it into '\'. */
> >> -		strreplace(item, '/', '\\');
> >> -
> >>  		/* Remove the value part */
> >>  		strreplace(item, ',', '\0');
> >>  
> >> @@ -1644,11 +1642,16 @@ static int tlmi_analyze(struct wmi_device *wdev)
> >>  		}
> >>  		setting->wdev = wdev;
> >>  		setting->index = i;
> >> +
> >> +		strscpy(setting->name, item);
> >> +		/* It is not allowed to have '/' for file name. Convert it into '\'. */
> >> +		strreplace(item, '/', '\\');
> >>  		strscpy(setting->display_name, item);
> >> +
> >>  		/* If BIOS selections supported, load those */
> >>  		if (tlmi_priv.can_get_bios_selections) {
> >> -			ret = tlmi_get_bios_selections(setting->display_name,
> >> -					&setting->possible_values);
> >> +			ret = tlmi_get_bios_selections(setting->name,
> >> +						       &setting->possible_values);
> >>  			if (ret || !setting->possible_values)
> >>  				pr_info("Error retrieving possible values for %d : %s\n",
> >>  						i, setting->display_name);
> >> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> >> index a80452482227..9b014644d316 100644
> >> --- a/drivers/platform/x86/think-lmi.h
> >> +++ b/drivers/platform/x86/think-lmi.h
> >> @@ -90,6 +90,7 @@ struct tlmi_attr_setting {
> >>  	struct kobject kobj;
> >>  	struct wmi_device *wdev;
> >>  	int index;
> >> +	char name[TLMI_SETTINGS_MAXLEN];
> >>  	char display_name[TLMI_SETTINGS_MAXLEN];
> >>  	char *possible_values;
> >>  };
> >> 
> >
> > -- 
> >  i.
> 
> Please confirm on if the fixes tag is needed or not, and I'll do v2 with 
> those minor changes
> 
> Thanks
> Mark
>