Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a547f88a-7ab2-e665-aa6d-d80ef74f4fd4@redhat.com>
Date:   Mon, 29 Aug 2022 13:45:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID
 messages
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Mark Gross <markgross@kernel.org>
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220826170053.2124-1-mario.limonciello@amd.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220826170053.2124-1-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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