[PATCH 0/6] Preparations for fixing WMI reprobing issue

[PATCH 0/6] Preparations for fixing WMI reprobing issue

[Armin Wolf]

This patch series contains preparations for fixing the WMI reprobing
issue discussed here:

https://lkml.kernel.org/1252c8fb-8d5f-98ad-b24a-5fabec2e1c8b@gmx.de

It mainly aims to decouple some parts of the WMI subsystem from the
wmi_block_list, which is going to get replaced in the future. It also
fixes some issues, like a probe failure when failing to register WMI
devices and a potential issue when opening the wmi char device.

The changes where tested on a Dell Inspiron 3505 and appear to work,
but additional feedback especially on the third patch is appreciated.

Armin Wolf (6):
  platform/x86: wmi: Update MAINTAINERS entry
  platform/x86: wmi: Decouple probe deferring from wmi_block_list
  platform/x86: wmi: Fix refcounting of WMI devices in legacy functions
  platform/x86: wmi: Fix probe failure when failing to register WMI
    devices
  platform/x86: wmi: Fix opening of char device
  platform/x86: wmi: Decouple WMI device removal from wmi_block_list

 MAINTAINERS                |   3 +-
 drivers/platform/x86/wmi.c | 263 ++++++++++++++++++++++---------------
 2 files changed, 159 insertions(+), 107 deletions(-)

--
2.39.2

[PATCH 1/6] platform/x86: wmi: Update MAINTAINERS entry

[Armin Wolf]

Since 2011, the WMI subsystem is marked as orphaned,
which means that a important part of platform support
for modern notebooks and even desktops is receiving
not enough maintenance.
So i decided to take over the maintenance of the WMI
subsystem.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..ba309dea6e4e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -378,8 +378,9 @@ F:	drivers/acpi/viot.c
 F:	include/linux/acpi_viot.h

 ACPI WMI DRIVER
+M:	Armin Wolf <W_Armin@gmx.de>
 L:	platform-driver-x86@vger.kernel.org
-S:	Orphan
+S:	Maintained
 F:	Documentation/driver-api/wmi.rst
 F:	Documentation/wmi/
 F:	drivers/platform/x86/wmi.c
--
2.39.2

Re: [PATCH 1/6] platform/x86: wmi: Update MAINTAINERS entry

[Hans de Goede]

Hi,

On 10/8/23 01:39, Armin Wolf wrote:
> Since 2011, the WMI subsystem is marked as orphaned,
> which means that a important part of platform support
> for modern notebooks and even desktops is receiving
> not enough maintenance.
> So i decided to take over the maintenance of the WMI
> subsystem.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch/series, I've applied this patch
(series) to the pdx86 fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

Note it will show up in the pdx86 fixes branch once I've pushed
my local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans




> ---
>  MAINTAINERS | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..ba309dea6e4e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -378,8 +378,9 @@ F:	drivers/acpi/viot.c
>  F:	include/linux/acpi_viot.h
> 
>  ACPI WMI DRIVER
> +M:	Armin Wolf <W_Armin@gmx.de>
>  L:	platform-driver-x86@vger.kernel.org
> -S:	Orphan
> +S:	Maintained
>  F:	Documentation/driver-api/wmi.rst
>  F:	Documentation/wmi/
>  F:	drivers/platform/x86/wmi.c
> --
> 2.39.2
> 

[PATCH 2/6] platform/x86: wmi: Decouple probe deferring from wmi_block_list

[Armin Wolf]

Many aggregate WMI drivers do not use -EPROBE_DEFER when they
cannot find a WMI device during probe, instead they require
all WMI devices associated with an platform device to become
available at once. This is currently achieved by adding those
WMI devices to the wmi_block_list before they are registered,
which is then used by the deprecated GUID-based functions to
search for WMI devices.
Replace this approach with a device link which defers probing
of the WMI device until the associated platform device has finished
probing (and has registered all WMI devices). New aggregate WMI
drivers should not rely on this behaviour.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index a78ddd83cda0..1dbef16acdeb 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1221,6 +1221,26 @@ static int wmi_create_device(struct device *wmi_bus_dev,
 	return 0;
 }

