| 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: <D8S7MF97YEGI.21PR2NBB42QRS@gmail.com>
Date: Fri, 28 Mar 2025 18:16:41 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Armin Wolf" <W_Armin@....de>, "Hans de Goede" <hdegoede@...hat.com>,
<platform-driver-x86@...r.kernel.org>, <Dell.Client.Kernel@...l.com>,
"LKML" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 03/12] platform/x86: alienware-wmi-wmax: Improve
internal AWCC API
On Fri Mar 28, 2025 at 11:51 AM -03, Ilpo Järvinen wrote:
> On Thu, 13 Mar 2025, Kurt Borja wrote:
>
>> Inline all AWCC WMI helper methods and directly return the newly
>> introduced __awcc_wmi_command() helper to simplify implementation.
>>
>> Drop awcc_thermal_control() in favor of awcc_op_activate_profile().
>>
>> Add awcc_op_get_resource_id(), awcc_op_get_current_profile() and a new
>> failure code.
>>
>> Reviewed-by: Armin Wolf <W_Armin@....de>
>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> ---
>> drivers/platform/x86/dell/alienware-wmi-wmax.c | 150 +++++++++++++++----------
>> 1 file changed, 91 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index 80aefba5b22d6b4ac18aeb2ca356f8c911150abd..b9dbfdc8096c571722b3c7ac3e73989e235e2eb9 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -32,6 +32,7 @@
>> #define AWCC_THERMAL_MODE_GMODE 0xAB
>>
>> #define AWCC_FAILURE_CODE 0xFFFFFFFF
>> +#define AWCC_FAILURE_CODE_2 0xFFFFFFFE
>> #define AWCC_THERMAL_TABLE_MASK GENMASK(7, 4)
>> #define AWCC_THERMAL_MODE_MASK GENMASK(3, 0)
>> /* Some IDs have a BIT(8) flag that we ignore */
>> @@ -443,8 +444,7 @@ const struct attribute_group wmax_deepsleep_attribute_group = {
>> };
>>
>> /*
>> - * Thermal Profile control
>> - * - Provides thermal profile control through the Platform Profile API
>> + * AWCC Helpers
>> */
>> static bool is_awcc_thermal_profile_id(u8 code)
>> {
>> @@ -463,72 +463,107 @@ static bool is_awcc_thermal_profile_id(u8 code)
>> return false;
>> }
>>
>> -static int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
>> - u8 arg, u32 *out_data)
>> +static int __awcc_wmi_command(struct wmi_device *wdev, u32 method_id,
>> + struct wmax_u32_args *args, u32 *out)
>
> Why did you put __ into the name?
>
> Some people oppose __ prefix altogether, I don't entirely agree with their
> stance but I don't really understand what the underscores here signify.
>
> Normally I see __ being used in three main cases:
> - non-__ variant does some locking around the call too the __ func (though
> many funcs use _locked postfix these days).
> - func is "dangerous" and caller has to really know what he/she is
> doing / be careful for some reason.
> - non-__ variant exists and somebody cannot come up better name for the
> internally called function (not very good use case, IMHO)
Oh - I didn't know. Andy also mentioned this prefix is used for
non-atomic versions of some functions like __set_bit().
>
> I don't see any of those apply here, this looks just an ordinary wrapper
> function, thus the question.
I did it to indicate it's a "private" method, so it's not used directly.
But ofc that's not the convention here, so I'll drop it.
>
>> {
>> - struct wmax_u32_args in_args = {
>> + int ret;
>> +
>> + ret = alienware_wmi_command(wdev, method_id, args, sizeof(*args), out);
>> + if (ret)
>> + return ret;
>> +
>> + if (*out == AWCC_FAILURE_CODE || *out == AWCC_FAILURE_CODE_2)
>> + return -EBADRQC;
>> +
>> + return 0;
>> +}
>> +
>> +static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
>> + u8 arg, u32 *out)
>> +{
>> + struct wmax_u32_args args = {
>> .operation = operation,
>> .arg1 = arg,
>> .arg2 = 0,
>> .arg3 = 0,
>> };
>> - int ret;
>>
>> - ret = alienware_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION,
>> - &in_args, sizeof(in_args), out_data);
>> - if (ret < 0)
>> - return ret;
>> -
>> - if (*out_data == AWCC_FAILURE_CODE)
>> - return -EBADRQC;
>> -
>> - return 0;
>> + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>> }
>>
>> -static int awcc_thermal_control(struct wmi_device *wdev, u8 profile)
>> +static inline int awcc_game_shift_status(struct wmi_device *wdev, u8 operation,
>
> As a general rule, don't add inline to .c files, we leave figuring
> inlining out to the compiler.
Good to know, thanks!
>
>> + u32 *out)
>> {
>> - struct wmax_u32_args in_args = {
>> - .operation = AWCC_OP_ACTIVATE_PROFILE,
>> - .arg1 = profile,
>> - .arg2 = 0,
>> - .arg3 = 0,
>> - };
>> - u32 out_data;
>> - int ret;
>> -
>> - ret = alienware_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL,
>> - &in_args, sizeof(in_args), &out_data);
>> - if (ret)
>> - return ret;
>> -
>> - if (out_data == AWCC_FAILURE_CODE)
>> - return -EBADRQC;
>> -
>> - return 0;
>> -}
>> -
>> -static int awcc_game_shift_status(struct wmi_device *wdev, u8 operation,
>> - u32 *out_data)
>> -{
>> - struct wmax_u32_args in_args = {
>> + struct wmax_u32_args args = {
>> .operation = operation,
>> .arg1 = 0,
>> .arg2 = 0,
>> .arg3 = 0,
>> };
>> - int ret;
>>
>> - ret = alienware_wmi_command(wdev, AWCC_METHOD_GAME_SHIFT_STATUS,
>> - &in_args, sizeof(in_args), out_data);
>> - if (ret < 0)
>> - return ret;
>> -
>> - if (*out_data == AWCC_FAILURE_CODE)
>> - return -EOPNOTSUPP;
>> -
>> - return 0;
>> + return __awcc_wmi_command(wdev, AWCC_METHOD_GAME_SHIFT_STATUS, &args, out);
>> }
>>
>> +/**
>> + * awcc_op_get_resource_id - Get the resource ID at a given index
>> + * @wdev: AWCC WMI device
>> + * @index: Index
>> + * @out: Value returned by the WMI call
>> + *
>> + * Get the resource ID at a given index. Resource IDs are listed in the
>
> Use @index to refer to the argument.
Ack
>
>> + * following order:
>> + *
>> + * - Fan IDs
>> + * - Sensor IDs
>> + * - Unknown IDs
>> + * - Thermal Profile IDs
>> + *
>> + * The total number of IDs of a given type can be obtained with
>> + * AWCC_OP_GET_SYSTEM_DESCRIPTION.
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static inline int awcc_op_get_resource_id(struct wmi_device *wdev, u8 index, u32 *out)
>> +{
>> + struct wmax_u32_args args = {
>> + .operation = AWCC_OP_GET_RESOURCE_ID,
>> + .arg1 = index,
>> + .arg2 = 0,
>> + .arg3 = 0,
>> + };
>> +
>> + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>> +}
>> +
>> +static inline int awcc_op_get_current_profile(struct wmi_device *wdev, u32 *out)
>> +{
>> + struct wmax_u32_args args = {
>> + .operation = AWCC_OP_GET_CURRENT_PROFILE,
>> + .arg1 = 0,
>> + .arg2 = 0,
>> + .arg3 = 0,
>> + };
>> +
>> + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>> +}
>> +
>> +static inline int awcc_op_activate_profile(struct wmi_device *wdev, u8 profile)
>> +{
>> + struct wmax_u32_args args = {
>> + .operation = AWCC_OP_ACTIVATE_PROFILE,
>> + .arg1 = profile,
>> + .arg2 = 0,
>> + .arg3 = 0,
>> + };
>> + u32 out;
>> +
>> + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL, &args, &out);
>> +}
>> +
>> +/*
>> + * Thermal Profile control
>> + * - Provides thermal profile control through the Platform Profile API
>> + */
>> static int awcc_platform_profile_get(struct device *dev,
>> enum platform_profile_option *profile)
>> {
>> @@ -536,10 +571,8 @@ static int awcc_platform_profile_get(struct device *dev,
>> u32 out_data;
>> int ret;
>>
>> - ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_CURRENT_PROFILE,
>> - 0, &out_data);
>> -
>> - if (ret < 0)
>> + ret = awcc_op_get_current_profile(priv->wdev, &out_data);
>> + if (ret)
>> return ret;
>>
>> if (out_data == AWCC_THERMAL_MODE_GMODE) {
>> @@ -550,7 +583,7 @@ static int awcc_platform_profile_get(struct device *dev,
>> if (!is_awcc_thermal_profile_id(out_data))
>> return -ENODATA;
>>
>> - out_data &= AWCC_THERMAL_MODE_MASK;
>> + out_data = FIELD_GET(AWCC_THERMAL_MODE_MASK, out_data);
>
> Should this be part of the earlier patch??
I think I'll leave it in this patch, but I'll move it to
awcc_op_get_resource_id().
--
~ Kurt
>
>> *profile = awcc_mode_to_platform_profile[out_data];
>>
>> return 0;
>> @@ -583,8 +616,8 @@ static int awcc_platform_profile_set(struct device *dev,
>> }
>> }
>>
>> - return awcc_thermal_control(priv->wdev,
>> - priv->supported_thermal_profiles[profile]);
>> + return awcc_op_activate_profile(priv->wdev,
>> + priv->supported_thermal_profiles[profile]);
>> }
>>
>> static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>> @@ -606,8 +639,7 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>> first_mode = sys_desc[0] + sys_desc[1];
>>
>> for (u32 i = 0; i < sys_desc[3]; i++) {
>> - ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_RESOURCE_ID,
>> - i + first_mode, &out_data);
>> + ret = awcc_op_get_resource_id(priv->wdev, i + first_mode, &out_data);
>>
>> if (ret == -EIO)
>> return ret;
>>
>>
Powered by blists - more mailing lists