Re: [PATCH 1/5] platform/x86: wmi: Pass event data directly to legacy notify handlers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 

[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux