Re: [PATCH v2 1/2] platform/x86: wmi: Disambiguate WMI event data on some ACPI tables

[Armin Wolf <W_Armin@gmx.de>]

Am 13.11.22 um 13:12 schrieb Philipp Jungkamp:

> The ACPI DSDT table includes multiple WMI blocks which emit events with
> the same notify_id. The wmi_get_event_data() function chooses the
> wmi_block with the _WED handler to call based on the notify_id. This
> function may call the wrong _WED event handler based on the order the
> WMI blocks are parsed.
>
> This introduces wmi_get_event_data_with_guid() to diambiguate the _WED
> call to get metadata for an event. The GUID here is the one of the
> containing WMI block, not the one of the WMI event itself.

Hello,

maybe it would be better to instead rewrite the driver to utilize the WMI bus infrastructure?
Because a GUID is not guaranteed to be unique inside a system, there would still be a chance
to call the wrong _WED handler.
AFAIK only utilizing the WMI bus infrastructure would fully disambiguate the WMI event data.

Armin Wolf

> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Was separating this change into it's own commit correct?
>
>   drivers/platform/x86/wmi.c | 30 ++++++++++++++++++++++++++++++
>   include/linux/acpi.h       |  3 +++
>   2 files changed, 33 insertions(+)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 223550a10d4d..56b666f4b40b 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -659,6 +659,36 @@ acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out)
>   }
>   EXPORT_SYMBOL_GPL(wmi_get_event_data);
>
> +/**
> + * wmi_get_event_data_with_guid - Get WMI data associated with an event by guid
> + *
> + * Consider using this instead of wmi_get_event_data() when the notify_id
> + * of the WMI event may not be unique among all WMI blocks of a device.
> + *
> + * @guid: GUID of the WMI block for this event
> + * @event: Event to find
> + * @out: Buffer to hold event data. out->pointer should be freed with kfree()
> + *
> + * Returns extra data associated with an event in WMI.
> + */
> +acpi_status wmi_get_event_data_with_guid(const char *guid, u32 event, struct acpi_buffer *out)
> +{
> +	struct wmi_block *wblock = NULL;
> +	struct guid_block *gblock;
> +	acpi_status status;
> +
> +	status = find_guid(guid, &wblock);
> +	if (ACPI_FAILURE(status))
> +		return AE_NOT_FOUND;
> +
> +	gblock = &wblock->gblock;
> +	if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id == event)
> +		return get_event_data(wblock, out);
> +
> +	return AE_NOT_FOUND;
> +}
> +EXPORT_SYMBOL_GPL(wmi_get_event_data_with_guid);
> +
>   /**
>    * wmi_has_guid - Check if a GUID is available
>    * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 3015235d65e3..51ac4d6bcae1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -423,6 +423,9 @@ extern acpi_status wmi_set_block(const char *guid, u8 instance,
>   extern acpi_status wmi_install_notify_handler(const char *guid,
>   					wmi_notify_handler handler, void *data);
>   extern acpi_status wmi_remove_notify_handler(const char *guid);
> +extern acpi_status wmi_get_event_data_with_guid(const char *guid,
> +						u32 event,
> +						struct acpi_buffer *out);
>   extern acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out);
>   extern bool wmi_has_guid(const char *guid);
>   extern char *wmi_get_acpi_device_uid(const char *guid);
> --
> 2.38.1
>