| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4512f55b-8dce-40b0-a207-7c3463ca4e01@gmx.de>
Date: Mon, 20 Nov 2023 22:29:50 +0100
From: Armin Wolf <W_Armin@....de>
To: Hans de Goede <hdegoede@...hat.com>, jithu.joseph@...el.com,
markgross@...nel.org, ilpo.jarvinen@...ux.intel.com
Cc: Dell.Client.Kernel@...l.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] platform/x86: wmi: Add wmidev_block_set()
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@....de>
> 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
>>
Powered by blists - more mailing lists