Messages in this thread | | | Date | Fri, 2 Sep 2022 07:19:03 -0500 | Subject | Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers that use struct wmi_driver | From | Mario Limonciello <> |
| |
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@gmail.com> >>> Sent: Thursday, September 1, 2022 12:17 >>> To: Limonciello, Mario <Mario.Limonciello@amd.com> >>> Cc: Hans de Goede <hdegoede@redhat.com>; Mark Gross >>> <markgross@kernel.org>; Platform Driver <platform-driver- >>> x86@vger.kernel.org>; Linux Kernel Mailing List <linux- >>> kernel@vger.kernel.org> >>> 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@amd.com> 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".
| |