diff mbox series

[3/6] platform/x86: wmi: Use devres for resource handling

Message ID 20231216015601.395118-4-W_Armin@gmx.de (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86: wmi: ACPI improvements | expand

Commit Message

Armin Wolf Dec. 16, 2023, 1:55 a.m. UTC 
Use devres for cleaning up the ACPI handlers and the
WMI bus device to simplify the error handling.

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

--
2.39.2

Comments

Hans de Goede Dec. 18, 2023, 2:15 p.m. UTC | #1 
Hi Armin,

On 12/16/23 02:55, Armin Wolf wrote:
> Use devres for cleaning up the ACPI handlers and the
> WMI bus device to simplify the error handling.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 54 +++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 2120c13e1676..4306a5533842 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1236,17 +1236,33 @@ static int wmi_remove_device(struct device *dev, void *data)
> 
>  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);
> 
>  	device_for_each_child_reverse(wmi_bus_device, NULL, wmi_remove_device);
> -	device_unregister(wmi_bus_device);
> +}
> +
> +static void acpi_wmi_remove_notify_handler(void *data)
> +{
> +	struct acpi_device *acpi_device = data;
> 
>  	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY, acpi_wmi_notify_handler);
> +}
> +
> +static void acpi_wmi_remove_address_space_handler(void *data)
> +{
> +	struct acpi_device *acpi_device = data;
> +
>  	acpi_remove_address_space_handler(acpi_device->handle, ACPI_ADR_SPACE_EC,
>  					  &acpi_wmi_ec_space_handler);
>  }
> 
> +static void acpi_wmi_remove_bus_device(void *data)
> +{
> +	struct device *wmi_bus_dev = data;
> +
> +	device_unregister(wmi_bus_dev);
> +}
> +
>  static int acpi_wmi_probe(struct platform_device *device)
>  {
>  	struct acpi_device *acpi_device;
> @@ -1268,6 +1284,10 @@ static int acpi_wmi_probe(struct platform_device *device)
>  		dev_err(&device->dev, "Error installing EC region handler\n");
>  		return -ENODEV;
>  	}
> +	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_notify_handler,
> +					 acpi_device);
> +	if (error < 0)
> +		return error;

The code just installed the EC address space handler here, so AFAICT
the added action here should be acpi_wmi_remove_address_space_handler and
... (continued below)

> 
>  	status = acpi_install_notify_handler(acpi_device->handle,
>  					     ACPI_ALL_NOTIFY,
> @@ -1275,39 +1295,31 @@ static int acpi_wmi_probe(struct platform_device *device)
>  					     NULL);
>  	if (ACPI_FAILURE(status)) {
>  		dev_err(&device->dev, "Error installing notify handler\n");
> -		error = -ENODEV;
> -		goto err_remove_ec_handler;
> +		return -ENODEV;
>  	}
> +	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_address_space_handler,
> +					 acpi_device);

The added action here should be acpi_wmi_remove_notify_handler ?

Otherwise this looks good to me (and so do patches 1-2).

Regards,

Hans


> +	if (error < 0)
> +		return error;
> 
>  	wmi_bus_dev = device_create(&wmi_bus_class, &device->dev, MKDEV(0, 0),
>  				    NULL, "wmi_bus-%s", dev_name(&device->dev));
>  	if (IS_ERR(wmi_bus_dev)) {
> -		error = PTR_ERR(wmi_bus_dev);
> -		goto err_remove_notify_handler;
> +		return PTR_ERR(wmi_bus_dev);
>  	}
> +	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_bus_device, wmi_bus_dev);
> +	if (error < 0)
> +		return error;
> +
>  	dev_set_drvdata(&device->dev, wmi_bus_dev);
> 
>  	error = parse_wdg(wmi_bus_dev, device);
>  	if (error) {
>  		pr_err("Failed to parse WDG method\n");
> -		goto err_remove_busdev;
> +		return error;
>  	}
> 
>  	return 0;
> -
> -err_remove_busdev:
> -	device_unregister(wmi_bus_dev);
> -
> -err_remove_notify_handler:
> -	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
> -				   acpi_wmi_notify_handler);
> -
> -err_remove_ec_handler:
> -	acpi_remove_address_space_handler(acpi_device->handle,
> -					  ACPI_ADR_SPACE_EC,
> -					  &acpi_wmi_ec_space_handler);
> -
> -	return error;
>  }
> 
>  int __must_check __wmi_driver_register(struct wmi_driver *driver,
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 2120c13e1676..4306a5533842 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1236,17 +1236,33 @@  static int wmi_remove_device(struct device *dev, void *data)

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

 	device_for_each_child_reverse(wmi_bus_device, NULL, wmi_remove_device);
-	device_unregister(wmi_bus_device);
+}
+
+static void acpi_wmi_remove_notify_handler(void *data)
+{
+	struct acpi_device *acpi_device = data;

 	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY, acpi_wmi_notify_handler);
+}
+
+static void acpi_wmi_remove_address_space_handler(void *data)
+{
+	struct acpi_device *acpi_device = data;
+
 	acpi_remove_address_space_handler(acpi_device->handle, ACPI_ADR_SPACE_EC,
 					  &acpi_wmi_ec_space_handler);
 }

+static void acpi_wmi_remove_bus_device(void *data)
+{
+	struct device *wmi_bus_dev = data;
+
+	device_unregister(wmi_bus_dev);
+}
+
 static int acpi_wmi_probe(struct platform_device *device)
 {
 	struct acpi_device *acpi_device;
@@ -1268,6 +1284,10 @@  static int acpi_wmi_probe(struct platform_device *device)
 		dev_err(&device->dev, "Error installing EC region handler\n");
 		return -ENODEV;
 	}
+	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_notify_handler,
+					 acpi_device);
+	if (error < 0)
+		return error;

 	status = acpi_install_notify_handler(acpi_device->handle,
 					     ACPI_ALL_NOTIFY,
@@ -1275,39 +1295,31 @@  static int acpi_wmi_probe(struct platform_device *device)
 					     NULL);
 	if (ACPI_FAILURE(status)) {
 		dev_err(&device->dev, "Error installing notify handler\n");
-		error = -ENODEV;
-		goto err_remove_ec_handler;
+		return -ENODEV;
 	}
+	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_address_space_handler,
+					 acpi_device);
+	if (error < 0)
+		return error;

 	wmi_bus_dev = device_create(&wmi_bus_class, &device->dev, MKDEV(0, 0),
 				    NULL, "wmi_bus-%s", dev_name(&device->dev));
 	if (IS_ERR(wmi_bus_dev)) {
-		error = PTR_ERR(wmi_bus_dev);
-		goto err_remove_notify_handler;
+		return PTR_ERR(wmi_bus_dev);
 	}
+	error = devm_add_action_or_reset(&device->dev, acpi_wmi_remove_bus_device, wmi_bus_dev);
+	if (error < 0)
+		return error;
+
 	dev_set_drvdata(&device->dev, wmi_bus_dev);

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

 	return 0;
-
-err_remove_busdev:
-	device_unregister(wmi_bus_dev);
-
-err_remove_notify_handler:
-	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
-				   acpi_wmi_notify_handler);
-
-err_remove_ec_handler:
-	acpi_remove_address_space_handler(acpi_device->handle,
-					  ACPI_ADR_SPACE_EC,
-					  &acpi_wmi_ec_space_handler);
-
-	return error;
 }

 int __must_check __wmi_driver_register(struct wmi_driver *driver,