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 22:42 schrieb Philipp Jungkamp:

> On Sun, 2022-11-13 at 21:30 +0100, Armin Wolf wrote:
>> 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
>>
> Hello,
>
> I thought the same, so I implemented the bare minimum to support the
> GUID on my hardware using the WMI bus in the v1 patch.
>
> But I also agree with Hans in that it should probably reside in the
> ideapad-laptop driver, so there aren't two modules for the same
> hardware.
>
> Therefore I'd need to change the WMI handling part of ideapad-laptop in
> a way that does not break other hardware.
>
> I don't know how to write a driver which connects on two buses.
> The driver is currently for a platform device with the corresponding
> probe and remove hooks. How would I coordinate the private data
> allocation/deletion between the  platform-probe/-remove and wmi-probe/-
> remove in a way that is established by other drivers.
>
> Do you have a recommendation on how to setup a module to register on
> two buses or another driver that is good to learn from?
>
> Greetings,
> Philipp Jungkamp

Well, the current driver does the WMI registration when probing the platform device,
so maybe you could just call wmi_driver_register() there?
Since the WMI notify handler may need to access the platform device, i suggest to
store the wmi_driver struct inside the ideapad_private struct, and then use container_of()
to get a reference from wmi_device->dev->driver to ideapad_private.
This should be safe as long as the WMI driver is unregistered on platform device removal.

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