[PATCH v2 0/2] Think-LMI improvements

[PATCH v2 0/2] Think-LMI improvements

[Mario Limonciello]

When testing on a Lenovo T14 AMD I found some problems with the
driver, and this series addresses them.

Mario Limonciello (2):
  platform/x86: think-lmi: Fix issues with duplicate attributes
  platform/x86: think-lmi: Split current_value to reflect only the value

 drivers/platform/x86/think-lmi.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes

[Mario Limonciello]

On an AMD based Lenovo T14, I find that the module doesn't work at
all, and instead has a traceback with messages like:

```
sysfs: cannot create duplicate filename '/devices/virtual/firmware-attributes/thinklmi/attributes/Reserved'
```

Duplicate and reserved values showing up appear to be a firmware bug,
but they shouldn't make the driver explode.  So catch them and skip
them.

Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/think-lmi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

V1->V2 changes:
 * Don't show reserved objects either
 * Clear the object so it doesn't explode on module unload

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index d2644230b91f..4d8b5c185f8e 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -691,6 +691,16 @@ static int tlmi_sysfs_init(void)
 		if (!tlmi_priv.setting[i])
 			continue;
 
+		/* check for duplicate or reserved values */
+		if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) ||
+		    !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) {
+			pr_debug("duplicate or reserved attribute name found - %s\n",
+				tlmi_priv.setting[i]->display_name);
+			kfree(tlmi_priv.setting[i]->possible_values);
+			tlmi_priv.setting[i] = NULL;
+			continue;
+		}
+
 		/* Build attribute */
 		tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset;
 		ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype,
-- 
2.25.1

Re: [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes

[Hans de Goede]

Hi,

On 6/22/21 10:07 PM, Mario Limonciello wrote:
> On an AMD based Lenovo T14, I find that the module doesn't work at
> all, and instead has a traceback with messages like:
> 
> ```
> sysfs: cannot create duplicate filename '/devices/virtual/firmware-attributes/thinklmi/attributes/Reserved'
> ```
> 
> Duplicate and reserved values showing up appear to be a firmware bug,
> but they shouldn't make the driver explode.  So catch them and skip
> them.
> 
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thank you.

> ---
>  drivers/platform/x86/think-lmi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> V1->V2 changes:
>  * Don't show reserved objects either
>  * Clear the object so it doesn't explode on module unload
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index d2644230b91f..4d8b5c185f8e 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -691,6 +691,16 @@ static int tlmi_sysfs_init(void)
>  		if (!tlmi_priv.setting[i])
>  			continue;
>  
> +		/* check for duplicate or reserved values */
> +		if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) ||
> +		    !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) {
> +			pr_debug("duplicate or reserved attribute name found - %s\n",
> +				tlmi_priv.setting[i]->display_name);
> +			kfree(tlmi_priv.setting[i]->possible_values);

You are missing a:

			kfree(tlmi_priv.setting[i]);

here, this is done through the kobj_put in tlmi_release_attr(), but since we will
now never register the kobj we should just free this directly to avoid a memleak.

Since I want to include this important fix in my first pull-req for 5.14 I've fixed
this up locally and merged this into my review-hans branch, so there is no need to
send out a new version.

I've a non AMD ThinkPad (X1C8) which does have a single "Reserved" entry, so I'll
make sure to test the modified patch myself before sending out the 5.14 pdx86
pull-req.

Regards,

Hans




> +			tlmi_priv.setting[i] = NULL;
> +			continue;
> +		}
> +
>  		/* Build attribute */
>  		tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset;
>  		ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype,
> 

