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

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

[Mario Limonciello]

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
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

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Change to an allow list for wmi-bmof and suffix the extra instances

 drivers/platform/x86/wmi.c | 45 ++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index aed293b5af81..2997dad79e8b 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -105,6 +105,12 @@ static const struct acpi_device_id wmi_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 
+/* 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,
+};
+
 static struct platform_driver acpi_wmi_driver = {
 	.driver = {
 		.name = "acpi-wmi",
@@ -1073,6 +1079,19 @@ static const struct device_type wmi_type_data = {
 	.release = wmi_dev_release,
 };
 
+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;
+}
+
 static int wmi_create_device(struct device *wmi_bus_dev,
 			     struct wmi_block *wblock,
 			     struct acpi_device *device)
@@ -1080,6 +1099,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	struct acpi_device_info *info;
 	char method[WMI_ACPI_METHOD_NAME_SIZE];
 	int result;
+	uint count;
 
 	if (wblock->gblock.flags & ACPI_WMI_EVENT) {
 		wblock->dev.dev.type = &wmi_type_event;
@@ -1134,7 +1154,11 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	wblock->dev.dev.bus = &wmi_bus_type;
 	wblock->dev.dev.parent = wmi_bus_dev;
 
-	dev_set_name(&wblock->dev.dev, "%pUL", &wblock->gblock.guid);
+	count = guid_count(&wblock->gblock.guid);
+	if (count)
+		dev_set_name(&wblock->dev.dev, "%pUL-%d", &wblock->gblock.guid, count);
+	else
+		dev_set_name(&wblock->dev.dev, "%pUL", &wblock->gblock.guid);
 
 	device_initialize(&wblock->dev.dev);
 
@@ -1154,11 +1178,20 @@ static void wmi_free_devices(struct acpi_device *device)
 	}
 }
 
-static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
+static bool guid_already_parsed_for_legacy(struct acpi_device *device, const guid_t *guid)
 {
 	struct wmi_block *wblock;
 
 	list_for_each_entry(wblock, &wmi_block_list, list) {
+		/* skip warning and register if we know the driver will use struct wmi_driver */
+		for (int i = 0; allow_duplicates[i] != NULL; i++) {
+			guid_t tmp;
+
+			if (guid_parse(allow_duplicates[i], &tmp))
+				continue;
+			if (guid_equal(&tmp, guid))
+				return false;
+		}
 		if (guid_equal(&wblock->gblock.guid, guid)) {
 			/*
 			 * Because we historically didn't track the relationship
@@ -1208,13 +1241,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 		if (debug_dump_wdg)
 			wmi_dump_wdg(&gblock[i]);
 
-		/*
-		 * Some WMI devices, like those for nVidia hooks, have a
-		 * duplicate GUID. It's not clear what we should do in this
-		 * case yet, so for now, we'll just ignore the duplicate
-		 * for device creation.
-		 */
-		if (guid_already_parsed(device, &gblock[i].guid))
+		if (guid_already_parsed_for_legacy(device, &gblock[i].guid))
 			continue;
 
 		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);
-- 
2.34.1

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

[Hans de Goede]

Hi,

On 8/29/22 22:14, Mario Limonciello 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
> 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
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Change to an allow list for wmi-bmof and suffix the extra instances

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> 
>  drivers/platform/x86/wmi.c | 45 ++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index aed293b5af81..2997dad79e8b 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -105,6 +105,12 @@ static const struct acpi_device_id wmi_device_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
>  
> +/* 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,
> +};
> +
>  static struct platform_driver acpi_wmi_driver = {
>  	.driver = {
>  		.name = "acpi-wmi",
> @@ -1073,6 +1079,19 @@ static const struct device_type wmi_type_data = {
>  	.release = wmi_dev_release,
>  };
>  
> +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;
> +}
> +
>  static int wmi_create_device(struct device *wmi_bus_dev,
>  			     struct wmi_block *wblock,
>  			     struct acpi_device *device)
> @@ -1080,6 +1099,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>  	struct acpi_device_info *info;
>  	char method[WMI_ACPI_METHOD_NAME_SIZE];
>  	int result;
> +	uint count;
>  
>  	if (wblock->gblock.flags & ACPI_WMI_EVENT) {
>  		wblock->dev.dev.type = &wmi_type_event;
> @@ -1134,7 +1154,11 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>  	wblock->dev.dev.bus = &wmi_bus_type;
>  	wblock->dev.dev.parent = wmi_bus_dev;
>  
> -	dev_set_name(&wblock->dev.dev, "%pUL", &wblock->gblock.guid);
> +	count = guid_count(&wblock->gblock.guid);
> +	if (count)
> +		dev_set_name(&wblock->dev.dev, "%pUL-%d", &wblock->gblock.guid, count);
> +	else
> +		dev_set_name(&wblock->dev.dev, "%pUL", &wblock->gblock.guid);
>  
>  	device_initialize(&wblock->dev.dev);
>  
> @@ -1154,11 +1178,20 @@ static void wmi_free_devices(struct acpi_device *device)
>  	}
>  }
>  
> -static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
> +static bool guid_already_parsed_for_legacy(struct acpi_device *device, const guid_t *guid)
>  {
>  	struct wmi_block *wblock;
>  
>  	list_for_each_entry(wblock, &wmi_block_list, list) {
> +		/* skip warning and register if we know the driver will use struct wmi_driver */
> +		for (int i = 0; allow_duplicates[i] != NULL; i++) {
> +			guid_t tmp;
> +
> +			if (guid_parse(allow_duplicates[i], &tmp))
> +				continue;
> +			if (guid_equal(&tmp, guid))
> +				return false;
> +		}
>  		if (guid_equal(&wblock->gblock.guid, guid)) {
>  			/*
>  			 * Because we historically didn't track the relationship
> @@ -1208,13 +1241,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
>  		if (debug_dump_wdg)
>  			wmi_dump_wdg(&gblock[i]);
>  
> -		/*
> -		 * Some WMI devices, like those for nVidia hooks, have a
> -		 * duplicate GUID. It's not clear what we should do in this
> -		 * case yet, so for now, we'll just ignore the duplicate
> -		 * for device creation.
> -		 */
> -		if (guid_already_parsed(device, &gblock[i].guid))
> +		if (guid_already_parsed_for_legacy(device, &gblock[i].guid))
>  			continue;
>  
>  		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Limonciello, Mario]

[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?

1) Spelling error in commit message
2) Remove comma on terminator
3) Add a comment why guid_count is safe (if you agree with me it is)

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

[Hans de Goede]

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

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

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

[Hans de Goede]

Hi,

On 9/2/22 14:19, Mario Limonciello wrote:
> 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".

Ok, that works for me. I've added that as a comment as
well as squashed in the other 2 suggestions by Andy.

Regards,

Hans