Message-ID: <dd71f560-6d57-8147-8010-f14abd4f63cf@redhat.com>
Date:   Mon, 29 Aug 2022 15:20:19 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Mario Limonciello <mario.limonciello@....com>,
        Mark Gross <markgross@...nel.org>
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID
 messages

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&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Ce38feb41da464767725808da89b3efcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637973703162395560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=sGh1bVTcO7vXOF6%2BwibhS7nbSiH3aEEdVNGfanKkGF8%3D&amp;reserved=0
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....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;
>>>   
>>
> 

Powered by blists - more mailing lists