This patch series contains fixes for the handling of WMI event data
when receiving WMI events.
The first patch aims to prevent WMI event drivers depending on WMI
event data support from binding to a WMI device which does not
support the retrieval of additional WMI event data.
The second patch makes sure that the WMI core not only checks that
evaluating the ACPI control method used for retrieving additional
event data (_WED) succeeded, but that it also returned any data.
The third patch fixes an compatibility issue with the ACPI firmware
of some ASUS notebooks. This issue was "fixed" inside asus-wmi by
manually retrieving event data items, sidestepping the WMI core.
The last patch reverts this hacky fixup, as the underlying issue is
now handled inside the WMI core itself.
All patches where tested on a Dell Inspiron 3505 and a ASUS Prime
B650-Plus motherboard. However the last patch should be tested on an
actual ASUS notebook which is affected by the workaround.
Changes since v1:
- Drivers are not "he"
Armin Wolf (5):
platform/x86: wmi: Prevent incompatible event driver from probing
platform/x86: wmi: Check if event data is not NULL
platform/x86: wmi: Always evaluate _WED when receiving an event
platform/x86: wmi: Update documentation regarding _WED
Revert "platform/x86: asus-wmi: Support WMI event queue"
Documentation/wmi/acpi-interface.rst | 5 +-
drivers/platform/x86/asus-wmi.c | 71 ++------------------------
drivers/platform/x86/wmi.c | 74 +++++++++++++++++++++++-----
include/linux/wmi.h | 2 +-
4 files changed, 71 insertions(+), 81 deletions(-)
--
2.39.2
This reverts commit 1a373d15e283937b51eaf5debf4fc31474c31436.
The WMI core now takes care of draining the event queue if asus-wmi
is not loaded, so the hacky event queue handling code is not needed
anymore.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 71 +++------------------------------
1 file changed, 5 insertions(+), 66 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 21dee425ea6f..2865af89e95c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -101,13 +101,6 @@ module_param(fnlock_default, bool, 0444);
#define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI 0x9c31
#define ASUS_ACPI_UID_ASUSWMI "ASUSWMI"
-#define ASUS_ACPI_UID_ATK "ATK"
-
-#define WMI_EVENT_QUEUE_SIZE 0x10
-#define WMI_EVENT_QUEUE_END 0x1
-#define WMI_EVENT_MASK 0xFFFF
-/* The WMI hotkey event value is always the same. */
-#define WMI_EVENT_VALUE_ATK 0xFF
#define WMI_EVENT_MASK 0xFFFF
@@ -219,7 +212,6 @@ struct asus_wmi {
int dsts_id;
int spec;
int sfun;
- bool wmi_event_queue;
struct input_dev *inputdev;
struct backlight_device *backlight_device;
@@ -4019,50 +4011,14 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
static void asus_wmi_notify(u32 value, void *context)
{
struct asus_wmi *asus = context;
- int code;
- int i;
-
- for (i = 0; i < WMI_EVENT_QUEUE_SIZE + 1; i++) {
- code = asus_wmi_get_event_code(value);
- if (code < 0) {
- pr_warn("Failed to get notify code: %d\n", code);
- return;
- }
-
- if (code == WMI_EVENT_QUEUE_END || code == WMI_EVENT_MASK)
- return;
-
- asus_wmi_handle_event_code(code, asus);
-
- /*
- * Double check that queue is present:
- * ATK (with queue) uses 0xff, ASUSWMI (without) 0xd2.
- */
- if (!asus->wmi_event_queue || value != WMI_EVENT_VALUE_ATK)
- return;
- }
+ int code = asus_wmi_get_event_code(value);
- pr_warn("Failed to process event queue, last code: 0x%x\n", code);
-}
-
-static int asus_wmi_notify_queue_flush(struct asus_wmi *asus)
-{
- int code;
- int i;
-
- for (i = 0; i < WMI_EVENT_QUEUE_SIZE + 1; i++) {
- code = asus_wmi_get_event_code(WMI_EVENT_VALUE_ATK);
- if (code < 0) {
- pr_warn("Failed to get event during flush: %d\n", code);
- return code;
- }
-
- if (code == WMI_EVENT_QUEUE_END || code == WMI_EVENT_MASK)
- return 0;
+ if (code < 0) {
+ pr_warn("Failed to get notify code: %d\n", code);
+ return;
}
- pr_warn("Failed to flush event queue\n");
- return -EIO;
+ asus_wmi_handle_event_code(code, asus);
}
/* Sysfs **********************************************************************/
@@ -4302,23 +4258,6 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
asus->dsts_id = ASUS_WMI_METHODID_DSTS;
}
- /*
- * Some devices can have multiple event codes stored in a queue before
- * the module load if it was unloaded intermittently after calling
- * the INIT method (enables event handling). The WMI notify handler is
- * expected to retrieve all event codes until a retrieved code equals
- * queue end marker (One or Ones). Old codes are flushed from the queue
- * upon module load. Not enabling this when it should be has minimal
- * visible impact so fall back if anything goes wrong.
- */
- wmi_uid = wmi_get_acpi_device_uid(asus->driver->event_guid);
- if (wmi_uid && !strcmp(wmi_uid, ASUS_ACPI_UID_ATK)) {
- dev_info(dev, "Detected ATK, enable event queue\n");
-
- if (!asus_wmi_notify_queue_flush(asus))
- asus->wmi_event_queue = true;
- }
-
/* CWAP allow to define the behavior of the Fn+F2 key,
* this method doesn't seems to be present on Eee PCs */
if (asus->driver->quirks->wapf >= 0)
--
2.39.2
WMI event drivers which do not have no_notify_data set expect
that each WMI event contains valid data. Evaluating _WED however
might return no data, which can cause issues with such drivers.
Fix this by validating that evaluating _WED did return data.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/platform/x86/wmi.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 8fb90b726f50..d0fe8153f803 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1210,6 +1210,7 @@ static void wmi_notify_driver(struct wmi_block *wblock)
{
struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj = NULL;
acpi_status status;
if (!driver->no_notify_data) {
@@ -1218,12 +1219,18 @@ static void wmi_notify_driver(struct wmi_block *wblock)
dev_warn(&wblock->dev.dev, "Failed to get event data\n");
return;
}
+
+ obj = data.pointer;
+ if (!obj) {
+ dev_warn(&wblock->dev.dev, "Event contains not event data\n");
+ return;
+ }
}
if (driver->notify)
- driver->notify(&wblock->dev, data.pointer);
+ driver->notify(&wblock->dev, obj);
- kfree(data.pointer);
+ kfree(obj);
}
static int wmi_notify_device(struct device *dev, void *data)
--
2.39.2
Update the WMI ACPI interface documentation to include the fact
that _WED should be evaluated every time an ACPI notification
is received.
Signed-off-by: Armin Wolf <[email protected]>
---
Documentation/wmi/acpi-interface.rst | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/wmi/acpi-interface.rst b/Documentation/wmi/acpi-interface.rst
index d31af0ed9c08..06fb7fcf4413 100644
--- a/Documentation/wmi/acpi-interface.rst
+++ b/Documentation/wmi/acpi-interface.rst
@@ -93,4 +93,7 @@ _WED ACPI method
----------------
Used to retrieve additional WMI event data, its single parameter is a integer
-holding the notification ID of the event.
+holding the notification ID of the event. This method should be evaluated every
+time an ACPI notification is received, since some ACPI implementations use a
+queue to store WMI event data items. This queue will overflow after a couple
+of WMI events are received without retrieving the associated WMI event data.
--
2.39.2
On Mon, 19 Feb 2024 12:59:14 +0100, Armin Wolf wrote:
> This patch series contains fixes for the handling of WMI event data
> when receiving WMI events.
>
> The first patch aims to prevent WMI event drivers depending on WMI
> event data support from binding to a WMI device which does not
> support the retrieval of additional WMI event data.
>
> [...]
Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.
The list of commits applied:
[1/5] platform/x86: wmi: Prevent incompatible event driver from probing
commit: b448bcb4cf02b27946f9bd6c4ded17e2209bf271
[2/5] platform/x86: wmi: Check if event data is not NULL
commit: b5003c979eb4128c7ca81ed144491e85232979b6
[3/5] platform/x86: wmi: Always evaluate _WED when receiving an event
commit: 2b30ea09e77d701f0b0b02243bdcf8aa408e4214
[4/5] platform/x86: wmi: Update documentation regarding _WED
commit: c865db581c295085fbfc1b6802f87c38ad92e4ca
[5/5] Revert "platform/x86: asus-wmi: Support WMI event queue"
commit: b27bb403e136b12cafeff4467d495bbc8c9b7441
--
i.
On Tue, 20 Feb 2024, Armin Wolf wrote:
> Am 19.02.24 um 12:59 schrieb Armin Wolf:
>
> > WMI event drivers which do not have no_notify_data set expect
> > that each WMI event contains valid data. Evaluating _WED however
> > might return no data, which can cause issues with such drivers.
> >
> > Fix this by validating that evaluating _WED did return data.
> >
> > Signed-off-by: Armin Wolf <[email protected]>
> > ---
> > drivers/platform/x86/wmi.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > index 8fb90b726f50..d0fe8153f803 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -1210,6 +1210,7 @@ static void wmi_notify_driver(struct wmi_block
> > *wblock)
> > {
> > struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
> > struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
> > + union acpi_object *obj = NULL;
> > acpi_status status;
> >
> > if (!driver->no_notify_data) {
> > @@ -1218,12 +1219,18 @@ static void wmi_notify_driver(struct wmi_block
> > *wblock)
> > dev_warn(&wblock->dev.dev, "Failed to get event
> > data\n");
> > return;
> > }
> > +
> > + obj = data.pointer;
> > + if (!obj) {
> > + dev_warn(&wblock->dev.dev, "Event contains not event
> > data\n");
>
> I just noticed that this should have been "Event contains no event data\n".
> Should i send
> another patch?
Hi Armin,
As I was doing some history manipulation anyway as is, I tweaked it
directly in the history. While doing the conflict resolution because of
that small change I realized the wording got corrected in the latter patch
anyway so it was quite harmless but it's now correct in both commits in
review-ilpo branch.
--
i.
Am 20.02.24 um 10:19 schrieb Ilpo Järvinen:
> On Tue, 20 Feb 2024, Armin Wolf wrote:
>
>> Am 19.02.24 um 12:59 schrieb Armin Wolf:
>>
>>> WMI event drivers which do not have no_notify_data set expect
>>> that each WMI event contains valid data. Evaluating _WED however
>>> might return no data, which can cause issues with such drivers.
>>>
>>> Fix this by validating that evaluating _WED did return data.
>>>
>>> Signed-off-by: Armin Wolf <[email protected]>
>>> ---
>>> drivers/platform/x86/wmi.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>> index 8fb90b726f50..d0fe8153f803 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -1210,6 +1210,7 @@ static void wmi_notify_driver(struct wmi_block
>>> *wblock)
>>> {
>>> struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
>>> struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
>>> + union acpi_object *obj = NULL;
>>> acpi_status status;
>>>
>>> if (!driver->no_notify_data) {
>>> @@ -1218,12 +1219,18 @@ static void wmi_notify_driver(struct wmi_block
>>> *wblock)
>>> dev_warn(&wblock->dev.dev, "Failed to get event
>>> data\n");
>>> return;
>>> }
>>> +
>>> + obj = data.pointer;
>>> + if (!obj) {
>>> + dev_warn(&wblock->dev.dev, "Event contains not event
>>> data\n");
>> I just noticed that this should have been "Event contains no event data\n".
>> Should i send
>> another patch?
> Hi Armin,
>
> As I was doing some history manipulation anyway as is, I tweaked it
> directly in the history. While doing the conflict resolution because of
> that small change I realized the wording got corrected in the latter patch
> anyway so it was quite harmless but it's now correct in both commits in
> review-ilpo branch.
>
Thank you!