+static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
+{
+	struct device_link *link;
+
+	/*
+	 * Many aggregate WMI drivers do not use -EPROBE_DEFER when they
+	 * are unable to find a WMI device during probe, instead they require
+	 * all WMI devices associated with an platform device to become available
+	 * at once. This device link thus prevents WMI drivers from probing until
+	 * the associated platform device has finished probing (and has registered
+	 * all discovered WMI devices).
+	 */
+
+	link = device_link_add(&wdev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
+	if (!link)
+		return -EINVAL;
+
+	return device_add(&wdev->dev);
+}
+
 static void wmi_free_devices(struct acpi_device *device)
 {
 	struct wmi_block *wblock, *next;
@@ -1263,11 +1283,12 @@ static bool guid_already_parsed_for_legacy(struct acpi_device *device, const gui
 /*
  * Parse the _WDG method for the GUID data blocks
  */
-static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
+static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 {
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
 	const struct guid_block *gblock;
-	struct wmi_block *wblock, *next;
+	struct wmi_block *wblock;
 	union acpi_object *obj;
 	acpi_status status;
 	int retval = 0;
@@ -1317,22 +1338,14 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
 			wblock->handler = wmi_notify_debug;
 			wmi_method_enable(wblock, true);
 		}
-	}

-	/*
-	 * Now that all of the devices are created, add them to the
-	 * device tree and probe subdrivers.
-	 */
-	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
-		if (wblock->acpi_device != device)
-			continue;
-
-		retval = device_add(&wblock->dev.dev);
+		retval = wmi_add_device(pdev, &wblock->dev);
 		if (retval) {
 			dev_err(wmi_bus_dev, "failed to register %pUL\n",
 				&wblock->gblock.guid);
 			if (debug_event)
 				wmi_method_enable(wblock, false);
+
 			list_del(&wblock->list);
 			put_device(&wblock->dev.dev);
 		}
@@ -1487,7 +1500,7 @@ static int acpi_wmi_probe(struct platform_device *device)
 	}
 	dev_set_drvdata(&device->dev, wmi_bus_dev);

-	error = parse_wdg(wmi_bus_dev, acpi_device);
+	error = parse_wdg(wmi_bus_dev, device);
 	if (error) {
 		pr_err("Failed to parse WDG method\n");
 		goto err_remove_busdev;
--
2.39.2

Re: [PATCH 2/6] platform/x86: wmi: Decouple probe deferring from wmi_block_list

[Ilpo Järvinen]

On Sun, 8 Oct 2023, Armin Wolf wrote:

> Many aggregate WMI drivers do not use -EPROBE_DEFER when they
> cannot find a WMI device during probe, instead they require
> all WMI devices associated with an platform device to become
> available at once. This is currently achieved by adding those
> WMI devices to the wmi_block_list before they are registered,
> which is then used by the deprecated GUID-based functions to
> search for WMI devices.
> Replace this approach with a device link which defers probing

To make these more readable, you should break the paragraphs with an empty 
line, not just using a newline.

-- 
 i.

> of the WMI device until the associated platform device has finished
> probing (and has registered all WMI devices). New aggregate WMI
> drivers should not rely on this behaviour.



> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 39 +++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index a78ddd83cda0..1dbef16acdeb 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1221,6 +1221,26 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>  	return 0;
>  }
> 
> +static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
> +{
> +	struct device_link *link;
> +
> +	/*
> +	 * Many aggregate WMI drivers do not use -EPROBE_DEFER when they
> +	 * are unable to find a WMI device during probe, instead they require
> +	 * all WMI devices associated with an platform device to become available
> +	 * at once. This device link thus prevents WMI drivers from probing until
> +	 * the associated platform device has finished probing (and has registered
> +	 * all discovered WMI devices).
> +	 */
> +
> +	link = device_link_add(&wdev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> +	if (!link)
> +		return -EINVAL;
> +
> +	return device_add(&wdev->dev);
> +}
> +
>  static void wmi_free_devices(struct acpi_device *device)
>  {
>  	struct wmi_block *wblock, *next;
> @@ -1263,11 +1283,12 @@ static bool guid_already_parsed_for_legacy(struct acpi_device *device, const gui
>  /*
>   * Parse the _WDG method for the GUID data blocks
>   */
> -static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
> +static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  {
> +	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
>  	const struct guid_block *gblock;
> -	struct wmi_block *wblock, *next;
> +	struct wmi_block *wblock;
>  	union acpi_object *obj;
>  	acpi_status status;
>  	int retval = 0;
> @@ -1317,22 +1338,14 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
>  			wblock->handler = wmi_notify_debug;
>  			wmi_method_enable(wblock, true);
>  		}
> -	}
> 
> -	/*
> -	 * Now that all of the devices are created, add them to the
> -	 * device tree and probe subdrivers.
> -	 */
> -	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
> -		if (wblock->acpi_device != device)
> -			continue;
> -
> -		retval = device_add(&wblock->dev.dev);
> +		retval = wmi_add_device(pdev, &wblock->dev);
>  		if (retval) {
>  			dev_err(wmi_bus_dev, "failed to register %pUL\n",
>  				&wblock->gblock.guid);
>  			if (debug_event)
>  				wmi_method_enable(wblock, false);
> +
>  			list_del(&wblock->list);
>  			put_device(&wblock->dev.dev);
>  		}
> @@ -1487,7 +1500,7 @@ static int acpi_wmi_probe(struct platform_device *device)
>  	}
>  	dev_set_drvdata(&device->dev, wmi_bus_dev);
> 
> -	error = parse_wdg(wmi_bus_dev, acpi_device);
> +	error = parse_wdg(wmi_bus_dev, device);
>  	if (error) {
>  		pr_err("Failed to parse WDG method\n");
>  		goto err_remove_busdev;
> --
> 2.39.2
> 

[PATCH 3/6] platform/x86: wmi: Fix refcounting of WMI devices in legacy functions

[Armin Wolf]

Until now, legacy GUID-based functions where using find_guid() when
searching for WMI devices, which did no refcounting on the returned
WMI device. This meant that the WMI device could disappear at any
moment, potentially leading to various errors. Fix this by using
bus_find_device() which returns an actual reference to the found
WMI device.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 1dbef16acdeb..e3984801883a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -109,33 +109,13 @@ static const char * const allow_duplicates[] = {
 	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)
+
 /*
  * GUID parsing functions
  */

-static acpi_status find_guid(const char *guid_string, struct wmi_block **out)
-{
-	guid_t guid_input;
-	struct wmi_block *wblock;
-
-	if (!guid_string)
-		return AE_BAD_PARAMETER;
-
-	if (guid_parse(guid_string, &guid_input))
-		return AE_BAD_PARAMETER;
-
-	list_for_each_entry(wblock, &wmi_block_list, list) {
-		if (guid_equal(&wblock->gblock.guid, &guid_input)) {
-			if (out)
-				*out = wblock;
-
-			return AE_OK;
-		}
-	}
-
-	return AE_NOT_FOUND;
-}
-
 static bool guid_parse_and_compare(const char *string, const guid_t *guid)
 {
 	guid_t guid_input;
@@ -245,6 +225,41 @@ static acpi_status get_event_data(const struct wmi_block *wblock, struct acpi_bu
 	return acpi_evaluate_object(wblock->acpi_device->handle, "_WED", &input, out);
 }

+static int wmidev_match_guid(struct device *dev, const void *data)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+	const guid_t *guid = data;
+
+	if (guid_equal(guid, &wblock->gblock.guid))
+		return 1;
+
+	return 0;
+}
+
+static struct bus_type wmi_bus_type;
+
+static struct wmi_device *wmi_find_device_by_guid(const char *guid_string)
+{
+	struct device *dev;
+	guid_t guid;
+	int ret;
+
+	ret = guid_parse(guid_string, &guid);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	dev = bus_find_device(&wmi_bus_type, NULL, &guid, wmidev_match_guid);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return dev_to_wdev(dev);
+}
+
+static void wmi_device_put(struct wmi_device *wdev)
+{
+	put_device(&wdev->dev);
+}
+
 /*
  * Exported WMI functions
  */
@@ -279,18 +294,17 @@ EXPORT_SYMBOL_GPL(set_required_buffer_size);
  */
 int wmi_instance_count(const char *guid_string)
 {
-	struct wmi_block *wblock;
-	acpi_status status;
+	struct wmi_device *wdev;
+	int ret;

-	status = find_guid(guid_string, &wblock);
-	if (ACPI_FAILURE(status)) {
-		if (status == AE_BAD_PARAMETER)
-			return -EINVAL;
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
+		return PTR_ERR(wdev);

-		return -ENODEV;
-	}
+	ret = wmidev_instance_count(wdev);
+	wmi_device_put(wdev);

-	return wmidev_instance_count(&wblock->dev);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(wmi_instance_count);

@@ -325,15 +339,18 @@ EXPORT_SYMBOL_GPL(wmidev_instance_count);
 acpi_status wmi_evaluate_method(const char *guid_string, u8 instance, u32 method_id,
 				const struct acpi_buffer *in, struct acpi_buffer *out)
 {
-	struct wmi_block *wblock = NULL;
+	struct wmi_device *wdev;
 	acpi_status status;

-	status = find_guid(guid_string, &wblock);
-	if (ACPI_FAILURE(status))
-		return status;
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
+		return AE_ERROR;
+
+	status = wmidev_evaluate_method(wdev, instance, method_id, in, out);

-	return wmidev_evaluate_method(&wblock->dev, instance, method_id,
-				      in, out);
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_evaluate_method);

@@ -472,13 +489,19 @@ acpi_status wmi_query_block(const char *guid_string, u8 instance,
 			    struct acpi_buffer *out)
 {
 	struct wmi_block *wblock;
+	struct wmi_device *wdev;
 	acpi_status status;

-	status = find_guid(guid_string, &wblock);
-	if (ACPI_FAILURE(status))
-		return status;
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
+		return AE_ERROR;

-	return __query_block(wblock, instance, out);
+	wblock = container_of(wdev, struct wmi_block, dev);
+	status = __query_block(wblock, instance, out);
+
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_query_block);

@@ -516,8 +539,9 @@ EXPORT_SYMBOL_GPL(wmidev_block_query);
 acpi_status wmi_set_block(const char *guid_string, u8 instance,
 			  const struct acpi_buffer *in)
 {
-	struct wmi_block *wblock = NULL;
+	struct wmi_block *wblock;
 	struct guid_block *block;
+	struct wmi_device *wdev;
 	acpi_handle handle;
 	struct acpi_object_list input;
 	union acpi_object params[2];
@@ -527,19 +551,26 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 	if (!in)
 		return AE_BAD_DATA;

-	status = find_guid(guid_string, &wblock);
-	if (ACPI_FAILURE(status))
-		return status;
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
+		return AE_ERROR;

+	wblock = container_of(wdev, struct wmi_block, dev);
 	block = &wblock->gblock;
 	handle = wblock->acpi_device->handle;

-	if (block->instance_count <= instance)
-		return AE_BAD_PARAMETER;
+	if (block->instance_count <= instance) {
+		status = AE_BAD_PARAMETER;
+
+		goto err_wdev_put;
+	}

 	/* Check GUID is a data block */
-	if (block->flags & (ACPI_WMI_EVENT | ACPI_WMI_METHOD))
-		return AE_ERROR;
+	if (block->flags & (ACPI_WMI_EVENT | ACPI_WMI_METHOD)) {
+		status = AE_ERROR;
+
+		goto err_wdev_put;
+	}

 	input.count = 2;
 	input.pointer = params;
@@ -551,7 +582,12 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,

 	get_acpi_method_name(wblock, 'S', method);

-	return acpi_evaluate_object(handle, method, &input, NULL);
+	status = acpi_evaluate_object(handle, method, &input, NULL);
+
+err_wdev_put:
+	wmi_device_put(wdev);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(wmi_set_block);

@@ -742,7 +778,15 @@ EXPORT_SYMBOL_GPL(wmi_get_event_data);
  */
 bool wmi_has_guid(const char *guid_string)
 {
-	return ACPI_SUCCESS(find_guid(guid_string, NULL));
+	struct wmi_device *wdev;
+
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
+		return false;
+
+	wmi_device_put(wdev);
+
+	return true;
 }
 EXPORT_SYMBOL_GPL(wmi_has_guid);

@@ -756,20 +800,23 @@ EXPORT_SYMBOL_GPL(wmi_has_guid);
  */
 char *wmi_get_acpi_device_uid(const char *guid_string)
 {
-	struct wmi_block *wblock = NULL;
-	acpi_status status;
+	struct wmi_block *wblock;
+	struct wmi_device *wdev;
+	char *uid;

-	status = find_guid(guid_string, &wblock);
-	if (ACPI_FAILURE(status))
+	wdev = wmi_find_device_by_guid(guid_string);
+	if (IS_ERR(wdev))
 		return NULL;

-	return acpi_device_uid(wblock->acpi_device);
+	wblock = container_of(wdev, struct wmi_block, dev);
+	uid = acpi_device_uid(wblock->acpi_device);
+
+	wmi_device_put(wdev);
+
+	return uid;
 }
 EXPORT_SYMBOL_GPL(wmi_get_acpi_device_uid);

-#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)
-
 static inline struct wmi_driver *drv_to_wdrv(struct device_driver *drv)
 {
 	return container_of(drv, struct wmi_driver, driver);
--
2.39.2

[PATCH 4/6] platform/x86: wmi: Fix probe failure when failing to register WMI devices

[Armin Wolf]

When a WMI device besides the first one somehow fails to register, retval
is returned while still containing a negative error code. This causes the
ACPI device failing to probe, leaving behind zombie WMI devices leading
to various errors later.
Fix this by handling the single error path separately and return 0 after
trying to register all WMI devices. Also continue to register WMI devices
even if some fail to allocate.

Fixes: 6ee50aaa9a20 ("platform/x86: wmi: Instantiate all devices before adding them")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e3984801883a..ab24ea9ffc9a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1338,8 +1338,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 	struct wmi_block *wblock;
 	union acpi_object *obj;
 	acpi_status status;
-	int retval = 0;
 	u32 i, total;
+	int retval;

 	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
 	if (ACPI_FAILURE(status))
@@ -1350,8 +1350,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 		return -ENXIO;

 	if (obj->type != ACPI_TYPE_BUFFER) {
-		retval = -ENXIO;
-		goto out_free_pointer;
+		kfree(obj);
+		return -ENXIO;
 	}

 	gblock = (const struct guid_block *)obj->buffer.pointer;
@@ -1366,8 +1366,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)

 		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);
 		if (!wblock) {
-			retval = -ENOMEM;
-			break;
+			dev_err(wmi_bus_dev, "Failed to allocate %pUL\n", &gblock[i].guid);
+			continue;
 		}

 		wblock->acpi_device = device;
@@ -1398,9 +1398,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 		}
 	}

-out_free_pointer:
-	kfree(out.pointer);
-	return retval;
+	kfree(obj);
+
+	return 0;
 }

 /*
--
2.39.2

Re: [PATCH 4/6] platform/x86: wmi: Fix probe failure when failing to register WMI devices

[Ilpo Järvinen]

On Sun, 8 Oct 2023, Armin Wolf wrote:

> When a WMI device besides the first one somehow fails to register, retval
> is returned while still containing a negative error code. This causes the
> ACPI device failing to probe, leaving behind zombie WMI devices leading
> to various errors later.
> Fix this by handling the single error path separately and return 0 after
> trying to register all WMI devices. Also continue to register WMI devices
> even if some fail to allocate.

I think the usual approach would be to unroll all registerations done so 
far when an error occurs while registering n devices.

Do you Hans have something to add what would be the best course of action 
here?

-- 
 i.

> Fixes: 6ee50aaa9a20 ("platform/x86: wmi: Instantiate all devices before adding them")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index e3984801883a..ab24ea9ffc9a 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1338,8 +1338,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  	struct wmi_block *wblock;
>  	union acpi_object *obj;
>  	acpi_status status;
> -	int retval = 0;
>  	u32 i, total;
> +	int retval;
> 
>  	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
>  	if (ACPI_FAILURE(status))
> @@ -1350,8 +1350,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  		return -ENXIO;
> 
>  	if (obj->type != ACPI_TYPE_BUFFER) {
> -		retval = -ENXIO;
> -		goto out_free_pointer;
> +		kfree(obj);
> +		return -ENXIO;
>  	}
> 
>  	gblock = (const struct guid_block *)obj->buffer.pointer;
> @@ -1366,8 +1366,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
> 
>  		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);
>  		if (!wblock) {
> -			retval = -ENOMEM;
> -			break;
> +			dev_err(wmi_bus_dev, "Failed to allocate %pUL\n", &gblock[i].guid);
> +			continue;
>  		}
> 
>  		wblock->acpi_device = device;
> @@ -1398,9 +1398,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  		}
>  	}
> 
> -out_free_pointer:
> -	kfree(out.pointer);
> -	return retval;
> +	kfree(obj);
> +
> +	return 0;
>  }
> 
>  /*
> --
> 2.39.2
> 

Re: [PATCH 4/6] platform/x86: wmi: Fix probe failure when failing to register WMI devices

[Armin Wolf]

Am 12.10.23 um 18:32 schrieb Ilpo Järvinen:

> On Sun, 8 Oct 2023, Armin Wolf wrote:
>
>> When a WMI device besides the first one somehow fails to register, retval
>> is returned while still containing a negative error code. This causes the
>> ACPI device failing to probe, leaving behind zombie WMI devices leading
>> to various errors later.
>> Fix this by handling the single error path separately and return 0 after
>> trying to register all WMI devices. Also continue to register WMI devices
>> even if some fail to allocate.
> I think the usual approach would be to unroll all registerations done so
> far when an error occurs while registering n devices.

I agree, however the surrounding code unrolls only the WMI device registration,
so i kept it that way. After all, this patch focuses on fixing the "zombie" WMI devices
problem, so changing the code to unroll all registrations should be done in a separate
patch IMHO.

Armin Wolf

> Do you Hans have something to add what would be the best course of action
> here?
>

[PATCH 5/6] platform/x86: wmi: Fix opening of char device

[Armin Wolf]

Since commit fa1f68db6ca7 ("drivers: misc: pass miscdevice pointer via
file private data"), the miscdevice stores a pointer to itself inside
filp->private_data, which means that private_data will not be NULL when
wmi_char_open() is called. This might cause memory corruption should
wmi_char_open() be unable to find its driver, something which can happen
when the associated WMI device is deleted in wmi_free_devices().
Fix this by using the miscdevice pointer to retrieve the WMI device data
associated with a char device using container_of(). This also avoids
wmi_char_open() picking a wrong WMI device bound to a driver with the
same name as the original driver.

Fixes: 44b6b7661132 ("platform/x86: wmi: create userspace interface for drivers")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index ab24ea9ffc9a..6b3b2fe464d2 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -958,21 +958,13 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 }
 static int wmi_char_open(struct inode *inode, struct file *filp)
 {
-	const char *driver_name = filp->f_path.dentry->d_iname;
-	struct wmi_block *wblock;
-	struct wmi_block *next;
-
-	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
-		if (!wblock->dev.dev.driver)
-			continue;
-		if (strcmp(driver_name, wblock->dev.dev.driver->name) == 0) {
-			filp->private_data = wblock;
-			break;
-		}
-	}
+	/*
+	 * The miscdevice already stores a pointer to itself
+	 * inside filp->private_data
+	 */
+	struct wmi_block *wblock = container_of(filp->private_data, struct wmi_block, char_dev);

-	if (!filp->private_data)
-		return -ENODEV;
+	filp->private_data = wblock;

 	return nonseekable_open(inode, filp);
 }
