lkml.org 
[lkml]   [2022]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers that use struct wmi_driver
From
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".

\
 
 \ /
  Last update: 2022-09-02 14:20    [W:0.134 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site