[PATCH 1/3] platform/x86: wmi: Ignore duplicated GUIDs in legacy matches

[PATCH 1/3] platform/x86: wmi: Ignore duplicated GUIDs in legacy matches

[Armin Wolf]

When matching a WMI device to a GUID used by the legacy GUID-based
API, devices with a duplicated GUID should be ignored.

Add an additional WMI device flag signaling that the GUID used by
the WMI device is also used by another WMI device. Ignore such
devices inside the match functions used by the legacy GUID-based API.

Tested on a ASUS Prime B650-Plus.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index abd0183c4107..29dfe52eb802 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -57,6 +57,7 @@ static_assert(__alignof__(struct guid_block) == 1);

 enum {	/* wmi_block flags */
 	WMI_READ_TAKES_NO_ARGS,
+	WMI_GUID_DUPLICATED,
 	WMI_NO_EVENT_DATA,
 };

@@ -196,6 +197,12 @@ static int wmidev_match_guid(struct device *dev, const void *data)
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	const guid_t *guid = data;

+	/* Legacy GUID-based functions are restricted to only see
+	 * a single WMI device for each GUID.
+	 */
+	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags))
+		return 0;
+
 	if (guid_equal(guid, &wblock->gblock.guid))
 		return 1;

@@ -207,6 +214,12 @@ static int wmidev_match_notify_id(struct device *dev, const void *data)
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	const u32 *notify_id = data;

+	/* Legacy GUID-based functions are restricted to only see
+	 * a single WMI device for each GUID.
+	 */
+	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags))
+		return 0;
+
 	if (wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *notify_id)
 		return 1;

@@ -1036,10 +1049,12 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	wblock->dev.dev.parent = wmi_bus_dev;

 	count = guid_count(&wblock->gblock.guid);
-	if (count)
+	if (count) {
 		dev_set_name(&wblock->dev.dev, "%pUL-%d", &wblock->gblock.guid, count);
-	else
+		set_bit(WMI_GUID_DUPLICATED, &wblock->flags);
+	} else {
 		dev_set_name(&wblock->dev.dev, "%pUL", &wblock->gblock.guid);
+	}

 	device_initialize(&wblock->dev.dev);

--
2.39.2

[PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Armin Wolf]

Many older WMI drivers cannot be instantiated multiple times for
two reasons:

- they are using the legacy GUID-based WMI API
- they are singletons (with global state)

Prevent such WMI drivers from binding to WMI devices with a duplicated
GUID, as this would mean that the WMI driver will be instantiated at
least two times (one for the original GUID and one for the duplicated
GUID).
WMI drivers which can be instantiated multiple times can signal this
by setting a flag inside struct wmi_driver.

Tested on a ASUS Prime B650-Plus.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c                 |  1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
 drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
 drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
 drivers/platform/x86/wmi-bmof.c                |  1 +
 drivers/platform/x86/wmi.c                     | 12 ++++++++++++
 include/linux/wmi.h                            |  2 ++
 7 files changed, 19 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 6d8c0f328b7b..168d669c4eca 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
 	},
 	.id_table = dell_smm_wmi_id_table,
 	.probe = dell_smm_wmi_probe,
