| 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: <7c8dd434-11d0-417f-8d49-a2f506bb00fa@app.fastmail.com>
Date: Wed, 16 Oct 2024 16:48:18 +0200
From: "Luke Jones" <luke@...nes.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: LKML <linux-kernel@...r.kernel.org>, linux-input@...r.kernel.org,
"Jiri Kosina" <jikos@...nel.org>, platform-driver-x86@...r.kernel.org,
"Hans de Goede" <hdegoede@...hat.com>, corentin.chary@...il.com,
"Mario Limonciello" <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 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@...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. :-)
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)
>> {
>>
Powered by blists - more mailing lists