Re: [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes

[Limonciello, Mario]

On 6/28/2021 04:30, Hans de Goede wrote:
> Hi,
> 
> On 6/22/21 10:07 PM, Mario Limonciello wrote:
>> On an AMD based Lenovo T14, I find that the module doesn't work at
>> all, and instead has a traceback with messages like:
>>
>> ```
>> sysfs: cannot create duplicate filename '/devices/virtual/firmware-attributes/thinklmi/attributes/Reserved'
>> ```
>>
>> Duplicate and reserved values showing up appear to be a firmware bug,
>> but they shouldn't make the driver explode.  So catch them and skip
>> them.
>>
>> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Thank you.
> 
>> ---
>>   drivers/platform/x86/think-lmi.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> V1->V2 changes:
>>   * Don't show reserved objects either
>>   * Clear the object so it doesn't explode on module unload
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index d2644230b91f..4d8b5c185f8e 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -691,6 +691,16 @@ static int tlmi_sysfs_init(void)
>>   		if (!tlmi_priv.setting[i])
>>   			continue;
>>   
>> +		/* check for duplicate or reserved values */
>> +		if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) ||
>> +		    !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) {
>> +			pr_debug("duplicate or reserved attribute name found - %s\n",
>> +				tlmi_priv.setting[i]->display_name);
>> +			kfree(tlmi_priv.setting[i]->possible_values);
> 
> You are missing a:
> 
> 			kfree(tlmi_priv.setting[i]);
> 
> here, this is done through the kobj_put in tlmi_release_attr(), but since we will
> now never register the kobj we should just free this directly to avoid a memleak.
> 
> Since I want to include this important fix in my first pull-req for 5.14 I've fixed
> this up locally and merged this into my review-hans branch, so there is no need to
> send out a new version.
> 
> I've a non AMD ThinkPad (X1C8) which does have a single "Reserved" entry, so I'll
> make sure to test the modified patch myself before sending out the 5.14 pdx86
> pull-req.
> 

Thanks for taking care of that, happy this stuff will be in the first 
version for 5.14.

FYI so you are aware, the first userspace consumer (that I know of) for 
thinklmi will be fwupd.  When newer fwupd is run on 5.14+, it will use 
thinklmi to tell if boot order lock is engaged and prevent UEFI updates 
from occurring until it's disabled.

https://github.com/fwupd/fwupd/tree/master/plugins/lenovo-thinklmi


> Regards,
> 
> Hans
> 
> 
> 
> 
>> +			tlmi_priv.setting[i] = NULL;
>> +			continue;
>> +		}
>> +
>>   		/* Build attribute */
>>   		tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset;
>>   		ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype,
>>
> 

[PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value

[Mario Limonciello]

Currently attributes will show things like:
`BootOrderLock,Disable`
rather than just
`Disable`.

Of course this works, but the attribute is intended to be read by
userspace tools and not require further processing.  That is a userspace
tool can display a drop down of `possible_values` and `current_value` is
one of them from the list.

This also aligns `think-lmi` with how `dell-wmi-sysman` works.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/think-lmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Changes from v1->v2:
 * Move into the series
 * Correct a char->char*

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 4d8b5c185f8e..e71b631e6984 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -492,14 +492,19 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
 static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
-	char *item;
+	char *item, *value;
 	int ret;
 
 	ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
 	if (ret)
 		return ret;
 
-	ret = sysfs_emit(buf, "%s\n", item);
+	/* validate and split from `item,value` -> `value` */
+	value = strpbrk(item, ",");
+	if (!value || value == item || !strlen(value + 1))
+		return -EINVAL;
+
+	ret = sysfs_emit(buf, "%s\n", value + 1);
 	kfree(item);
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value

[Hans de Goede]

Hi,

On 6/22/21 10:07 PM, Mario Limonciello wrote:
> Currently attributes will show things like:
> `BootOrderLock,Disable`
> rather than just
> `Disable`.
> 
> Of course this works, but the attribute is intended to be read by
> userspace tools and not require further processing.  That is a userspace
> tool can display a drop down of `possible_values` and `current_value` is
> one of them from the list.
> 
> This also aligns `think-lmi` with how `dell-wmi-sysman` works.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/think-lmi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> Changes from v1->v2:
>  * Move into the series
>  * Correct a char->char*
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 4d8b5c185f8e..e71b631e6984 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -492,14 +492,19 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
>  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> -	char *item;
> +	char *item, *value;
>  	int ret;
>  
>  	ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
>  	if (ret)
>  		return ret;
>  
> -	ret = sysfs_emit(buf, "%s\n", item);
> +	/* validate and split from `item,value` -> `value` */
> +	value = strpbrk(item, ",");
> +	if (!value || value == item || !strlen(value + 1))
> +		return -EINVAL;
> +
> +	ret = sysfs_emit(buf, "%s\n", value + 1);
>  	kfree(item);
>  	return ret;
>  }
>