On Thu, 22 Aug 2024, Armin Wolf wrote:
> The current legacy WMI handlers are susceptible to picking up wrong
> WMI event data on systems where different WMI devices share some
> notification IDs.
>
> Prevent this by letting the WMI driver core taking care of retrieving
> the event data. This also simplifies the legacy WMI handlers and their
> implementation inside the WMI driver core.
>
> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> ---
> drivers/hwmon/hp-wmi-sensors.c | 17 ++--------
> drivers/platform/x86/acer-wmi.c | 16 +--------
> drivers/platform/x86/asus-wmi.c | 19 ++---------
> drivers/platform/x86/dell/dell-wmi-aio.c | 13 +------
> drivers/platform/x86/hp/hp-wmi.c | 16 +--------
> drivers/platform/x86/huawei-wmi.c | 14 +-------
> drivers/platform/x86/lg-laptop.c | 13 +------
> drivers/platform/x86/msi-wmi.c | 20 ++---------
> drivers/platform/x86/toshiba-wmi.c | 15 +--------
> drivers/platform/x86/wmi.c | 43 ++++++++++--------------
> include/linux/acpi.h | 2 +-
> 11 files changed, 34 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> index b5325d0e72b9..6892518d537c 100644
> --- a/drivers/hwmon/hp-wmi-sensors.c
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -1597,15 +1597,13 @@ static void hp_wmi_devm_notify_remove(void *ignored)
> }
>
> /* hp_wmi_notify - WMI event notification handler */
> -static void hp_wmi_notify(u32 value, void *context)
> +static void hp_wmi_notify(union acpi_object *wobj, void *context)
> {
> struct hp_wmi_info *temp_info[HP_WMI_MAX_INSTANCES] = {};
> - struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
> struct hp_wmi_sensors *state = context;
> struct device *dev = &state->wdev->dev;
> struct hp_wmi_event event = {};
> struct hp_wmi_info *fan_info;
> - union acpi_object *wobj;
> acpi_status err;
> int event_type;
> u8 count;
> @@ -1632,16 +1630,10 @@ static void hp_wmi_notify(u32 value, void *context)
>
> mutex_lock(&state->lock);
>
> - err = wmi_get_event_data(value, &out);
> - if (ACPI_FAILURE(err))
> - goto out_unlock;
> -
> - wobj = out.pointer;
> -
> err = populate_event_from_wobj(dev, &event, wobj);
> if (err) {
> dev_warn(dev, "Bad event data (ACPI type %d)\n", wobj->type);
> - goto out_free_wobj;
> + goto out_free;
> }
>
> event_type = classify_event(event.name, event.category);
> @@ -1666,13 +1658,10 @@ static void hp_wmi_notify(u32 value, void *context)
> break;
> }
>
> -out_free_wobj:
> - kfree(wobj);
> -
> +out_free:
> devm_kfree(dev, event.name);
> devm_kfree(dev, event.description);
Totally unrelated to your patch, using devm_*() for the members of an
on-stack struct looks very very odd. :-/
Your change looks good and removes lots of code duplication. :-)
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
--
i.
>
> -out_unlock:
> mutex_unlock(&state->lock);
> }
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 349169d050c5..7169b84ccdb6 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -2223,39 +2223,25 @@ static void acer_rfkill_exit(void)
> }
> }
>
> -static void acer_wmi_notify(u32 value, void *context)
> +static void acer_wmi_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> struct event_return_value return_value;
> - acpi_status status;
> u16 device_state;
> const struct key_entry *key;
> u32 scancode;
>
> - status = wmi_get_event_data(value, &response);
> - if (status != AE_OK) {
> - pr_warn("bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> -
> if (!obj)
> return;
> if (obj->type != ACPI_TYPE_BUFFER) {
> pr_warn("Unknown response received %d\n", obj->type);
> - kfree(obj);
> return;
> }
> if (obj->buffer.length != 8) {
> pr_warn("Unknown buffer length %d\n", obj->buffer.length);
> - kfree(obj);
> return;
> }
>
> return_value = *((struct event_return_value *)obj->buffer.pointer);
> - kfree(obj);
>
> switch (return_value.function) {
> case WMID_HOTKEY_EVENT:
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 9c6b3937ac71..1eb6b39df604 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -4187,28 +4187,15 @@ static void asus_wmi_fnlock_update(struct asus_wmi *asus)
>
> /* WMI events *****************************************************************/
>
> -static int asus_wmi_get_event_code(u32 value)
> +static int asus_wmi_get_event_code(union acpi_object *obj)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - acpi_status status;
> int code;
>
> - status = wmi_get_event_data(value, &response);
> - if (ACPI_FAILURE(status)) {
> - pr_warn("Failed to get WMI notify code: %s\n",
> - acpi_format_exception(status));
> - return -EIO;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> -
> if (obj && obj->type == ACPI_TYPE_INTEGER)
> code = (int)(obj->integer.value & WMI_EVENT_MASK);
> else
> code = -EIO;
>
> - kfree(obj);
> return code;
> }
>
> @@ -4274,10 +4261,10 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> pr_info("Unknown key code 0x%x\n", code);
> }
>
> -static void asus_wmi_notify(u32 value, void *context)
> +static void asus_wmi_notify(union acpi_object *obj, void *context)
> {
> struct asus_wmi *asus = context;
> - int code = asus_wmi_get_event_code(value);
> + int code = asus_wmi_get_event_code(obj);
>
> if (code < 0) {
> pr_warn("Failed to get notify code: %d\n", code);
> diff --git a/drivers/platform/x86/dell/dell-wmi-aio.c b/drivers/platform/x86/dell/dell-wmi-aio.c
> index c7b7f1e403fb..54096495719b 100644
> --- a/drivers/platform/x86/dell/dell-wmi-aio.c
> +++ b/drivers/platform/x86/dell/dell-wmi-aio.c
> @@ -70,20 +70,10 @@ static bool dell_wmi_aio_event_check(u8 *buffer, int length)
> return false;
> }
>
> -static void dell_wmi_aio_notify(u32 value, void *context)
> +static void dell_wmi_aio_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> struct dell_wmi_event *event;
> - acpi_status status;
>
> - status = wmi_get_event_data(value, &response);
> - if (status != AE_OK) {
> - pr_info("bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> if (obj) {
> unsigned int scancode = 0;
>
> @@ -114,7 +104,6 @@ static void dell_wmi_aio_notify(u32 value, void *context)
> break;
> }
> }
> - kfree(obj);
> }
>
> static int __init dell_wmi_aio_input_setup(void)
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 876e0a97cee1..8c05e0dd2a21 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -834,28 +834,16 @@ static struct attribute *hp_wmi_attrs[] = {
> };
> ATTRIBUTE_GROUPS(hp_wmi);
>
> -static void hp_wmi_notify(u32 value, void *context)
> +static void hp_wmi_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> u32 event_id, event_data;
> - union acpi_object *obj;
> - acpi_status status;
> u32 *location;
> int key_code;
>
> - status = wmi_get_event_data(value, &response);
> - if (status != AE_OK) {
> - pr_info("bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> -
> if (!obj)
> return;
> if (obj->type != ACPI_TYPE_BUFFER) {
> pr_info("Unknown response received %d\n", obj->type);
> - kfree(obj);
> return;
> }
>
> @@ -872,10 +860,8 @@ static void hp_wmi_notify(u32 value, void *context)
> event_data = *(location + 2);
> } else {
> pr_info("Unknown buffer length %d\n", obj->buffer.length);
> - kfree(obj);
> return;
> }
> - kfree(obj);
>
> switch (event_id) {
> case HPWMI_DOCK_EVENT:
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index 09d476dd832e..d81fd5df4a00 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -734,26 +734,14 @@ static void huawei_wmi_process_key(struct input_dev *idev, int code)
> sparse_keymap_report_entry(idev, key, 1, true);
> }
>
> -static void huawei_wmi_input_notify(u32 value, void *context)
> +static void huawei_wmi_input_notify(union acpi_object *obj, void *context)
> {
> struct input_dev *idev = (struct input_dev *)context;
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - acpi_status status;
>
> - status = wmi_get_event_data(value, &response);
> - if (ACPI_FAILURE(status)) {
> - dev_err(&idev->dev, "Unable to get event data\n");
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> if (obj && obj->type == ACPI_TYPE_INTEGER)
> huawei_wmi_process_key(idev, obj->integer.value);
> else
> dev_err(&idev->dev, "Bad response type\n");
> -
> - kfree(response.pointer);
> }
>
> static int huawei_wmi_input_setup(struct device *dev, const char *guid)
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 9c7857842caf..4d57cf803473 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -182,21 +182,11 @@ static union acpi_object *lg_wmbb(struct device *dev, u32 method_id, u32 arg1, u
> return (union acpi_object *)buffer.pointer;
> }
>
> -static void wmi_notify(u32 value, void *context)
> +static void wmi_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - acpi_status status;
> long data = (long)context;
>
> pr_debug("event guid %li\n", data);
> - status = wmi_get_event_data(value, &response);
> - if (ACPI_FAILURE(status)) {
> - pr_err("Bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> if (!obj)
> return;
>
> @@ -218,7 +208,6 @@ static void wmi_notify(u32 value, void *context)
>
> pr_debug("Type: %i Eventcode: 0x%llx\n", obj->type,
> obj->integer.value);
> - kfree(response.pointer);
> }
>
> static void wmi_input_setup(void)
> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index fd318cdfe313..4a7ac85c4db4 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -170,20 +170,9 @@ static const struct backlight_ops msi_backlight_ops = {
> .update_status = bl_set_status,
> };
>
> -static void msi_wmi_notify(u32 value, void *context)
> +static void msi_wmi_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> struct key_entry *key;
> - union acpi_object *obj;
> - acpi_status status;
> -
> - status = wmi_get_event_data(value, &response);
> - if (status != AE_OK) {
> - pr_info("bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
>
> if (obj && obj->type == ACPI_TYPE_INTEGER) {
> int eventcode = obj->integer.value;
> @@ -192,7 +181,7 @@ static void msi_wmi_notify(u32 value, void *context)
> eventcode);
> if (!key) {
> pr_info("Unknown key pressed - %x\n", eventcode);
> - goto msi_wmi_notify_exit;
> + return;
> }
>
> if (event_wmi->quirk_last_pressed) {
> @@ -204,7 +193,7 @@ static void msi_wmi_notify(u32 value, void *context)
> pr_debug("Suppressed key event 0x%X - "
> "Last press was %lld us ago\n",
> key->code, ktime_to_us(diff));
> - goto msi_wmi_notify_exit;
> + return;
> }
> last_pressed = cur;
> }
> @@ -221,9 +210,6 @@ static void msi_wmi_notify(u32 value, void *context)
> }
> } else
> pr_info("Unknown event received\n");
> -
> -msi_wmi_notify_exit:
> - kfree(response.pointer);
> }
>
> static int __init msi_wmi_backlight_setup(void)
> diff --git a/drivers/platform/x86/toshiba-wmi.c b/drivers/platform/x86/toshiba-wmi.c
> index 77c35529ab6f..12c46455e8dc 100644
> --- a/drivers/platform/x86/toshiba-wmi.c
> +++ b/drivers/platform/x86/toshiba-wmi.c
> @@ -32,26 +32,13 @@ static const struct key_entry toshiba_wmi_keymap[] __initconst = {
> { KE_END, 0 }
> };
>
> -static void toshiba_wmi_notify(u32 value, void *context)
> +static void toshiba_wmi_notify(union acpi_object *obj, void *context)
> {
> - struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - acpi_status status;
> -
> - status = wmi_get_event_data(value, &response);
> - if (ACPI_FAILURE(status)) {
> - pr_err("Bad event status 0x%x\n", status);
> - return;
> - }
> -
> - obj = (union acpi_object *)response.pointer;
> if (!obj)
> return;
>
> /* TODO: Add proper checks once we have data */
> pr_debug("Unknown event received, obj type %x\n", obj->type);
> -
> - kfree(response.pointer);
> }
>
> static const struct dmi_system_id toshiba_wmi_dmi_table[] __initconst = {
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 1d0b2d6040d1..6ab181dd94ab 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1227,40 +1227,33 @@ static int wmi_notify_device(struct device *dev, void *data)
> if (!(wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *event))
> return 0;
>
> + /* The ACPI WMI specification says that _WED should be
> + * evaluated every time an notification is received, even
> + * if no consumers are present.
> + *
> + * Some firmware implementations actually depend on this
> + * by using a queue for events which will fill up if the
> + * WMI driver core stops evaluating _WED due to missing
> + * WMI event consumers.
> + */
> + ret = wmi_get_notify_data(wblock, &obj);
> + if (ret < 0)
> + return -EIO;
> +
> down_read(&wblock->notify_lock);
> /* The WMI driver notify handler conflicts with the legacy WMI handler.
> * Because of this the WMI driver notify handler takes precedence.
> */
> if (wblock->dev.dev.driver && wblock->driver_ready) {
> - ret = wmi_get_notify_data(wblock, &obj);
> - if (ret >= 0) {
> - wmi_notify_driver(wblock, obj);
> - kfree(obj);
> - }
> + wmi_notify_driver(wblock, obj);
> } else {
> - if (wblock->handler) {
> - wblock->handler(*event, wblock->handler_data);
> - } else {
> - /* The ACPI WMI specification says that _WED should be
> - * evaluated every time an notification is received, even
> - * if no consumers are present.
> - *
> - * Some firmware implementations actually depend on this
> - * by using a queue for events which will fill up if the
> - * WMI driver core stops evaluating _WED due to missing
> - * WMI event consumers.
> - *
> - * Because of this we need this seemingly useless call to
> - * wmi_get_notify_data() which in turn evaluates _WED.
> - */
> - ret = wmi_get_notify_data(wblock, &obj);
> - if (ret >= 0)
> - kfree(obj);
> - }
> -
> + if (wblock->handler)
> + wblock->handler(obj, wblock->handler_data);
> }
> up_read(&wblock->notify_lock);
>
> + kfree(obj);
> +
> acpi_bus_generate_netlink_event("wmi", acpi_dev_name(wblock->acpi_device), *event, 0);
>
> return -EBUSY;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 0687a442fec7..eed105b1fbfb 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -386,7 +386,7 @@ extern bool acpi_is_pnp_device(struct acpi_device *);
>
> #if defined(CONFIG_ACPI_WMI) || defined(CONFIG_ACPI_WMI_MODULE)
>
> -typedef void (*wmi_notify_handler) (u32 value, void *context);
> +typedef void (*wmi_notify_handler) (union acpi_object *data, void *context);
>
> int wmi_instance_count(const char *guid);
>
> --
> 2.39.2
>
