Bug 216460

Summary:	lenovo-thinklmi doesn't export a 'type' attribute and doesn't populate possible_values properly
Product:	Drivers	Reporter:	Mario Limonciello (AMD) (mario.limonciello)
Component:	Platform_x86	Assignee:	Mark Pearson (mpearson-lenovo)
Status:	RESOLVED CODE_FIX
Severity:	normal	CC:	fkrueger, js
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	6.0-rc3	Subsystem:
Regression:	No	Bisected commit-id:
Attachments:	Patch to expose type attributes Patch to expose type attributes (v2)

Description Mario Limonciello (AMD) 2022-09-07 18:42:48 UTC

The firmware-attributes documentation ( https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-firmware-attributes) indicates that all drivers that support firmware-attributes need to support the 'type' attribute.

This is important for userspace software to know how to interact with the driver.  

fwupd 1.8.4 added support for the firmware-attributes class API, but since lenovo-thinklmi doesn't meet it shows the following error on every boot.

KERNEL BUG: 'type' attribute not exported: (failed to load type: Failed to open file “/sys/class/firmware-attributes/thinklmi/attributes/MCRUSBHeader/type”: No such file or directory)

A workaround has been landed in fwupd to avoid this attribute and hardcode the known behavior of the current driver when it finds it.

It seems like all the attributes are actually "enumeration", so the lenovo-thinklmi kernel driver should probably just export a type sysfs file with "enumeration" hardcoded for all attributes.

Also though, the possible_values key is not populated either.  Instead the values are put into the "current_value" string.  On a Lenovo P620 the following happens:

$ sudo cat /sys/class/firmware-attributes/thinklmi/attributes/NUMA/current_value
Auto;[Optional:NPS1,NPS2,NPS4,Auto]
$ sudo cat /sys/class/firmware-attributes/thinklmi/attributes/NUMA/possible_values
cat: /sys/class/firmware-attributes/thinklmi/attributes/NUMA/possible_values: Operation not supported

That is the userspace software tears apart the current_value string and instead puts it into what the possible_values are.  This should be the responsibility of the kernel driver.  Even if the firwmare returns all that in current_value, the kernel driver should be doing the splitting so that userspace doesn't need to carry quirks for different kernel drivers behaving differently.

Comment 1 Frank Kruger 2022-10-01 08:54:24 UTC

JFYI: Given fwupd 1.8.5, I am seeing "fwupd[31123]: 19:40:48:0984 FuBiosSettings       KERNEL BUG: 'type' attribute not exported: (failed to load type: File >>/sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/type<<  No such file or directory)"

Comment 2 Frank Kruger 2022-10-01 08:56:23 UTC

(In reply to Frank Kruger from comment #1)
> JFYI: Given fwupd 1.8.5, I am seeing "fwupd[31123]: 19:40:48:0984
> FuBiosSettings       KERNEL BUG: 'type' attribute not exported: (failed to
> load type: File
> >>/sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/type<<  No
> such file or directory)"

...with ThinkPad T14 Gen1 and up-to-date FW 1.41.

Comment 3 Julian Stecklina 2022-11-28 01:44:42 UTC

Created attachment 303304 [details]
Patch to expose type attributes

I've given fixing this a shot. I've made every exported firmware attribute a "string". If this is not desired the fix will definitely be more involved.

Comment 4 Mario Limonciello (AMD) 2022-11-29 14:56:04 UTC

I don't think that's technically the right solution. Those are mostly enumeration types and possible values need to be exported. That's what fwupd is working around.

Comment 5 Julian Stecklina 2022-11-30 08:20:15 UTC

I can probably fix this in the patch, but it would be nice to have some documentation of this interface. Does anyone have pointers?

Comment 6 Mario Limonciello (AMD) 2022-11-30 15:20:56 UTC

I believe https://download.lenovo.com/ibmdl/pub/pc/pccbbs/thinkcentre_pdf/hrdeploy_en.pdf might be what you're looking for.

Comment 7 Julian Stecklina 2022-11-30 20:20:06 UTC

Ah, now I understand it. Then the patch may be extremely simple. We already expose `possible_values` for all firmware attributes in sysfs. This means we just have to expose type=enumeration as well.

I'm going to give this a spin on my Thinkpad.

Comment 8 Mario Limonciello (AMD) 2022-11-30 20:41:12 UTC

Well yes and no.  A new type=enumeration is needed; yes.

But this is what I see on my system:
$ cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/possible_values
cat: /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/possible_values: Operation not supported
$ sudo cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/current_value
Disable;[Optional:Disable,Enable]

What I expected to see is something like this:

$ cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/possible_values
Disable,Enable
$ sudo cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/current_value
Disable

Comment 9 Julian Stecklina 2022-12-10 00:07:21 UTC

Created attachment 303391 [details]
Patch to expose type attributes (v2)

I've updated the patch. It now only adds the `enumeration` type field:

/sys/class/firmware-attributes/thinklmi/attributes/SecureBoot # cat current_value 
Disable

/sys/class/firmware-attributes/thinklmi/attributes/SecureBoot # cat possible_values 
Disable,Enable

/sys/class/firmware-attributes/thinklmi/attributes/SecureBoot # cat type 
enumeration

Comment 10 Mario Limonciello (AMD) 2022-12-12 05:10:49 UTC

That seems like part of the solution, but on my system (P620) I still see:

$ cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/possible_values 
cat: /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/possible_values: Operation not supported

$ cat /sys/class/firmware-attributes/thinklmi/attributes/SecureBoot/current_value 
Disable;[Optional:Disable,Enable]

So there needs to be further string processing.

Comment 11 Julian Stecklina 2022-12-12 06:53:17 UTC

I can try to dig deeper, but this may be a firmware issue. Are you running the latest firmware?

Comment 12 Mario Limonciello (AMD) 2022-12-14 02:10:01 UTC

I just upgraded from 1.38 to 1.77 (the latest firmware), but the issue still persists on P620.

Comment 13 Mark Pearson 2023-02-28 18:07:27 UTC

I've got an internal ticket open with the FW team on the possible_values issue. The ThinkStation implementation doesn't support the WMI call that provides the possible_values separately (instead they've lumped them in with the current_value).

The type - I'd forgotten about this. It's not really a thing we have in the thinklmi implementation. I'm not actually sure why it was made mandatory but regardless I should fix the driver.
Enumeration works as a fix except for AlarmDate, AlarmTime and UserDefinedAlarmTime which are strings.....sigh

Comment 14 Mario Limonciello (AMD) 2023-02-28 18:09:41 UTC

At least from an "interface to userspace" perspective I think we should abstract the differences from one platform to another.

It might mean slicing and dicing strings in the kernel, but at least then we don't have to carry a pile of quirks for every software above the kernel.

Comment 15 Mario Limonciello (AMD) 2023-03-29 04:26:13 UTC

Fixes for this have been landed in mainline with these commit hashes:

8a02d70679fc1c434401863333c8ea7dbf201494
cf337f27f3bfc4aeab4954c468239fd6233c7638
45e21289bfc6e257885514790a8a8887da822d40
583329dcf22e568a328a944f20427ccfc95dce01

I validated with a snapshot of 6.3rc4+ (fcd476ea6a88 ("Merge tag 'urgent-rcu.2023.03.28a' of git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu")) and fwupd 1.8.13.

Everything is behaving properly now on Thinkstation P620, many thanks.