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@xxxxxxx> >> >> 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; >>> >> >
