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

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

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.

>
>> ---
>>  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