* [PATCH v2 0/2] Think-LMI improvements
@ 2021-06-22 20:07 Mario Limonciello
2021-06-22 20:07 ` [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes Mario Limonciello
2021-06-22 20:07 ` [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value Mario Limonciello
0 siblings, 2 replies; 6+ messages in thread
From: Mario Limonciello @ 2021-06-22 20:07 UTC (permalink / raw)
To: Hans de Goede, Mark Gross, open list:X86 PLATFORM DRIVERS, markpearson
Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes
2021-06-22 20:07 [PATCH v2 0/2] Think-LMI improvements Mario Limonciello
@ 2021-06-22 20:07 ` Mario Limonciello
2021-06-28 9:30 ` Hans de Goede
2021-06-22 20:07 ` [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value Mario Limonciello
1 sibling, 1 reply; 6+ messages in thread
From: Mario Limonciello @ 2021-06-22 20:07 UTC (permalink / raw)
To: Hans de Goede, Mark Gross, open list:X86 PLATFORM DRIVERS, markpearson
Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value
2021-06-22 20:07 [PATCH v2 0/2] Think-LMI improvements Mario Limonciello
2021-06-22 20:07 ` [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes Mario Limonciello
@ 2021-06-22 20:07 ` Mario Limonciello
2021-06-28 9:34 ` Hans de Goede
1 sibling, 1 reply; 6+ messages in thread
From: Mario Limonciello @ 2021-06-22 20:07 UTC (permalink / raw)
To: Hans de Goede, Mark Gross, open list:X86 PLATFORM DRIVERS, markpearson
Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes
2021-06-22 20:07 ` [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes Mario Limonciello
@ 2021-06-28 9:30 ` Hans de Goede
2021-06-28 14:49 ` Limonciello, Mario
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2021-06-28 9:30 UTC (permalink / raw)
To: Mario Limonciello, Mark Gross, open list:X86 PLATFORM DRIVERS,
markpearson
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value
2021-06-22 20:07 ` [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value Mario Limonciello
@ 2021-06-28 9:34 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2021-06-28 9:34 UTC (permalink / raw)
To: Mario Limonciello, Mark Gross, open list:X86 PLATFORM DRIVERS,
markpearson
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;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes
2021-06-28 9:30 ` Hans de Goede
@ 2021-06-28 14:49 ` Limonciello, Mario
0 siblings, 0 replies; 6+ messages in thread
From: Limonciello, Mario @ 2021-06-28 14:49 UTC (permalink / raw)
To: Hans de Goede, Mark Gross, open list:X86 PLATFORM DRIVERS, markpearson
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,
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-28 14:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 20:07 [PATCH v2 0/2] Think-LMI improvements Mario Limonciello
2021-06-22 20:07 ` [PATCH v2 1/2] platform/x86: think-lmi: Fix issues with duplicate attributes Mario Limonciello
2021-06-28 9:30 ` Hans de Goede
2021-06-28 14:49 ` Limonciello, Mario
2021-06-22 20:07 ` [PATCH v2 2/2] platform/x86: think-lmi: Split current_value to reflect only the value Mario Limonciello
2021-06-28 9:34 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.