linux-kernel - Re: [PATCH v6 1/9] platform/x86: asus-wmi: export symbols used for read/write WMI

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <39044aeb-f00f-c9f2-4249-437906d56631@linux.intel.com>
Date: Wed, 16 Oct 2024 16:50:37 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Luke D. Jones" <luke@...nes.dev>
cc: LKML <linux-kernel@...r.kernel.org>, linux-input@...r.kernel.org, 
    jikos@...nel.org, platform-driver-x86@...r.kernel.org, 
    Hans de Goede <hdegoede@...hat.com>, corentin.chary@...il.com, 
    superm1@...nel.org, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v6 1/9] platform/x86: asus-wmi: export symbols used for
 read/write WMI

On Mon, 30 Sep 2024, Luke D. Jones wrote:

> Export some rather helpful read/write WMI symbols using a namespace.
> These are DEVS and DSTS only, or require the arg0 input.
> 
> Also does a slight refactor of internals of these functions.

I'm a bit lost where this refers to. I see you're adding another function 
but nothing is being refactored AFAICT.

> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> Reviewed-by: Mario Limonciello <mario.limonciello@....com>
> ---
>  drivers/platform/x86/asus-wmi.c            | 51 ++++++++++++++++++++--
>  include/linux/platform_data/x86/asus-wmi.h | 10 +++++
>  2 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 6725a27df62f..0a5221d65130 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -385,7 +385,7 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
>  {
>  	return asus_wmi_evaluate_method3(method_id, arg0, arg1, 0, retval);
>  }
> -EXPORT_SYMBOL_GPL(asus_wmi_evaluate_method);
> +EXPORT_SYMBOL_NS_GPL(asus_wmi_evaluate_method, ASUS_WMI);
>  
>  static int asus_wmi_evaluate_method5(u32 method_id,
>  		u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4, u32 *retval)
> @@ -549,12 +549,57 @@ static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
>  	return 0;
>  }
>  
> -static int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param,
> -				 u32 *retval)
> +/**
> + * asus_wmi_get_devstate_dsts() - Get the WMI function state.
> + * @dev_id: The WMI function to call.
> + * @retval: A pointer to where to store the value returned from WMI.
> + *
> + * The returned WMI function state can also be used to determine if the WMI

"also" ?? You're lacking some context here what else this is useful for,  
you only talk about "also" part.

> + * function is supported by checking if the asus_wmi_get_devstate_dsts()
> + * returns an error.
> + *
> + * On success the return value is 0, and the retval is a valid value returned
> + * by the successful WMI function call. An error value is returned only if the
> + * WMI function failed, or if it returns "unsupported" which is typically a 0
> + * (no return, and no 'supported' bit set), or a 0xFFFFFFFE (~1) which if not
> + * caught here can result in unexpected behaviour later.
> + */
> +int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval)
> +{
> +	int err;
> +
> +	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, dev_id, 0, retval);
> +	if (err)
> +		return err;
> +
> +	*retval &= ~ASUS_WMI_DSTS_PRESENCE_BIT;
> +	if (*retval == ASUS_WMI_UNSUPPORTED_METHOD)

This seems buggy. First ASUS_WMI_DSTS_PRESENCE_BIT bit is unmasked from 
*retval and then you compare it to ASUS_WMI_UNSUPPORTED_METHOD which can 
never be true.

> +		return -ENODEV;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(asus_wmi_get_devstate_dsts, ASUS_WMI);
> +
> +/**
> + * asus_wmi_set_devstate() - Set the WMI function state.
> + * @dev_id: The WMI function to call.
> + * @ctrl_param: The argument to be used for this WMI function.
> + * @retval: A pointer to where to store the value returned from WMI.
> + *
> + * The returned WMI function state if not checked here for error as
> + * asus_wmi_set_devstate() is not called unless first paired with a call to
> + * asus_wmi_get_devstate_dsts() to check that the WMI function is supported.

Please try to rephrase this mess. :-)

-- 
 i.

> + * On success the return value is 0, and the retval is a valid value returned
> + * by the successful WMI function call. An error value is returned only if the
> + * WMI function failed.
> + */
> +int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
>  {
>  	return asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS, dev_id,
>  					ctrl_param, retval);
>  }
> +EXPORT_SYMBOL_NS_GPL(asus_wmi_set_devstate, ASUS_WMI);
>  
>  /* Helper for special devices with magic return codes */
>  static int asus_wmi_get_devstate_bits(struct asus_wmi *asus,
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 365e119bebaa..6ea4dedfb85e 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -158,8 +158,18 @@
>  #define ASUS_WMI_DSTS_LIGHTBAR_MASK	0x0000000F
>  
>  #if IS_REACHABLE(CONFIG_ASUS_WMI)
> +int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval);
> +int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
>  int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
>  #else
> +static inline int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval)
> +{
> +	return -ENODEV;
> +}
> +static inline int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
> +{
> +	return -ENODEV;
> +}
>  static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
>  					   u32 *retval)
>  {
>