[PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

[PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

[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.  As there isn't an active need to
get the binary from each of the `wmi-bmof` device, special case it
and lower the message to debugging.  This will help to identify if
there are other duplicate GUIDs in the wild.

If there are and the information contained in them is desirable it
may be worth considering a design change to the WMI subsystem to
access those.

Link: https://lkml.org/lkml/2017/12/8/913
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/wmi-bmof.c |  2 --
 drivers/platform/x86/wmi.c      | 10 ++++++++--
 include/linux/wmi.h             |  2 ++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
index 80137afb9753..af46e9e03376 100644
--- a/drivers/platform/x86/wmi-bmof.c
+++ b/drivers/platform/x86/wmi-bmof.c
@@ -18,8 +18,6 @@
 #include <linux/types.h>
 #include <linux/wmi.h>
 
-#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
-
 struct bmof_priv {
 	union acpi_object *bmofdata;
 	struct bin_attribute bmof_bin_attr;
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index aed293b5af81..d7a1f4bf443b 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1157,6 +1157,9 @@ static void wmi_free_devices(struct acpi_device *device)
 static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
 {
 	struct wmi_block *wblock;
+	guid_t guid_wmi_bmof;
+
+	guid_parse(WMI_BMOF_GUID, &guid_wmi_bmof);
 
 	list_for_each_entry(wblock, &wmi_block_list, list) {
 		if (guid_equal(&wblock->gblock.guid, guid)) {
@@ -1166,8 +1169,11 @@ static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
 			 * we need to suppress GUIDs that are unique on a
 			 * given node but duplicated across nodes.
 			 */
-			dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
-				 guid, dev_name(&wblock->acpi_device->dev));
+			if (guid_equal(guid, &guid_wmi_bmof))
+				dev_dbg(&device->dev, "duplicate WMI-BMOF GUID found\n");
+			else
+				dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
+					 guid, dev_name(&wblock->acpi_device->dev));
 			return true;
 		}
 	}
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index b88d7b58e61e..59acdceb4411 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -13,6 +13,8 @@
 #include <linux/mod_devicetable.h>
 #include <uapi/linux/wmi.h>
 
+#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
+
 struct wmi_device {
 	struct device dev;
 
-- 
2.34.1

Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

[Hans de Goede]

Hi Mario,

On 8/26/22 19:00, 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.  As there isn't an active need to
> get the binary from each of the `wmi-bmof` device, special case it
> and lower the message to debugging.  This will help to identify if
> there are other duplicate GUIDs in the wild.
> 
> If there are and the information contained in them is desirable it
> may be worth considering a design change to the WMI subsystem to
> access those.
> 
> Link: https://lkml.org/lkml/2017/12/8/913
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

I am a bit surprised by this patch. I though that there was
consensus that the right thing to do here is actually create
wmi-bus devices for the duplicate WMI-ids adding a numbered
postfix to the extra devices (lets not add the postfix
to the first device for each WMI GUID as some userspace
code / scripts may depend on the sysfs paths not changing).

IMHO registering wmi-bus devices for all the WMI devices
in the ACPI table would be the right thing to do ?

Regards,

Hans




> ---
>  drivers/platform/x86/wmi-bmof.c |  2 --
>  drivers/platform/x86/wmi.c      | 10 ++++++++--
>  include/linux/wmi.h             |  2 ++
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
> index 80137afb9753..af46e9e03376 100644
> --- a/drivers/platform/x86/wmi-bmof.c
> +++ b/drivers/platform/x86/wmi-bmof.c
> @@ -18,8 +18,6 @@
>  #include <linux/types.h>
>  #include <linux/wmi.h>
>  
> -#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
> -
>  struct bmof_priv {
>  	union acpi_object *bmofdata;
>  	struct bin_attribute bmof_bin_attr;
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index aed293b5af81..d7a1f4bf443b 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1157,6 +1157,9 @@ static void wmi_free_devices(struct acpi_device *device)
>  static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>  {
>  	struct wmi_block *wblock;
> +	guid_t guid_wmi_bmof;
> +
> +	guid_parse(WMI_BMOF_GUID, &guid_wmi_bmof);
>  
>  	list_for_each_entry(wblock, &wmi_block_list, list) {
>  		if (guid_equal(&wblock->gblock.guid, guid)) {
> @@ -1166,8 +1169,11 @@ static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>  			 * we need to suppress GUIDs that are unique on a
>  			 * given node but duplicated across nodes.
>  			 */
> -			dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
> -				 guid, dev_name(&wblock->acpi_device->dev));
> +			if (guid_equal(guid, &guid_wmi_bmof))
> +				dev_dbg(&device->dev, "duplicate WMI-BMOF GUID found\n");
> +			else
> +				dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
> +					 guid, dev_name(&wblock->acpi_device->dev));
>  			return true;
>  		}
>  	}
> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
> index b88d7b58e61e..59acdceb4411 100644
> --- a/include/linux/wmi.h
> +++ b/include/linux/wmi.h
> @@ -13,6 +13,8 @@
>  #include <linux/mod_devicetable.h>
>  #include <uapi/linux/wmi.h>
>  
> +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
> +
>  struct wmi_device {
>  	struct device dev;
>  

Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

[Mario Limonciello]

On 8/29/22 06:45, Hans de Goede wrote:
> Hi Mario,
> 
> On 8/26/22 19:00, 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.  As there isn't an active need to
>> get the binary from each of the `wmi-bmof` device, special case it
>> and lower the message to debugging.  This will help to identify if
>> there are other duplicate GUIDs in the wild.
>>
>> If there are and the information contained in them is desirable it
>> may be worth considering a design change to the WMI subsystem to
>> access those.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2017%2F12%2F8%2F913&data=05%7C01%7Cmario.limonciello%40amd.com%7Ce38feb41da464767725808da89b3efcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637973703162395560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sGh1bVTcO7vXOF6%2BwibhS7nbSiH3aEEdVNGfanKkGF8%3D&reserved=0
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> I am a bit surprised by this patch. I though that there was
> consensus that the right thing to do here is actually create
> wmi-bus devices for the duplicate WMI-ids adding a numbered
> postfix to the extra devices (lets not add the postfix
> to the first device for each WMI GUID as some userspace
> code / scripts may depend on the sysfs paths not changing).
> 
> IMHO registering wmi-bus devices for all the WMI devices
> in the ACPI table would be the right thing to do ?

I don't disagree it's the correct eventual direction, but I looked at it 
and it seems to be a much larger overhaul because that means drivers 
would also need to be able to specify which ACPI device they're 
intending on interacting with from wmi.c rather than just a GUID string.

So before going down that path I think it's best to understand if it 
really is just wmi-bmof causing these cases or more cases (low priority 
IMO) or if there really is a strong need for the overhaul.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>> ---
>>   drivers/platform/x86/wmi-bmof.c |  2 --
>>   drivers/platform/x86/wmi.c      | 10 ++++++++--
>>   include/linux/wmi.h             |  2 ++
>>   3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>> index 80137afb9753..af46e9e03376 100644
>> --- a/drivers/platform/x86/wmi-bmof.c
>> +++ b/drivers/platform/x86/wmi-bmof.c
>> @@ -18,8 +18,6 @@
>>   #include <linux/types.h>
>>   #include <linux/wmi.h>
>>   
>> -#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>> -
>>   struct bmof_priv {
>>   	union acpi_object *bmofdata;
>>   	struct bin_attribute bmof_bin_attr;
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index aed293b5af81..d7a1f4bf443b 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1157,6 +1157,9 @@ static void wmi_free_devices(struct acpi_device *device)
>>   static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>   {
>>   	struct wmi_block *wblock;
>> +	guid_t guid_wmi_bmof;
>> +
>> +	guid_parse(WMI_BMOF_GUID, &guid_wmi_bmof);
>>   
>>   	list_for_each_entry(wblock, &wmi_block_list, list) {
>>   		if (guid_equal(&wblock->gblock.guid, guid)) {
>> @@ -1166,8 +1169,11 @@ static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>   			 * we need to suppress GUIDs that are unique on a
>>   			 * given node but duplicated across nodes.
>>   			 */
>> -			dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>> -				 guid, dev_name(&wblock->acpi_device->dev));
>> +			if (guid_equal(guid, &guid_wmi_bmof))
>> +				dev_dbg(&device->dev, "duplicate WMI-BMOF GUID found\n");
>> +			else
>> +				dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>> +					 guid, dev_name(&wblock->acpi_device->dev));
>>   			return true;
>>   		}
>>   	}
>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>> index b88d7b58e61e..59acdceb4411 100644
>> --- a/include/linux/wmi.h
>> +++ b/include/linux/wmi.h
>> @@ -13,6 +13,8 @@
>>   #include <linux/mod_devicetable.h>
>>   #include <uapi/linux/wmi.h>
>>   
>> +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>> +
>>   struct wmi_device {
>>   	struct device dev;
>>   
> 

Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

[Hans de Goede]

Hi,

On 8/29/22 14:17, Mario Limonciello wrote:
> On 8/29/22 06:45, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 8/26/22 19:00, 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.  As there isn't an active need to
>>> get the binary from each of the `wmi-bmof` device, special case it
>>> and lower the message to debugging.  This will help to identify if
>>> there are other duplicate GUIDs in the wild.
>>>
>>> If there are and the information contained in them is desirable it
>>> may be worth considering a design change to the WMI subsystem to
>>> access those.
>>>
>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2017%2F12%2F8%2F913&data=05%7C01%7Cmario.limonciello%40amd.com%7Ce38feb41da464767725808da89b3efcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637973703162395560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sGh1bVTcO7vXOF6%2BwibhS7nbSiH3aEEdVNGfanKkGF8%3D&reserved=0
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>> I am a bit surprised by this patch. I though that there was
>> consensus that the right thing to do here is actually create
>> wmi-bus devices for the duplicate WMI-ids adding a numbered
>> postfix to the extra devices (lets not add the postfix
>> to the first device for each WMI GUID as some userspace
>> code / scripts may depend on the sysfs paths not changing).
>>
>> IMHO registering wmi-bus devices for all the WMI devices
>> in the ACPI table would be the right thing to do ?
> 
> I don't disagree it's the correct eventual direction, but I looked at it and it seems to be a much larger overhaul because that means drivers would also need to be able to specify which ACPI device they're intending on interacting with from wmi.c rather than just a GUID string.
> 
> So before going down that path I think it's best to understand if it really is just wmi-bmof causing these cases or more cases (low priority IMO) or if there really is a strong need for the overhaul.

Hmm, some alternative ideas (just brainstorming here):

1. Use an allow-multiple-instances-guids list/array fo guids and create multiple-devices
   for those, starting with the bmof guid. The bmof driver is a new-style wmi-bus driver
   so it can handle multiple instances/devices just fine

2. Always instantiate multiple devices, making sure that we keep an ordered list of
   them, so that when searching for a guid through the old-style APIs we always
   find the first instance; and document that the old-style APIs always operate
   on the first wmi_device probed which matches the requested GUID

IMHO if an old-style driver needs to support multiple instances of the same GUID
it really should be converted to a new-style driver.

I personally think both suggestions are workable but I have a preference for option 1.

Regards,

Hans







> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> ---
>>>   drivers/platform/x86/wmi-bmof.c |  2 --
>>>   drivers/platform/x86/wmi.c      | 10 ++++++++--
>>>   include/linux/wmi.h             |  2 ++
>>>   3 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>>> index 80137afb9753..af46e9e03376 100644
>>> --- a/drivers/platform/x86/wmi-bmof.c
>>> +++ b/drivers/platform/x86/wmi-bmof.c
>>> @@ -18,8 +18,6 @@
>>>   #include <linux/types.h>
>>>   #include <linux/wmi.h>
>>>   -#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>>> -
>>>   struct bmof_priv {
>>>       union acpi_object *bmofdata;
>>>       struct bin_attribute bmof_bin_attr;
>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>> index aed293b5af81..d7a1f4bf443b 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -1157,6 +1157,9 @@ static void wmi_free_devices(struct acpi_device *device)
>>>   static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>>   {
>>>       struct wmi_block *wblock;
>>> +    guid_t guid_wmi_bmof;
>>> +
>>> +    guid_parse(WMI_BMOF_GUID, &guid_wmi_bmof);
>>>         list_for_each_entry(wblock, &wmi_block_list, list) {
>>>           if (guid_equal(&wblock->gblock.guid, guid)) {
>>> @@ -1166,8 +1169,11 @@ static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>>                * we need to suppress GUIDs that are unique on a
>>>                * given node but duplicated across nodes.
>>>                */
>>> -            dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>>> -                 guid, dev_name(&wblock->acpi_device->dev));
>>> +            if (guid_equal(guid, &guid_wmi_bmof))
>>> +                dev_dbg(&device->dev, "duplicate WMI-BMOF GUID found\n");
>>> +            else
>>> +                dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>>> +                     guid, dev_name(&wblock->acpi_device->dev));
>>>               return true;
>>>           }
>>>       }
>>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>>> index b88d7b58e61e..59acdceb4411 100644
>>> --- a/include/linux/wmi.h
>>> +++ b/include/linux/wmi.h
>>> @@ -13,6 +13,8 @@
>>>   #include <linux/mod_devicetable.h>
>>>   #include <uapi/linux/wmi.h>
>>>   +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>>> +
>>>   struct wmi_device {
>>>       struct device dev;
>>>   
>>
>