--
2.39.2

[PATCH 6/6] platform/x86: wmi: Decouple WMI device removal from wmi_block_list

[Armin Wolf]

Use device_for_each_child_reverse() to find and unregister WMI devices
belonging to a WMI bus device instead of iterating thru the entire
wmi_block_list.

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

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 6b3b2fe464d2..5c27b4aa9690 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1280,19 +1280,6 @@ static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
 	return device_add(&wdev->dev);
 }

-static void wmi_free_devices(struct acpi_device *device)
-{
-	struct wmi_block *wblock, *next;
-
-	/* Delete devices for all the GUIDs */
-	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
-		if (wblock->acpi_device == device) {
-			list_del(&wblock->list);
-			device_unregister(&wblock->dev.dev);
-		}
-	}
-}
-
 static bool guid_already_parsed_for_legacy(struct acpi_device *device, const guid_t *guid)
 {
 	struct wmi_block *wblock;
@@ -1487,16 +1474,28 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
 		event, 0);
 }

+static int wmi_remove_device(struct device *dev, void *data)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	list_del(&wblock->list);
+	device_unregister(dev);
+
+	return 0;
+}
+
 static void acpi_wmi_remove(struct platform_device *device)
 {
 	struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
+	struct device *wmi_bus_device = dev_get_drvdata(&device->dev);

 	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
 				   acpi_wmi_notify_handler);
 	acpi_remove_address_space_handler(acpi_device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
-	wmi_free_devices(acpi_device);
-	device_unregister(dev_get_drvdata(&device->dev));
+
+	device_for_each_child_reverse(wmi_bus_device, NULL, wmi_remove_device);
+	device_unregister(wmi_bus_device);
 }

 static int acpi_wmi_probe(struct platform_device *device)
--
2.39.2