Re: [PATCH v2 1/4] platform/x86: wmi: Add wmidev_block_set()

From: Armin Wolf
Date: Mon Nov 20 2023 - 16:30:15 EST

Am 20.11.23 um 13:00 schrieb Hans de Goede:

Hi Armin,

On 11/3/23 19:25, Armin Wolf wrote:
Currently, WMI drivers have to use the deprecated GUID-based
interface when setting data blocks. This prevents those
drivers from fully moving away from this interface.

Provide wmidev_block_set() so drivers using wmi_set_block() can
fully migrate to the modern bus-based interface.

Tested with a custom SSDT from the Intel Slim Bootloader project.

Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

One thing which I noticed during review of patch 3/4 is that
some WMI drivers might benefit from having a wmidev_block_query_typed()
similar to how we have a acpi_evaluate_dsm_typed() which takes an
ACPI type and returns a NULL pointer instead of a wrongly typed
ACPI object when the type does not match. Specifically this
would allow dropping the return obj type checking from
sbl-fw-update.c : get_fwu_request() .

Now adding a wmidev_block_query_typed() wrapper around
wmidev_block_query () just for this is not really a win,
but it might be useful in the future ? Anyways just an idea.

Regards,

Hans

Good idea, i am already working one something similar:

The ACPI-WMI mapper on Windows translates the ACPI object into an standard WMI buffer, see
https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items for details.

This "normalization" of the buffer content is necessary since for example WMI strings can
be expressed either thru an ACPI string or thru an utf16 ACPI buffer. Such a problem already
affected the HP WMI sensor driver, and i am planning to solve this with an new API.
This API would decouple WMI drivers from the ACPI subsystem.

However i am currently busy cleaning up the WMI subsystem (suspend fixes come next), so
it might take some time. But i have it on my (increasingly long) list.

Thanks,
Armin Wolf




---
Changes in v2:
- applies on pdx86/for-next
---
drivers/platform/x86/wmi.c | 64 ++++++++++++++++++++------------------
include/linux/wmi.h | 2 ++
2 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 5c27b4aa9690..9d9a050e7086 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -536,41 +536,50 @@ EXPORT_SYMBOL_GPL(wmidev_block_query);
*
* Return: acpi_status signaling success or error.
*/
-acpi_status wmi_set_block(const char *guid_string, u8 instance,
- const struct acpi_buffer *in)
+acpi_status wmi_set_block(const char *guid_string, u8 instance, const struct acpi_buffer *in)
{
- struct wmi_block *wblock;
- struct guid_block *block;
struct wmi_device *wdev;
- acpi_handle handle;
- struct acpi_object_list input;
- union acpi_object params[2];
- char method[WMI_ACPI_METHOD_NAME_SIZE];
acpi_status status;

- if (!in)
- return AE_BAD_DATA;
-
wdev = wmi_find_device_by_guid(guid_string);
if (IS_ERR(wdev))
return AE_ERROR;

- wblock = container_of(wdev, struct wmi_block, dev);
- block = &wblock->gblock;
- handle = wblock->acpi_device->handle;
+ status = wmidev_block_set(wdev, instance, in);
+ wmi_device_put(wdev);

- if (block->instance_count <= instance) {
- status = AE_BAD_PARAMETER;
+ return status;
+}
+EXPORT_SYMBOL_GPL(wmi_set_block);

- goto err_wdev_put;
- }
+/**
+ * wmidev_block_set - Write to a WMI block
+ * @wdev: A wmi bus device from a driver
+ * @instance: Instance index
+ * @in: Buffer containing new values for the data block
+ *
+ * Write contents of the input buffer to an ACPI-WMI data block.
+ *
+ * Return: acpi_status signaling success or error.
+ */
+acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct acpi_buffer *in)
+{
+ struct wmi_block *wblock = container_of(wdev, struct wmi_block, dev);
+ acpi_handle handle = wblock->acpi_device->handle;
+ struct guid_block *block = &wblock->gblock;
+ char method[WMI_ACPI_METHOD_NAME_SIZE];
+ struct acpi_object_list input;
+ union acpi_object params[2];

- /* Check GUID is a data block */
- if (block->flags & (ACPI_WMI_EVENT | ACPI_WMI_METHOD)) {
- status = AE_ERROR;
+ if (!in)
+ return AE_BAD_DATA;

- goto err_wdev_put;
- }
+ if (block->instance_count <= instance)
+ return AE_BAD_PARAMETER;
+
+ /* Check GUID is a data block */
+ if (block->flags & (ACPI_WMI_EVENT | ACPI_WMI_METHOD))
+ return AE_ERROR;

input.count = 2;
input.pointer = params;
@@ -582,14 +591,9 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,

get_acpi_method_name(wblock, 'S', method);

- status = acpi_evaluate_object(handle, method, &input, NULL);
-
-err_wdev_put:
- wmi_device_put(wdev);
-
- return status;
+ return acpi_evaluate_object(handle, method, &input, NULL);
}
-EXPORT_SYMBOL_GPL(wmi_set_block);
+EXPORT_SYMBOL_GPL(wmidev_block_set);

static void wmi_dump_wdg(const struct guid_block *g)
{
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 763bd382cf2d..207544968268 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -35,6 +35,8 @@ extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
u8 instance);

+acpi_status wmidev_block_set(struct wmi_device *wdev, u8 instance, const struct acpi_buffer *in);
+
u8 wmidev_instance_count(struct wmi_device *wdev);

extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
--
2.39.2