On 9/2/22 03:07, Hans de Goede wrote:
Hi,
On 9/1/22 23:39, Limonciello, Mario wrote:
[Public]
-----Original Message-----
From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Sent: Thursday, September 1, 2022 12:17
To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; Mark Gross
<markgross@xxxxxxxxxx>; Platform Driver <platform-driver-
x86@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
kernel@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
that use struct wmi_driver
On Mon, Aug 29, 2022 at 11:20 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
The WMI subsystem in the kernel currently tracks WMI devices by
a GUID string not by ACPI device. The GUID used by the `wmi-bmof`
module however is available from many devices on nearly every machine.
This originally was though to be a bug, but as it happens on most
thought
machines it is a design mistake. It has been fixed by tying an ACPI
device to the driver with struct wmi_driver. So drivers that have
moved over to struct wmi_driver can actually support multiple
instantiations of a GUID without any problem.
Add an allow list into wmi.c for GUIDs that the drivers that are known
to use struct wmi_driver. The list is populated with `wmi-bmof` right
now. The additional instances of that in sysfs with be suffixed with -%d
...
+/* allow duplicate GUIDs as these device drivers use struct wmi_driver */
+static const char * const allow_duplicates[] = {
+ "05901221-D566-11D1-B2F0-00A0C9062910", /* wmi-bmof */
+ NULL,
No comma for the terminator.
+};
...
+static int guid_count(const guid_t *guid)
+{
+ struct wmi_block *wblock;
+ int count = 0;
+
+ list_for_each_entry(wblock, &wmi_block_list, list) {
+ if (guid_equal(&wblock->gblock.guid, guid))
+ count++;
+ }
+
+ return count;
+}
I haven't deeply checked the code, but this kind of approach is
fragile and proven to be error prone as shown in practice. The
scenario is (again, not sure if it's possible, need a comment in the
code if it's not possible) removing an entry from the list in the
middle and trying to add it again. you will see the duplicate count
values. That's why in the general case we use IDA or similar
approaches.
It shouldn't be possible to add/remove from the list, they're fixed
lists that were parsed from _WDG.
Hans - since you already took this into your review queue, can you
land fixes for the 3 things Andy pointed out before it goes to -next
or do you want me to do a manual follow up for them?
I can do a local fix and squash it into the original commit.
1) Spelling error in commit message
2) Remove comma on terminator
Ack, will fix.
3) Add a comment why guid_count is safe (if you agree with me it is)
I agree it is safe.
Can you suggest some wording for the comment please ?
Regards,
Hans
Maybe something like "_WDG is a static list that is only parsed at
startup, it's safe to count entries without extra protection".
