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