mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/5] platform/x86: wmi: Fixes for event data handling
@ 2024-02-14  7:04 Armin Wolf
  2024-02-14  7:04 ` [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing Armin Wolf
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

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.

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing
  2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
@ 2024-02-14  7:04 ` Armin Wolf
  2024-02-15 12:32   ` Ilpo Järvinen
  2024-02-14  7:04 ` [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL Armin Wolf
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

If a WMI event driver has no_notify_data set, then he indicates
support for WMI events which provide no notify data, otherwise
the notify() callback expects a valid ACPI object as notify data.

However if a WMI event driver which requires notify data is bound
to a WMI event device which cannot retrieve such data due to the
_WED ACPI method being absent, then the driver will be dysfunctional
since all WMI events will be dropped due to the missing notify data.

Fix this by not allowing such WMI event drivers to bind to WMI event
devices which do not support retrieving of notify data. Also reword
the description of no_notify_data a bit.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 10 ++++++++++
 include/linux/wmi.h        |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index b83c0f0ddd5c..34d8f55afaad 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -57,6 +57,7 @@ static_assert(__alignof__(struct guid_block) == 1);

 enum {	/* wmi_block flags */
 	WMI_READ_TAKES_NO_ARGS,
+	WMI_NO_EVENT_DATA,
 };

 struct wmi_block {
@@ -870,6 +871,11 @@ static int wmi_dev_probe(struct device *dev)
 	struct wmi_driver *wdriver = drv_to_wdrv(dev->driver);
 	int ret = 0;

+	if (wdriver->notify) {
+		if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags) && !wdriver->no_notify_data)
+			return -ENODEV;
+	}
+
 	if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");

@@ -1095,6 +1101,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
 	const struct guid_block *gblock;
+	bool event_data_available;
 	struct wmi_block *wblock;
 	union acpi_object *obj;
 	acpi_status status;
@@ -1114,6 +1121,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 		return -ENXIO;
 	}

+	event_data_available = acpi_has_method(device->handle, "_WED");
 	gblock = (const struct guid_block *)obj->buffer.pointer;
 	total = obj->buffer.length / sizeof(struct guid_block);

@@ -1132,6 +1140,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)

 		wblock->acpi_device = device;
 		wblock->gblock = gblock[i];
