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
