Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <4eb8e035-26f1-44e6-ae32-61d843c61f13@redhat.com>
Date:   Mon, 20 Nov 2023 13:00:59 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/4] platform/x86: wmi: Add wmidev_block_set()
Content-Language: en-US, nl
To:     Armin Wolf <W_Armin@gmx.de>, jithu.joseph@intel.com,
        markgross@kernel.org, ilpo.jarvinen@linux.intel.com
Cc:     Dell.Client.Kernel@dell.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231103182526.3524-1-W_Armin@gmx.de>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20231103182526.3524-1-W_Armin@gmx.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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


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