+		if (gblock[i].flags & ACPI_WMI_EVENT && !event_data_available)
+			set_bit(WMI_NO_EVENT_DATA, &wblock->flags);

 		retval = wmi_create_device(wmi_bus_dev, wblock, device);
 		if (retval) {
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 686291b87852..781958310bfb 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -48,7 +48,7 @@ u8 wmidev_instance_count(struct wmi_device *wdev);
  * struct wmi_driver - WMI driver structure
  * @driver: Driver model structure
  * @id_table: List of WMI GUIDs supported by this driver
- * @no_notify_data: WMI events provide no event data
+ * @no_notify_data: Driver supports WMI events which provide no event data
  * @probe: Callback for device binding
  * @remove: Callback for device unbinding
  * @notify: Callback for receiving WMI events
--
2.39.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL
  2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
  2024-02-14  7:04 ` [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing Armin Wolf
@ 2024-02-14  7:04 ` Armin Wolf
  2024-02-15 12:31   ` Ilpo Järvinen
  2024-02-14  7:04 ` [PATCH 3/5] platform/x86: wmi: Always evaluate _WED when receiving an event Armin Wolf
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

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 <W_Armin@gmx.de>
---
 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 34d8f55afaad..8a916887c546 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1211,6 +1211,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) {
@@ -1219,12 +1220,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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/5] platform/x86: wmi: Always evaluate _WED when receiving an event
  2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
  2024-02-14  7:04 ` [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing Armin Wolf
  2024-02-14  7:04 ` [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL Armin Wolf
@ 2024-02-14  7:04 ` Armin Wolf
  2024-02-14  7:04 ` [PATCH 4/5] platform/x86: wmi: Update documentation regarding _WED Armin Wolf
  2024-02-14  7:04 ` [PATCH 5/5] Revert "platform/x86: asus-wmi: Support WMI event queue" Armin Wolf
  4 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

The ACPI WMI specification states:

	"The _WED control method is evaluated by the mapper in
	 response to receiving a notification from a control
	 method."

This means that _WED should be evaluated unconditionally even
if no WMI event consumers are present.
Some firmware implementations actually depend on this behavior
by storing the event data inside a queue which will fill up if
the WMI core stops retrieving event data items due to no
consumers being present

Fix this by always evaluating _WED even if no WMI event consumers
are present.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/wmi.c | 67 ++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 8a916887c546..8622ce3fecfd 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1207,37 +1207,46 @@ acpi_wmi_ec_space_handler(u32 function, acpi_physical_address address,
 	}
 }

-static void wmi_notify_driver(struct wmi_block *wblock)
+static int wmi_get_notify_data(struct wmi_block *wblock, union acpi_object **obj)
 {
-	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) {
-		status = get_event_data(wblock, &data);
-		if (ACPI_FAILURE(status)) {
-			dev_warn(&wblock->dev.dev, "Failed to get event data\n");
-			return;
-		}
+	if (test_bit(WMI_NO_EVENT_DATA, &wblock->flags)) {
+		*obj = NULL;
+		return 0;
+	}

-		obj = data.pointer;
-		if (!obj) {
-			dev_warn(&wblock->dev.dev, "Event contains not event data\n");
-			return;
-		}
+	status = get_event_data(wblock, &data);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&wblock->dev.dev, "Failed to get event data\n");
+		return -EIO;
+	}
+
+	*obj = data.pointer;
+
+	return 0;
+}
+
+static void wmi_notify_driver(struct wmi_block *wblock, union acpi_object *obj)
+{
+	struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
+
+	if (!obj && !driver->no_notify_data) {
+		dev_warn(&wblock->dev.dev, "Event contains no event data\n");
+		return;
 	}

 	if (driver->notify)
 		driver->notify(&wblock->dev, obj);
-
-	kfree(obj);
 }

 static int wmi_notify_device(struct device *dev, void *data)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
+	union acpi_object *obj;
 	u32 *event = data;
+	int ret;

 	if (!(wblock->gblock.flags & ACPI_WMI_EVENT && wblock->gblock.notify_id == *event))
 		return 0;
@@ -1247,10 +1256,32 @@ static int wmi_notify_device(struct device *dev, void *data)
 	 * Because of this the WMI driver notify handler takes precedence.
 	 */
 	if (wblock->dev.dev.driver && wblock->driver_ready) {
-		wmi_notify_driver(wblock);
+		ret = wmi_get_notify_data(wblock, &obj);
+		if (ret >= 0) {
+			wmi_notify_driver(wblock, obj);
+			kfree(obj);
+		}
 	} else {
-		if (wblock->handler)
+		if (wblock->handler) {
 			wblock->handler(*event, wblock->handler_data);
+		} else {
+			/* The ACPI WMI specification says that _WED should be
+			 * evaluated every time an notification is received, even
+			 * if no consumers are present.
+			 *
+			 * Some firmware implementations actually depend on this
+			 * by using a queue for events which will fill up if the
+			 * WMI driver core stops evaluating _WED due to missing
+			 * WMI event consumers.
+			 *
+			 * Because of this we need this seemingly useless call to
+			 * wmi_get_notify_data() which in turn evaluates _WED.
+			 */
+			ret = wmi_get_notify_data(wblock, &obj);
+			if (ret >= 0)
+				kfree(obj);
+		}
+
 	}
 	up_read(&wblock->notify_lock);

--
2.39.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 4/5] platform/x86: wmi: Update documentation regarding _WED
  2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
                   ` (2 preceding siblings ...)
  2024-02-14  7:04 ` [PATCH 3/5] platform/x86: wmi: Always evaluate _WED when receiving an event Armin Wolf
@ 2024-02-14  7:04 ` Armin Wolf
  2024-02-14  7:04 ` [PATCH 5/5] Revert "platform/x86: asus-wmi: Support WMI event queue" Armin Wolf
  4 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

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 <W_Armin@gmx.de>
---
 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 5/5] Revert "platform/x86: asus-wmi: Support WMI event queue"
  2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
                   ` (3 preceding siblings ...)
  2024-02-14  7:04 ` [PATCH 4/5] platform/x86: wmi: Update documentation regarding _WED Armin Wolf
@ 2024-02-14  7:04 ` Armin Wolf
  4 siblings, 0 replies; 10+ messages in thread
From: Armin Wolf @ 2024-02-14  7:04 UTC (permalink / raw)
  To: corentin.chary, luke
  Cc: hdegoede, ilpo.jarvinen, platform-driver-x86, linux-kernel

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 <W_Armin@gmx.de>
---
 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL
  2024-02-14  7:04 ` [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL Armin Wolf
@ 2024-02-15 12:31   ` Ilpo Järvinen
  2024-02-15 20:27     ` Armin Wolf
  0 siblings, 1 reply; 10+ messages in thread
From: Ilpo Järvinen @ 2024-02-15 12:31 UTC (permalink / raw)
  To: Armin Wolf; +Cc: corentin.chary, luke, Hans de Goede, platform-driver-x86, LKML

On Wed, 14 Feb 2024, Armin Wolf wrote:

> 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 <W_Armin@gmx.de>
> ---
>  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 34d8f55afaad..8a916887c546 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1211,6 +1211,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) {
> @@ -1219,12 +1220,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);

Hi Armin,

While looking into this patch, I failed to connect the mention of 
no_notify_data in the commit message with the code change that does
nothing differently based no_notify_data being set or not, AFAICT.

It could be just that you need to explain things better in the commit 
message, I'm not sure.

-- 
 i.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing
  2024-02-14  7:04 ` [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing Armin Wolf
@ 2024-02-15 12:32   ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-02-15 12:32 UTC (permalink / raw)
  To: Armin Wolf; +Cc: corentin.chary, luke, Hans de Goede, platform-driver-x86, LKML

On Wed, 14 Feb 2024, Armin Wolf wrote:

> If a WMI event driver has no_notify_data set, then he indicates

Drivers are not "he". :-)

-- 
 i.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL
  2024-02-15 12:31   ` Ilpo Järvinen
@ 2024-02-15 20:27     ` Armin Wolf
  2024-02-19 11:57       ` Ilpo Järvinen
  0 siblings, 1 reply; 10+ messages in thread
From: Armin Wolf @ 2024-02-15 20:27 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: corentin.chary, luke, Hans de Goede, platform-driver-x86, LKML

Am 15.02.24 um 13:31 schrieb Ilpo Järvinen:

> On Wed, 14 Feb 2024, Armin Wolf wrote:
>
>> 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 <W_Armin@gmx.de>
>> ---
>>   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 34d8f55afaad..8a916887c546 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1211,6 +1211,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) {
>> @@ -1219,12 +1220,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);
> Hi Armin,
>
> While looking into this patch, I failed to connect the mention of
> no_notify_data in the commit message with the code change that does
> nothing differently based no_notify_data being set or not, AFAICT.
>
> It could be just that you need to explain things better in the commit
> message, I'm not sure.

Here the _WED ACPI control method is only evaluated if driver->no_notify_data is not set.
So the returned ACPI object should only be validated in this case, as we pass NULL otherwise.

Armin Wolf


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL
  2024-02-15 20:27     ` Armin Wolf
@ 2024-02-19 11:57       ` Ilpo Järvinen
  0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2024-02-19 11:57 UTC (permalink / raw)
  To: Armin Wolf
  Cc: Ilpo Järvinen, corentin.chary, luke, Hans de Goede,
	platform-driver-x86, LKML

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On Thu, 15 Feb 2024, Armin Wolf wrote:

> Am 15.02.24 um 13:31 schrieb Ilpo Järvinen:
> 
> > On Wed, 14 Feb 2024, Armin Wolf wrote:
> > 
> > > 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 <W_Armin@gmx.de>
> > > ---
> > >   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 34d8f55afaad..8a916887c546 100644
> > > --- a/drivers/platform/x86/wmi.c
> > > +++ b/drivers/platform/x86/wmi.c
> > > @@ -1211,6 +1211,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) {
> > > @@ -1219,12 +1220,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);
> > Hi Armin,
> > 
> > While looking into this patch, I failed to connect the mention of
> > no_notify_data in the commit message with the code change that does
> > nothing differently based no_notify_data being set or not, AFAICT.
> > 
> > It could be just that you need to explain things better in the commit
> > message, I'm not sure.
> 
> Here the _WED ACPI control method is only evaluated if driver->no_notify_data
> is not set.
> So the returned ACPI object should only be validated in this case, as we pass
> NULL otherwise.

Yes, I'm sorry, it seems fine. For some reason I was very confused while 
reviewing even if no_notify_data was mentioned right in the previous 
context (maybe Iused some older version of the code while trying to figure 
things out, I dunno).

-- 
 i.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-02-19 11:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-14  7:04 [PATCH 0/5] platform/x86: wmi: Fixes for event data handling Armin Wolf
2024-02-14  7:04 ` [PATCH 1/5] platform/x86: wmi: Prevent incompatible event driver from probing Armin Wolf
2024-02-15 12:32   ` Ilpo Järvinen
2024-02-14  7:04 ` [PATCH 2/5] platform/x86: wmi: Check if event data is not NULL Armin Wolf
2024-02-15 12:31   ` Ilpo Järvinen
2024-02-15 20:27     ` Armin Wolf
2024-02-19 11:57       ` Ilpo Järvinen
2024-02-14  7:04 ` [PATCH 3/5] platform/x86: wmi: Always evaluate _WED when receiving an event Armin Wolf
2024-02-14  7:04 ` [PATCH 4/5] platform/x86: wmi: Update documentation regarding _WED Armin Wolf
2024-02-14  7:04 ` [PATCH 5/5] Revert "platform/x86: asus-wmi: Support WMI event queue" Armin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome