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

[Hans de Goede <hdegoede@redhat.com>]

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