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
> > Signed-off-by: Philipp Jungkamp <p.jungkamp@xxxxxxx>
> > ---
> > 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
> >