+	.no_singleton = true,
 };

 /*
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index db1e9240dd02..0b2299f7a2de 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
 	},
 	.id_table = dell_wmi_ddv_id_table,
 	.probe = dell_wmi_ddv_probe,
+	.no_singleton = true,
 };
 module_wmi_driver(dell_wmi_ddv_driver);

diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
index 040153ad67c1..75c82c08117f 100644
--- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
+++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
@@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
 	.probe = intel_wmi_sbl_fw_update_probe,
 	.remove = intel_wmi_sbl_fw_update_remove,
 	.id_table = intel_wmi_sbl_id_table,
+	.no_singleton = true,
 };
 module_wmi_driver(intel_wmi_sbl_fw_update_driver);

diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
index e2ad3f46f356..08df560a2c7a 100644
--- a/drivers/platform/x86/intel/wmi/thunderbolt.c
+++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
@@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
 		.dev_groups = tbt_groups,
 	},
 	.id_table = intel_wmi_thunderbolt_id_table,
+	.no_singleton = true,
 };

 module_wmi_driver(intel_wmi_thunderbolt_driver);
diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
index 644d2fd889c0..df6f0ae6e6c7 100644
--- a/drivers/platform/x86/wmi-bmof.c
+++ b/drivers/platform/x86/wmi-bmof.c
@@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
 	.probe = wmi_bmof_probe,
 	.remove = wmi_bmof_remove,
 	.id_table = wmi_bmof_id_table,
+	.no_singleton = true,
 };

 module_wmi_driver(wmi_bmof_driver);
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 29dfe52eb802..349deced87e8 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
 	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
 	int ret = 0;

+	/* Some older WMI drivers will break if instantiated multiple times,
+	 * so they are blocked from probing WMI devices with a duplicated GUID.
+	 *
+	 * New WMI drivers should support being instantiated multiple times.
+	 */
+	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
+		dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
+			 dev->driver->name);
+
+		return -ENODEV;
+	}
+
 	if (wdriver->notify) {
 		if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
 			return -ENODEV;
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 781958310bfb..63cca3b58d6d 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
  * @driver: Driver model structure
  * @id_table: List of WMI GUIDs supported by this driver
  * @no_notify_data: Driver supports WMI events which provide no event data
+ * @no_singleton: Driver can be instantiated multiple times
  * @probe: Callback for device binding
  * @remove: Callback for device unbinding
  * @notify: Callback for receiving WMI events
@@ -59,6 +60,7 @@ struct wmi_driver {
 	struct device_driver driver;
 	const struct wmi_device_id *id_table;
 	bool no_notify_data;
+	bool no_singleton;

 	int (*probe)(struct wmi_device *wdev, const void *context);
 	void (*remove)(struct wmi_device *wdev);
--
2.39.2

Re: [PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Pali Rohár]

On Monday 26 February 2024 20:35:56 Armin Wolf wrote:
> Many older WMI drivers cannot be instantiated multiple times for
> two reasons:
> 
> - they are using the legacy GUID-based WMI API
> - they are singletons (with global state)
> 
> Prevent such WMI drivers from binding to WMI devices with a duplicated
> GUID, as this would mean that the WMI driver will be instantiated at
> least two times (one for the original GUID and one for the duplicated
> GUID).
> WMI drivers which can be instantiated multiple times can signal this
> by setting a flag inside struct wmi_driver.

What do you think about opposite direction? Adding ".singleton = true"
into every driver which is not compatible with duplicated initialization
and having the default value that drivers are not singletons.

But if the direction it to not accept new "legacy" drivers and start get
rid of all "legacy" drivers by rewriting them, then it does not matter
if "no_singleton" or "is_singleton" is used...

> Tested on a ASUS Prime B650-Plus.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c                 |  1 +
>  drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
>  drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
>  drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
>  drivers/platform/x86/wmi-bmof.c                |  1 +
>  drivers/platform/x86/wmi.c                     | 12 ++++++++++++
>  include/linux/wmi.h                            |  2 ++
>  7 files changed, 19 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 6d8c0f328b7b..168d669c4eca 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
>  	},
>  	.id_table = dell_smm_wmi_id_table,
>  	.probe = dell_smm_wmi_probe,
> +	.no_singleton = true,
>  };
> 
>  /*
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index db1e9240dd02..0b2299f7a2de 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
>  	},
>  	.id_table = dell_wmi_ddv_id_table,
>  	.probe = dell_wmi_ddv_probe,
> +	.no_singleton = true,
>  };
>  module_wmi_driver(dell_wmi_ddv_driver);
> 
> diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> index 040153ad67c1..75c82c08117f 100644
> --- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> +++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> @@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
>  	.probe = intel_wmi_sbl_fw_update_probe,
>  	.remove = intel_wmi_sbl_fw_update_remove,
>  	.id_table = intel_wmi_sbl_id_table,
> +	.no_singleton = true,
>  };
>  module_wmi_driver(intel_wmi_sbl_fw_update_driver);
> 
> diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
> index e2ad3f46f356..08df560a2c7a 100644
> --- a/drivers/platform/x86/intel/wmi/thunderbolt.c
> +++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
> @@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
>  		.dev_groups = tbt_groups,
>  	},
>  	.id_table = intel_wmi_thunderbolt_id_table,
> +	.no_singleton = true,
>  };
> 
>  module_wmi_driver(intel_wmi_thunderbolt_driver);
> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
> index 644d2fd889c0..df6f0ae6e6c7 100644
> --- a/drivers/platform/x86/wmi-bmof.c
> +++ b/drivers/platform/x86/wmi-bmof.c
> @@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
>  	.probe = wmi_bmof_probe,
>  	.remove = wmi_bmof_remove,
>  	.id_table = wmi_bmof_id_table,
> +	.no_singleton = true,
>  };
> 
>  module_wmi_driver(wmi_bmof_driver);
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 29dfe52eb802..349deced87e8 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
>  	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
>  	int ret = 0;
> 
> +	/* Some older WMI drivers will break if instantiated multiple times,
> +	 * so they are blocked from probing WMI devices with a duplicated GUID.
> +	 *
> +	 * New WMI drivers should support being instantiated multiple times.
> +	 */
> +	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
> +		dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
> +			 dev->driver->name);
> +
> +		return -ENODEV;
> +	}
> +
>  	if (wdriver->notify) {
>  		if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
>  			return -ENODEV;
> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
> index 781958310bfb..63cca3b58d6d 100644
> --- a/include/linux/wmi.h
> +++ b/include/linux/wmi.h
> @@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
>   * @driver: Driver model structure
>   * @id_table: List of WMI GUIDs supported by this driver
>   * @no_notify_data: Driver supports WMI events which provide no event data
> + * @no_singleton: Driver can be instantiated multiple times
>   * @probe: Callback for device binding
>   * @remove: Callback for device unbinding
>   * @notify: Callback for receiving WMI events
> @@ -59,6 +60,7 @@ struct wmi_driver {
>  	struct device_driver driver;
>  	const struct wmi_device_id *id_table;
>  	bool no_notify_data;
> +	bool no_singleton;
> 
>  	int (*probe)(struct wmi_device *wdev, const void *context);
>  	void (*remove)(struct wmi_device *wdev);
> --
> 2.39.2
> 

Re: [PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Armin Wolf]

Am 27.02.24 um 21:30 schrieb Pali Rohár:

> On Monday 26 February 2024 20:35:56 Armin Wolf wrote:
>> Many older WMI drivers cannot be instantiated multiple times for
>> two reasons:
>>
>> - they are using the legacy GUID-based WMI API
>> - they are singletons (with global state)
>>
>> Prevent such WMI drivers from binding to WMI devices with a duplicated
>> GUID, as this would mean that the WMI driver will be instantiated at
>> least two times (one for the original GUID and one for the duplicated
>> GUID).
>> WMI drivers which can be instantiated multiple times can signal this
>> by setting a flag inside struct wmi_driver.
> What do you think about opposite direction? Adding ".singleton = true"
> into every driver which is not compatible with duplicated initialization
> and having the default value that drivers are not singletons.
>
> But if the direction it to not accept new "legacy" drivers and start get
> rid of all "legacy" drivers by rewriting them, then it does not matter
> if "no_singleton" or "is_singleton" is used...

Hi,

i want to make sure that i wont forget any legacy WMI drivers. This way, the
older legacy WMI drivers automatically initialize no_singleton with false.

Also i intent to not accept new legacy WMI drivers.

Thanks,
Armin Wolf

>> Tested on a ASUS Prime B650-Plus.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c                 |  1 +
>>   drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
>>   drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
>>   drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
>>   drivers/platform/x86/wmi-bmof.c                |  1 +
>>   drivers/platform/x86/wmi.c                     | 12 ++++++++++++
>>   include/linux/wmi.h                            |  2 ++
>>   7 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 6d8c0f328b7b..168d669c4eca 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
>>   	},
>>   	.id_table = dell_smm_wmi_id_table,
>>   	.probe = dell_smm_wmi_probe,
>> +	.no_singleton = true,
>>   };
>>
>>   /*
>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> index db1e9240dd02..0b2299f7a2de 100644
>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> @@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
>>   	},
>>   	.id_table = dell_wmi_ddv_id_table,
>>   	.probe = dell_wmi_ddv_probe,
>> +	.no_singleton = true,
>>   };
>>   module_wmi_driver(dell_wmi_ddv_driver);
>>
>> diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>> index 040153ad67c1..75c82c08117f 100644
>> --- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>> +++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>> @@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
>>   	.probe = intel_wmi_sbl_fw_update_probe,
>>   	.remove = intel_wmi_sbl_fw_update_remove,
>>   	.id_table = intel_wmi_sbl_id_table,
>> +	.no_singleton = true,
>>   };
>>   module_wmi_driver(intel_wmi_sbl_fw_update_driver);
>>
>> diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
>> index e2ad3f46f356..08df560a2c7a 100644
>> --- a/drivers/platform/x86/intel/wmi/thunderbolt.c
>> +++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
>> @@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
>>   		.dev_groups = tbt_groups,
>>   	},
>>   	.id_table = intel_wmi_thunderbolt_id_table,
>> +	.no_singleton = true,
>>   };
>>
>>   module_wmi_driver(intel_wmi_thunderbolt_driver);
>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>> index 644d2fd889c0..df6f0ae6e6c7 100644
>> --- a/drivers/platform/x86/wmi-bmof.c
>> +++ b/drivers/platform/x86/wmi-bmof.c
>> @@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
>>   	.probe = wmi_bmof_probe,
>>   	.remove = wmi_bmof_remove,
>>   	.id_table = wmi_bmof_id_table,
>> +	.no_singleton = true,
>>   };
>>
>>   module_wmi_driver(wmi_bmof_driver);
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 29dfe52eb802..349deced87e8 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
>>   	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
>>   	int ret = 0;
>>
>> +	/* Some older WMI drivers will break if instantiated multiple times,
>> +	 * so they are blocked from probing WMI devices with a duplicated GUID.
>> +	 *
>> +	 * New WMI drivers should support being instantiated multiple times.
>> +	 */
>> +	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
>> +		dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
>> +			 dev->driver->name);
>> +
>> +		return -ENODEV;
>> +	}
>> +
>>   	if (wdriver->notify) {
>>   		if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
>>   			return -ENODEV;
>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>> index 781958310bfb..63cca3b58d6d 100644
>> --- a/include/linux/wmi.h
>> +++ b/include/linux/wmi.h
>> @@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
>>    * @driver: Driver model structure
>>    * @id_table: List of WMI GUIDs supported by this driver
>>    * @no_notify_data: Driver supports WMI events which provide no event data
>> + * @no_singleton: Driver can be instantiated multiple times
>>    * @probe: Callback for device binding
>>    * @remove: Callback for device unbinding
>>    * @notify: Callback for receiving WMI events
>> @@ -59,6 +60,7 @@ struct wmi_driver {
>>   	struct device_driver driver;
>>   	const struct wmi_device_id *id_table;
>>   	bool no_notify_data;
>> +	bool no_singleton;
>>
>>   	int (*probe)(struct wmi_device *wdev, const void *context);
>>   	void (*remove)(struct wmi_device *wdev);
>> --
>> 2.39.2
>>

Re: [PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Pali Rohár]

On Tuesday 27 February 2024 23:47:11 Armin Wolf wrote:
> Am 27.02.24 um 21:30 schrieb Pali Rohár:
> 
> > On Monday 26 February 2024 20:35:56 Armin Wolf wrote:
> > > Many older WMI drivers cannot be instantiated multiple times for
> > > two reasons:
> > > 
> > > - they are using the legacy GUID-based WMI API
> > > - they are singletons (with global state)
> > > 
> > > Prevent such WMI drivers from binding to WMI devices with a duplicated
> > > GUID, as this would mean that the WMI driver will be instantiated at
> > > least two times (one for the original GUID and one for the duplicated
> > > GUID).
> > > WMI drivers which can be instantiated multiple times can signal this
> > > by setting a flag inside struct wmi_driver.
> > What do you think about opposite direction? Adding ".singleton = true"
> > into every driver which is not compatible with duplicated initialization
> > and having the default value that drivers are not singletons.
> > 
> > But if the direction it to not accept new "legacy" drivers and start get
> > rid of all "legacy" drivers by rewriting them, then it does not matter
> > if "no_singleton" or "is_singleton" is used...
> 
> Hi,
> 
> i want to make sure that i wont forget any legacy WMI drivers. This way, the
> older legacy WMI drivers automatically initialize no_singleton with false.
> 
> Also i intent to not accept new legacy WMI drivers.

Ok. In this case it does not matter.

> Thanks,
> Armin Wolf
> 
> > > Tested on a ASUS Prime B650-Plus.
> > > 
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   drivers/hwmon/dell-smm-hwmon.c                 |  1 +
> > >   drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
> > >   drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
> > >   drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
> > >   drivers/platform/x86/wmi-bmof.c                |  1 +
> > >   drivers/platform/x86/wmi.c                     | 12 ++++++++++++
> > >   include/linux/wmi.h                            |  2 ++
> > >   7 files changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index 6d8c0f328b7b..168d669c4eca 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
> > >   	},
> > >   	.id_table = dell_smm_wmi_id_table,
> > >   	.probe = dell_smm_wmi_probe,
> > > +	.no_singleton = true,
> > >   };
> > > 
> > >   /*
> > > diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> > > index db1e9240dd02..0b2299f7a2de 100644
> > > --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> > > +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> > > @@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
> > >   	},
> > >   	.id_table = dell_wmi_ddv_id_table,
> > >   	.probe = dell_wmi_ddv_probe,
> > > +	.no_singleton = true,
> > >   };
> > >   module_wmi_driver(dell_wmi_ddv_driver);
> > > 
> > > diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> > > index 040153ad67c1..75c82c08117f 100644
> > > --- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> > > +++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
> > > @@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
> > >   	.probe = intel_wmi_sbl_fw_update_probe,
> > >   	.remove = intel_wmi_sbl_fw_update_remove,
> > >   	.id_table = intel_wmi_sbl_id_table,
> > > +	.no_singleton = true,
> > >   };
> > >   module_wmi_driver(intel_wmi_sbl_fw_update_driver);
> > > 
> > > diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
> > > index e2ad3f46f356..08df560a2c7a 100644
> > > --- a/drivers/platform/x86/intel/wmi/thunderbolt.c
> > > +++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
> > > @@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
> > >   		.dev_groups = tbt_groups,
> > >   	},
> > >   	.id_table = intel_wmi_thunderbolt_id_table,
> > > +	.no_singleton = true,
> > >   };
> > > 
> > >   module_wmi_driver(intel_wmi_thunderbolt_driver);
> > > diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
> > > index 644d2fd889c0..df6f0ae6e6c7 100644
> > > --- a/drivers/platform/x86/wmi-bmof.c
> > > +++ b/drivers/platform/x86/wmi-bmof.c
> > > @@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
> > >   	.probe = wmi_bmof_probe,
> > >   	.remove = wmi_bmof_remove,
> > >   	.id_table = wmi_bmof_id_table,
> > > +	.no_singleton = true,
> > >   };
> > > 
> > >   module_wmi_driver(wmi_bmof_driver);
> > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > > index 29dfe52eb802..349deced87e8 100644
> > > --- a/drivers/platform/x86/wmi.c
> > > +++ b/drivers/platform/x86/wmi.c
> > > @@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
> > >   	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
> > >   	int ret = 0;
> > > 
> > > +	/* Some older WMI drivers will break if instantiated multiple times,
> > > +	 * so they are blocked from probing WMI devices with a duplicated GUID.
> > > +	 *
> > > +	 * New WMI drivers should support being instantiated multiple times.
> > > +	 */
> > > +	if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
> > > +		dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
> > > +			 dev->driver->name);
> > > +
> > > +		return -ENODEV;
> > > +	}
> > > +
> > >   	if (wdriver->notify) {
> > >   		if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
> > >   			return -ENODEV;
> > > diff --git a/include/linux/wmi.h b/include/linux/wmi.h
> > > index 781958310bfb..63cca3b58d6d 100644
> > > --- a/include/linux/wmi.h
> > > +++ b/include/linux/wmi.h
> > > @@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
> > >    * @driver: Driver model structure
> > >    * @id_table: List of WMI GUIDs supported by this driver
> > >    * @no_notify_data: Driver supports WMI events which provide no event data
> > > + * @no_singleton: Driver can be instantiated multiple times
> > >    * @probe: Callback for device binding
> > >    * @remove: Callback for device unbinding
> > >    * @notify: Callback for receiving WMI events
> > > @@ -59,6 +60,7 @@ struct wmi_driver {
> > >   	struct device_driver driver;
> > >   	const struct wmi_device_id *id_table;
> > >   	bool no_notify_data;
> > > +	bool no_singleton;
> > > 
> > >   	int (*probe)(struct wmi_device *wdev, const void *context);
> > >   	void (*remove)(struct wmi_device *wdev);
> > > --
> > > 2.39.2
> > > 

Re: [PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Hans de Goede]

Hi Armin,

On 2/27/24 23:47, Armin Wolf wrote:
> Am 27.02.24 um 21:30 schrieb Pali Rohár:
> 
>> On Monday 26 February 2024 20:35:56 Armin Wolf wrote:
>>> Many older WMI drivers cannot be instantiated multiple times for
>>> two reasons:
>>>
>>> - they are using the legacy GUID-based WMI API
>>> - they are singletons (with global state)
>>>
>>> Prevent such WMI drivers from binding to WMI devices with a duplicated
>>> GUID, as this would mean that the WMI driver will be instantiated at
>>> least two times (one for the original GUID and one for the duplicated
>>> GUID).
>>> WMI drivers which can be instantiated multiple times can signal this
>>> by setting a flag inside struct wmi_driver.
>> What do you think about opposite direction? Adding ".singleton = true"
>> into every driver which is not compatible with duplicated initialization
>> and having the default value that drivers are not singletons.
>>
>> But if the direction it to not accept new "legacy" drivers and start get
>> rid of all "legacy" drivers by rewriting them, then it does not matter
>> if "no_singleton" or "is_singleton" is used...
> 
> Hi,
> 
> i want to make sure that i wont forget any legacy WMI drivers. This way, the
> older legacy WMI drivers automatically initialize no_singleton with false.
> 
> Also i intent to not accept new legacy WMI drivers.

Somewhat offtopic question, how do you plan to handle the case where
there are 2 WMI GUIDs for what really is a single "thing",
specifically one main WMI GUID for a vendor specific interface
for say the embedded-controller and a separate GUID for events ?

IIRC we have several such cases. I thought we even have a case
where the main WMI GUID already is bound to using wmi_bus wmi_driver,
while the event guid is listened to by using wmi_install_notify_handler()
but I cannot find the code doing this, so I might be mistaken on this.

Regards,

Hans



>>> Tested on a ASUS Prime B650-Plus.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/hwmon/dell-smm-hwmon.c                 |  1 +
>>>   drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
>>>   drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
>>>   drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
>>>   drivers/platform/x86/wmi-bmof.c                |  1 +
>>>   drivers/platform/x86/wmi.c                     | 12 ++++++++++++
>>>   include/linux/wmi.h                            |  2 ++
>>>   7 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>> index 6d8c0f328b7b..168d669c4eca 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
>>>       },
>>>       .id_table = dell_smm_wmi_id_table,
>>>       .probe = dell_smm_wmi_probe,
>>> +    .no_singleton = true,
>>>   };
>>>
>>>   /*
>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> index db1e9240dd02..0b2299f7a2de 100644
>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> @@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
>>>       },
>>>       .id_table = dell_wmi_ddv_id_table,
>>>       .probe = dell_wmi_ddv_probe,
>>> +    .no_singleton = true,
>>>   };
>>>   module_wmi_driver(dell_wmi_ddv_driver);
>>>
>>> diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>> index 040153ad67c1..75c82c08117f 100644
>>> --- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>> +++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>> @@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
>>>       .probe = intel_wmi_sbl_fw_update_probe,
>>>       .remove = intel_wmi_sbl_fw_update_remove,
>>>       .id_table = intel_wmi_sbl_id_table,
>>> +    .no_singleton = true,
>>>   };
>>>   module_wmi_driver(intel_wmi_sbl_fw_update_driver);
>>>
>>> diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
>>> index e2ad3f46f356..08df560a2c7a 100644
>>> --- a/drivers/platform/x86/intel/wmi/thunderbolt.c
>>> +++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
>>> @@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
>>>           .dev_groups = tbt_groups,
>>>       },
>>>       .id_table = intel_wmi_thunderbolt_id_table,
>>> +    .no_singleton = true,
>>>   };
>>>
>>>   module_wmi_driver(intel_wmi_thunderbolt_driver);
>>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>>> index 644d2fd889c0..df6f0ae6e6c7 100644
>>> --- a/drivers/platform/x86/wmi-bmof.c
>>> +++ b/drivers/platform/x86/wmi-bmof.c
>>> @@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
>>>       .probe = wmi_bmof_probe,
>>>       .remove = wmi_bmof_remove,
>>>       .id_table = wmi_bmof_id_table,
>>> +    .no_singleton = true,
>>>   };
>>>
>>>   module_wmi_driver(wmi_bmof_driver);
>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>> index 29dfe52eb802..349deced87e8 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
>>>       struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
>>>       int ret = 0;
>>>
>>> +    /* Some older WMI drivers will break if instantiated multiple times,
>>> +     * so they are blocked from probing WMI devices with a duplicated GUID.
>>> +     *
>>> +     * New WMI drivers should support being instantiated multiple times.
>>> +     */
>>> +    if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
>>> +        dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
>>> +             dev->driver->name);
>>> +
>>> +        return -ENODEV;
>>> +    }
>>> +
>>>       if (wdriver->notify) {
>>>           if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
>>>               return -ENODEV;
>>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>>> index 781958310bfb..63cca3b58d6d 100644
>>> --- a/include/linux/wmi.h
>>> +++ b/include/linux/wmi.h
>>> @@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
>>>    * @driver: Driver model structure
>>>    * @id_table: List of WMI GUIDs supported by this driver
>>>    * @no_notify_data: Driver supports WMI events which provide no event data
>>> + * @no_singleton: Driver can be instantiated multiple times
>>>    * @probe: Callback for device binding
>>>    * @remove: Callback for device unbinding
>>>    * @notify: Callback for receiving WMI events
>>> @@ -59,6 +60,7 @@ struct wmi_driver {
>>>       struct device_driver driver;
>>>       const struct wmi_device_id *id_table;
>>>       bool no_notify_data;
>>> +    bool no_singleton;
>>>
>>>       int (*probe)(struct wmi_device *wdev, const void *context);
>>>       void (*remove)(struct wmi_device *wdev);
>>> -- 
>>> 2.39.2
>>>
> 

Re: [PATCH 2/3] platform/x86: wmi: Do not instantiate older WMI drivers multiple times

[Armin Wolf]

Am 28.02.24 um 14:23 schrieb Hans de Goede:

> Hi Armin,
>
> On 2/27/24 23:47, Armin Wolf wrote:
>> Am 27.02.24 um 21:30 schrieb Pali Rohár:
>>
>>> On Monday 26 February 2024 20:35:56 Armin Wolf wrote:
>>>> Many older WMI drivers cannot be instantiated multiple times for
>>>> two reasons:
>>>>
>>>> - they are using the legacy GUID-based WMI API
>>>> - they are singletons (with global state)
>>>>
>>>> Prevent such WMI drivers from binding to WMI devices with a duplicated
>>>> GUID, as this would mean that the WMI driver will be instantiated at
>>>> least two times (one for the original GUID and one for the duplicated
>>>> GUID).
>>>> WMI drivers which can be instantiated multiple times can signal this
>>>> by setting a flag inside struct wmi_driver.
>>> What do you think about opposite direction? Adding ".singleton = true"
>>> into every driver which is not compatible with duplicated initialization
>>> and having the default value that drivers are not singletons.
>>>
>>> But if the direction it to not accept new "legacy" drivers and start get
>>> rid of all "legacy" drivers by rewriting them, then it does not matter
>>> if "no_singleton" or "is_singleton" is used...
>> Hi,
>>
>> i want to make sure that i wont forget any legacy WMI drivers. This way, the
>> older legacy WMI drivers automatically initialize no_singleton with false.
>>
>> Also i intent to not accept new legacy WMI drivers.
> Somewhat offtopic question, how do you plan to handle the case where
> there are 2 WMI GUIDs for what really is a single "thing",
> specifically one main WMI GUID for a vendor specific interface
> for say the embedded-controller and a separate GUID for events ?
>
> IIRC we have several such cases. I thought we even have a case
> where the main WMI GUID already is bound to using wmi_bus wmi_driver,
> while the event guid is listened to by using wmi_install_notify_handler()
> but I cannot find the code doing this, so I might be mistaken on this.
>
> Regards,
>
> Hans

I am aware of those WMI device constellations, for the classic event-data constellation
i think there should be two WMI drivers (one for the event and one for the data) which
are connected with each other through a standard notifier.

I plan on writing a WMI driver developing guide which will address this by giving examples
on what to do in such cases.

The problem is that many WMI interfaces consisting of multiple GUIDs are inherently unstable
since the vendor assumes that they will just be available after the Windows userspace has loaded,
so the WMI driver has no means of discovering which GUIDs are available at probe time (other GUIDs
can be discovered later by the WMI driver core, so the WMI driver cannot see them sooner).

The solution for this depends on the specific WMI device constellation.

Armin Wolf.

>
>>>> Tested on a ASUS Prime B650-Plus.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>    drivers/hwmon/dell-smm-hwmon.c                 |  1 +
>>>>    drivers/platform/x86/dell/dell-wmi-ddv.c       |  1 +
>>>>    drivers/platform/x86/intel/wmi/sbl-fw-update.c |  1 +
>>>>    drivers/platform/x86/intel/wmi/thunderbolt.c   |  1 +
>>>>    drivers/platform/x86/wmi-bmof.c                |  1 +
>>>>    drivers/platform/x86/wmi.c                     | 12 ++++++++++++
>>>>    include/linux/wmi.h                            |  2 ++
>>>>    7 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>>> index 6d8c0f328b7b..168d669c4eca 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -1587,6 +1587,7 @@ static struct wmi_driver dell_smm_wmi_driver = {
>>>>        },
>>>>        .id_table = dell_smm_wmi_id_table,
>>>>        .probe = dell_smm_wmi_probe,
>>>> +    .no_singleton = true,
>>>>    };
>>>>
>>>>    /*
>>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> index db1e9240dd02..0b2299f7a2de 100644
>>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>>> @@ -882,6 +882,7 @@ static struct wmi_driver dell_wmi_ddv_driver = {
>>>>        },
>>>>        .id_table = dell_wmi_ddv_id_table,
>>>>        .probe = dell_wmi_ddv_probe,
>>>> +    .no_singleton = true,
>>>>    };
>>>>    module_wmi_driver(dell_wmi_ddv_driver);
>>>>
>>>> diff --git a/drivers/platform/x86/intel/wmi/sbl-fw-update.c b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>>> index 040153ad67c1..75c82c08117f 100644
>>>> --- a/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>>> +++ b/drivers/platform/x86/intel/wmi/sbl-fw-update.c
>>>> @@ -131,6 +131,7 @@ static struct wmi_driver intel_wmi_sbl_fw_update_driver = {
>>>>        .probe = intel_wmi_sbl_fw_update_probe,
>>>>        .remove = intel_wmi_sbl_fw_update_remove,
>>>>        .id_table = intel_wmi_sbl_id_table,
>>>> +    .no_singleton = true,
>>>>    };
>>>>    module_wmi_driver(intel_wmi_sbl_fw_update_driver);
>>>>
>>>> diff --git a/drivers/platform/x86/intel/wmi/thunderbolt.c b/drivers/platform/x86/intel/wmi/thunderbolt.c
>>>> index e2ad3f46f356..08df560a2c7a 100644
>>>> --- a/drivers/platform/x86/intel/wmi/thunderbolt.c
>>>> +++ b/drivers/platform/x86/intel/wmi/thunderbolt.c
>>>> @@ -63,6 +63,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
>>>>            .dev_groups = tbt_groups,
>>>>        },
>>>>        .id_table = intel_wmi_thunderbolt_id_table,
>>>> +    .no_singleton = true,
>>>>    };
>>>>
>>>>    module_wmi_driver(intel_wmi_thunderbolt_driver);
>>>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>>>> index 644d2fd889c0..df6f0ae6e6c7 100644
>>>> --- a/drivers/platform/x86/wmi-bmof.c
>>>> +++ b/drivers/platform/x86/wmi-bmof.c
>>>> @@ -94,6 +94,7 @@ static struct wmi_driver wmi_bmof_driver = {
>>>>        .probe = wmi_bmof_probe,
>>>>        .remove = wmi_bmof_remove,
>>>>        .id_table = wmi_bmof_id_table,
>>>> +    .no_singleton = true,
>>>>    };
>>>>
>>>>    module_wmi_driver(wmi_bmof_driver);
>>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>>> index 29dfe52eb802..349deced87e8 100644
>>>> --- a/drivers/platform/x86/wmi.c
>>>> +++ b/drivers/platform/x86/wmi.c
>>>> @@ -883,6 +883,18 @@ static int wmi_dev_probe(struct device *dev)
>>>>        struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
>>>>        int ret = 0;
>>>>
>>>> +    /* Some older WMI drivers will break if instantiated multiple times,
>>>> +     * so they are blocked from probing WMI devices with a duplicated GUID.
>>>> +     *
>>>> +     * New WMI drivers should support being instantiated multiple times.
>>>> +     */
>>>> +    if (test_bit(WMI_GUID_DUPLICATED, &wblock->flags) && !wdriver->no_singleton) {
>>>> +        dev_warn(dev, "Legacy driver %s cannot be instantiated multiple times\n",
>>>> +             dev->driver->name);
>>>> +
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>>        if (wdriver->notify) {
>>>>            if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
>>>>                return -ENODEV;
>>>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>>>> index 781958310bfb..63cca3b58d6d 100644
>>>> --- a/include/linux/wmi.h
>>>> +++ b/include/linux/wmi.h
>>>> @@ -49,6 +49,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
>>>>     * @driver: Driver model structure
>>>>     * @id_table: List of WMI GUIDs supported by this driver
>>>>     * @no_notify_data: Driver supports WMI events which provide no event data
>>>> + * @no_singleton: Driver can be instantiated multiple times
>>>>     * @probe: Callback for device binding
>>>>     * @remove: Callback for device unbinding
>>>>     * @notify: Callback for receiving WMI events
>>>> @@ -59,6 +60,7 @@ struct wmi_driver {
>>>>        struct device_driver driver;
>>>>        const struct wmi_device_id *id_table;
>>>>        bool no_notify_data;
>>>> +    bool no_singleton;
>>>>
>>>>        int (*probe)(struct wmi_device *wdev, const void *context);
>>>>        void (*remove)(struct wmi_device *wdev);
>>>> --
>>>> 2.39.2
>>>>
>

[PATCH 3/3] platform/x86: wmi: Remove obsolete duplicate GUID allowlist

[Armin Wolf]

The whitelist-based approach for preventing older WMI drivers from
being instantiated multiple times has many drawbacks:

- uses cannot see all available WMI devices (if not whitelisted)
- whitelisting a WMI driver requires changes in the WMI driver core
- maintenance burden for driver and subsystem developers

Since the WMI driver core already takes care that older WMI drivers
are not being instantiated multiple times, remove the now redundant
whitelist.

Tested on a ASUS Prime B650-Plus.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 39 --------------------------------------
 1 file changed, 39 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 349deced87e8..1920e115da89 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -90,16 +90,6 @@ 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 */
-	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
-	"44FADEB1-B204-40F2-8581-394BBDC1B651",	/* intel-wmi-sbl-fw-update */
-	"86CCFD48-205E-4A77-9C48-2021CBEDE341",	/* intel-wmi-thunderbolt */
-	"F1DDEE52-063C-4784-A11E-8A06684B9B01",	/* dell-smm-hwmon */
-	NULL
-};
-
 #define dev_to_wblock(__dev)	container_of_const(__dev, struct wmi_block, dev.dev)
 #define dev_to_wdev(__dev)	container_of_const(__dev, struct wmi_device, dev)

@@ -1093,32 +1083,6 @@ static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
 	return device_add(&wdev->dev);
 }

-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++) {
-			if (guid_parse_and_compare(allow_duplicates[i], guid))
-				return false;
-		}
-		if (guid_equal(&wblock->gblock.guid, guid)) {
-			/*
-			 * Because we historically didn't track the relationship
-			 * between GUIDs and ACPI nodes, we don't know whether
-			 * 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));
-			return true;
-		}
-	}
-
-	return false;
-}
-
 /*
  * Parse the _WDG method for the GUID data blocks
  */
@@ -1157,9 +1121,6 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 			continue;
 		}

-		if (guid_already_parsed_for_legacy(device, &gblock[i].guid))
-			continue;
-
 		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);
 		if (!wblock)
 			continue;
