* [PATCH 0/6] platform/x86: wmi: ACPI improvements
@ 2023-12-16 1:55 Armin Wolf
2023-12-16 1:55 ` [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler Armin Wolf
` (6 more replies)
0 siblings, 7 replies; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:55 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
This patch series improves the ACPI handling inside the ACPI WMI driver.
The first patch removes an unused variable, while the second patch
changes the order in which the ACPI handlers are removed on shutdown.
The third patch simplifies the error handling during probe by using
devres to manage devie resources, while the next two patches decouple
the ACPI notify handler from the wmi_block_list. The last patch
simplifies yet another ACPI-related function.
All patches have been tested on a Dell Inspiron 3505 and appear to work.
Armin Wolf (6):
platform/x86: wmi: Remove unused variable in address space handler
platform/x86: wmi: Remove ACPI handlers after WMI devices
platform/x86: wmi: Use devres for resource handling
platform/x86: wmi: Create WMI bus device first
platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list
platform/x86: wmi: Simplify get_subobj_info()
drivers/platform/x86/wmi.c | 143 ++++++++++++++++++-------------------
1 file changed, 71 insertions(+), 72 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
@ 2023-12-16 1:55 ` Armin Wolf
2023-12-18 12:05 ` Ilpo Järvinen
2023-12-16 1:55 ` [PATCH 2/6] platform/x86: wmi: Remove ACPI handlers after WMI devices Armin Wolf
` (5 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:55 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
The variable "i" is always zero and only used in shift operations.
Remove it to make the code more readable.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/wmi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7303702290e5..906d3a2831ae 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1144,7 +1144,7 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
u32 bits, u64 *value,
void *handler_context, void *region_context)
{
- int result = 0, i = 0;
+ int result = 0;
u8 temp = 0;
if ((address > 0xFF) || !value)
@@ -1158,9 +1158,9 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
if (function == ACPI_READ) {
result = ec_read(address, &temp);
- (*value) |= ((u64)temp) << i;
+ *value = temp;
} else {
- temp = 0xff & ((*value) >> i);
+ temp = 0xff & *value;
result = ec_write(address, temp);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/6] platform/x86: wmi: Remove ACPI handlers after WMI devices
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
2023-12-16 1:55 ` [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler Armin Wolf
@ 2023-12-16 1:55 ` Armin Wolf
2023-12-16 1:55 ` [PATCH 3/6] platform/x86: wmi: Use devres for resource handling Armin Wolf
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:55 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
When removing the ACPI notify/address space handlers, the WMI devices
are still active and might still depend on ACPI EC access or
WMI events.
Fix this by removing the ACPI handlers after all WMI devices
associated with an ACPI device have been removed.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/wmi.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 906d3a2831ae..2120c13e1676 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1239,13 +1239,12 @@ 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);
-
device_for_each_child_reverse(wmi_bus_device, NULL, wmi_remove_device);
device_unregister(wmi_bus_device);
+
+ 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);
}
static int acpi_wmi_probe(struct platform_device *device)
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/6] platform/x86: wmi: Use devres for resource handling
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
2023-12-16 1:55 ` [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler Armin Wolf
2023-12-16 1:55 ` [PATCH 2/6] platform/x86: wmi: Remove ACPI handlers after WMI devices Armin Wolf
@ 2023-12-16 1:55 ` Armin Wolf
2023-12-18 14:15 ` Hans de Goede
2023-12-16 1:55 ` [PATCH 4/6] platform/x86: wmi: Create WMI bus device first Armin Wolf
` (3 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:55 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
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;
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,
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/6] platform/x86: wmi: Create WMI bus device first
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
` (2 preceding siblings ...)
2023-12-16 1:55 ` [PATCH 3/6] platform/x86: wmi: Use devres for resource handling Armin Wolf
@ 2023-12-16 1:55 ` Armin Wolf
2023-12-16 1:56 ` [PATCH 5/6] platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list Armin Wolf
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:55 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
Create the WMI bus device first so that it can be used
by the ACPI handlers.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/wmi.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4306a5533842..61849a43e2c2 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1276,6 +1276,17 @@ static int acpi_wmi_probe(struct platform_device *device)
return -ENODEV;
}
+ 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))
+ 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);
+
status = acpi_install_address_space_handler(acpi_device->handle,
ACPI_ADR_SPACE_EC,
&acpi_wmi_ec_space_handler,
@@ -1302,17 +1313,6 @@ static int acpi_wmi_probe(struct platform_device *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)) {
- 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");
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/6] platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
` (3 preceding siblings ...)
2023-12-16 1:55 ` [PATCH 4/6] platform/x86: wmi: Create WMI bus device first Armin Wolf
@ 2023-12-16 1:56 ` Armin Wolf
2023-12-16 1:56 ` [PATCH 6/6] platform/x86: wmi: Simplify get_subobj_info() Armin Wolf
2023-12-18 14:31 ` [PATCH 0/6] platform/x86: wmi: ACPI improvements Hans de Goede
6 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:56 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
Currently, the ACPI notify handler searches all WMI devices for
a matching WMI event device. This is inefficient since only WMI devices
associated with the notified ACPI device need to be searched.
Use the WMI bus device and device_for_each_child() to search for
a matching WMI event device instead.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/wmi.c | 46 +++++++++++++++++---------------------
1 file changed, 20 insertions(+), 26 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 61849a43e2c2..6b581c772fd1 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1176,24 +1176,13 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
}
}
-static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
- void *context)
+static int wmi_notify_device(struct device *dev, void *data)
{
- struct wmi_block *wblock = NULL, *iter;
-
- list_for_each_entry(iter, &wmi_block_list, list) {
- struct guid_block *block = &iter->gblock;
-
- if (iter->acpi_device->handle == handle &&
- (block->flags & ACPI_WMI_EVENT) &&
- (block->notify_id == event)) {
- wblock = iter;
- break;
- }
- }
+ struct wmi_block *wblock = dev_to_wblock(dev);
+ u32 *event = data;
- if (!wblock)
- return;
+ if (!(wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *event))
+ return 0;
/* If a driver is bound, then notify the driver. */
if (test_bit(WMI_PROBED, &wblock->flags) && wblock->dev.dev.driver) {
@@ -1205,7 +1194,7 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
status = get_event_data(wblock, &evdata);
if (ACPI_FAILURE(status)) {
dev_warn(&wblock->dev.dev, "failed to get event data\n");
- return;
+ return -EIO;
}
}
@@ -1215,13 +1204,20 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event,
kfree(evdata.pointer);
} else if (wblock->handler) {
/* Legacy handler */
- wblock->handler(event, wblock->handler_data);
+ wblock->handler(*event, wblock->handler_data);
}
- acpi_bus_generate_netlink_event(
- wblock->acpi_device->pnp.device_class,
- dev_name(&wblock->dev.dev),
- event, 0);
+ acpi_bus_generate_netlink_event(wblock->acpi_device->pnp.device_class,
+ dev_name(&wblock->dev.dev), *event, 0);
+
+ return -EBUSY;
+}
+
+static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, void *context)
+{
+ struct device *wmi_bus_dev = context;
+
+ device_for_each_child(wmi_bus_dev, &event, wmi_notify_device);
}
static int wmi_remove_device(struct device *dev, void *data)
@@ -1300,10 +1296,8 @@ static int acpi_wmi_probe(struct platform_device *device)
if (error < 0)
return error;
- status = acpi_install_notify_handler(acpi_device->handle,
- ACPI_ALL_NOTIFY,
- acpi_wmi_notify_handler,
- NULL);
+ status = acpi_install_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
+ acpi_wmi_notify_handler, wmi_bus_dev);
if (ACPI_FAILURE(status)) {
dev_err(&device->dev, "Error installing notify handler\n");
return -ENODEV;
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/6] platform/x86: wmi: Simplify get_subobj_info()
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
` (4 preceding siblings ...)
2023-12-16 1:56 ` [PATCH 5/6] platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list Armin Wolf
@ 2023-12-16 1:56 ` Armin Wolf
2023-12-18 14:31 ` [PATCH 0/6] platform/x86: wmi: ACPI improvements Hans de Goede
6 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2023-12-16 1:56 UTC (permalink / raw
To: hdegoede, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
All callers who call get_subobj_info() with **info being NULL
should better use acpi_has_method() instead.
Convert the only caller who does this to acpi_has_method()
to drop the dummy info handling.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/wmi.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 6b581c772fd1..73570744bc33 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -132,23 +132,19 @@ static const void *find_guid_context(struct wmi_block *wblock,
static int get_subobj_info(acpi_handle handle, const char *pathname,
struct acpi_device_info **info)
{
- struct acpi_device_info *dummy_info, **info_ptr;
acpi_handle subobj_handle;
acpi_status status;
- status = acpi_get_handle(handle, (char *)pathname, &subobj_handle);
+ status = acpi_get_handle(handle, pathname, &subobj_handle);
if (status == AE_NOT_FOUND)
return -ENOENT;
- else if (ACPI_FAILURE(status))
- return -EIO;
- info_ptr = info ? info : &dummy_info;
- status = acpi_get_object_info(subobj_handle, info_ptr);
if (ACPI_FAILURE(status))
return -EIO;
- if (!info)
- kfree(dummy_info);
+ status = acpi_get_object_info(subobj_handle, info);
+ if (ACPI_FAILURE(status))
+ return -EIO;
return 0;
}
@@ -998,9 +994,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
kfree(info);
get_acpi_method_name(wblock, 'S', method);
- result = get_subobj_info(device->handle, method, NULL);
-
- if (result == 0)
+ if (acpi_has_method(device->handle, method))
wblock->dev.setable = true;
out_init:
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler
2023-12-16 1:55 ` [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler Armin Wolf
@ 2023-12-18 12:05 ` Ilpo Järvinen
0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2023-12-18 12:05 UTC (permalink / raw
To: Armin Wolf; +Cc: Hans de Goede, platform-driver-x86, LKML
[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]
On Sat, 16 Dec 2023, Armin Wolf wrote:
> The variable "i" is always zero and only used in shift operations.
> Remove it to make the code more readable.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> drivers/platform/x86/wmi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 7303702290e5..906d3a2831ae 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1144,7 +1144,7 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
> u32 bits, u64 *value,
> void *handler_context, void *region_context)
> {
> - int result = 0, i = 0;
> + int result = 0;
> u8 temp = 0;
>
> if ((address > 0xFF) || !value)
> @@ -1158,9 +1158,9 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
>
> if (function == ACPI_READ) {
> result = ec_read(address, &temp);
> - (*value) |= ((u64)temp) << i;
> + *value = temp;
> } else {
> - temp = 0xff & ((*value) >> i);
> + temp = 0xff & *value;
> result = ec_write(address, temp);
> }
Seems to have been like this already when it got introduced...
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/6] platform/x86: wmi: Use devres for resource handling
2023-12-16 1:55 ` [PATCH 3/6] platform/x86: wmi: Use devres for resource handling Armin Wolf
@ 2023-12-18 14:15 ` Hans de Goede
0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2023-12-18 14:15 UTC (permalink / raw
To: Armin Wolf, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] platform/x86: wmi: ACPI improvements
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
` (5 preceding siblings ...)
2023-12-16 1:56 ` [PATCH 6/6] platform/x86: wmi: Simplify get_subobj_info() Armin Wolf
@ 2023-12-18 14:31 ` Hans de Goede
6 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2023-12-18 14:31 UTC (permalink / raw
To: Armin Wolf, ilpo.jarvinen; +Cc: platform-driver-x86, linux-kernel
Hi Armin,
On 12/16/23 02:55, Armin Wolf wrote:
> This patch series improves the ACPI handling inside the ACPI WMI driver.
> The first patch removes an unused variable, while the second patch
> changes the order in which the ACPI handlers are removed on shutdown.
> The third patch simplifies the error handling during probe by using
> devres to manage devie resources, while the next two patches decouple
> the ACPI notify handler from the wmi_block_list. The last patch
> simplifies yet another ACPI-related function.
>
> All patches have been tested on a Dell Inspiron 3505 and appear to work.
>
> Armin Wolf (6):
> platform/x86: wmi: Remove unused variable in address space handler
> platform/x86: wmi: Remove ACPI handlers after WMI devices
> platform/x86: wmi: Use devres for resource handling
> platform/x86: wmi: Create WMI bus device first
> platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list
> platform/x86: wmi: Simplify get_subobj_info()
>
> drivers/platform/x86/wmi.c | 143 ++++++++++++++++++-------------------
> 1 file changed, 71 insertions(+), 72 deletions(-)
Thank you for the series, with the exception of the one small remark
to patch 3/7 the entire series looks good to me.
With that remark addressed you may add my:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
to the series.
Regards,
Hans
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-18 14:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-16 1:55 [PATCH 0/6] platform/x86: wmi: ACPI improvements Armin Wolf
2023-12-16 1:55 ` [PATCH 1/6] platform/x86: wmi: Remove unused variable in address space handler Armin Wolf
2023-12-18 12:05 ` Ilpo Järvinen
2023-12-16 1:55 ` [PATCH 2/6] platform/x86: wmi: Remove ACPI handlers after WMI devices Armin Wolf
2023-12-16 1:55 ` [PATCH 3/6] platform/x86: wmi: Use devres for resource handling Armin Wolf
2023-12-18 14:15 ` Hans de Goede
2023-12-16 1:55 ` [PATCH 4/6] platform/x86: wmi: Create WMI bus device first Armin Wolf
2023-12-16 1:56 ` [PATCH 5/6] platform/x86: wmi: Decouple ACPI notify handler from wmi_block_list Armin Wolf
2023-12-16 1:56 ` [PATCH 6/6] platform/x86: wmi: Simplify get_subobj_info() Armin Wolf
2023-12-18 14:31 ` [PATCH 0/6] platform/x86: wmi: ACPI improvements Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).