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

["Luke Jones" <luke@ljones.dev>]

On Wed, 16 Oct 2024, at 3:50 PM, Ilpo Järvinen wrote:
> 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@ljones.dev>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.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. :-)

Bloody hell who wrote that??? :-|

Ack everything.

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