--
2.39.2

Re: [PATCH 3/3] platform/x86: wmi: Remove obsolete duplicate GUID allowlist

[Ilpo Järvinen]

On Mon, 26 Feb 2024, Armin Wolf wrote:

> The whitelist-based approach for preventing older WMI drivers from
> being instantiated multiple times has many drawbacks:
> 
> - uses cannot see all available WMI devices (if not whitelisted)
> - whitelisting a WMI driver requires changes in the WMI driver core
> - maintenance burden for driver and subsystem developers
> 
> Since the WMI driver core already takes care that older WMI drivers
> are not being instantiated multiple times, remove the now redundant
> whitelist.
> 
> Tested on a ASUS Prime B650-Plus.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 39 --------------------------------------
>  1 file changed, 39 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 349deced87e8..1920e115da89 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -90,16 +90,6 @@ 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 */
> -	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
> -	"44FADEB1-B204-40F2-8581-394BBDC1B651",	/* intel-wmi-sbl-fw-update */
> -	"86CCFD48-205E-4A77-9C48-2021CBEDE341",	/* intel-wmi-thunderbolt */
> -	"F1DDEE52-063C-4784-A11E-8A06684B9B01",	/* dell-smm-hwmon */
> -	NULL
> -};

Great work, thank you.

I've applied the series to review-ilpo.

-- 
 i.

Re: [PATCH 1/3] platform/x86: wmi: Ignore duplicated GUIDs in legacy matches

[Pali Rohár]

I think that changes in this patch series looks good. You can add my:

Acked-by: Pali Rohár <pali@kernel.org>