Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers that use struct wmi_driver

[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@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".