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