linux-kernel - Re: [PATCH v2 02/10] platform/x86: alienware-wmi-wmax: Refactor is_awcc_thermal

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: <2f8f3818-f1ba-c7b8-f3e5-148436b930f6@linux.intel.com>
Date: Tue, 4 Mar 2025 17:02:49 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Kurt Borja <kuurtb@...il.com>
cc: Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org, 
    Hans de Goede <hdegoede@...hat.com>, Dell.Client.Kernel@...l.com, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 02/10] platform/x86: alienware-wmi-wmax: Refactor
 is_awcc_thermal_mode()

On Tue, 25 Feb 2025, Kurt Borja wrote:

> Refactor is_awcc_thermal_mode() to use FIELD_GET() instead of bitwise
> operations. Drop the check for BIT(8) sensor flag and rename it to
> is_awcc_thermal_profile_id().
> 
> Signed-off-by: Kurt Borja <kuurtb@...il.com>
> Reviewed-by: Armin Wolf <W_Armin@....de>
> ---
>  .../platform/x86/dell/alienware-wmi-wmax.c    | 31 ++++++++++---------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> index ed70e12d73d7..7f0aa88221d6 100644
> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> @@ -34,7 +34,7 @@
>  #define AWCC_FAILURE_CODE			0xFFFFFFFF
>  #define AWCC_THERMAL_TABLE_MASK			GENMASK(7, 4)
>  #define AWCC_THERMAL_MODE_MASK			GENMASK(3, 0)
> -#define AWCC_SENSOR_ID_MASK			BIT(8)
> +#define AWCC_RESOURCE_ID_MASK			GENMASK(7, 0)
>  
>  static bool force_platform_profile;
>  module_param_unsafe(force_platform_profile, bool, 0);
> @@ -168,8 +168,8 @@ enum AWCC_GAME_SHIFT_STATUS_OPERATIONS {
>  };
>  
>  enum AWCC_THERMAL_TABLES {
> -	AWCC_THERMAL_TABLE_LEGACY		= 0x90,
> -	AWCC_THERMAL_TABLE_USTT			= 0xA0,
> +	AWCC_THERMAL_TABLE_LEGACY		= 0x9,
> +	AWCC_THERMAL_TABLE_USTT			= 0xA,
>  };
>  
>  enum awcc_thermal_profile {
> @@ -445,20 +445,18 @@ const struct attribute_group wmax_deepsleep_attribute_group = {
>   * Thermal Profile control
>   *  - Provides thermal profile control through the Platform Profile API
>   */
> -static bool is_awcc_thermal_mode(u32 code)
> +static bool is_awcc_thermal_profile_id(u8 code)
>  {
> -	if (code & AWCC_SENSOR_ID_MASK)
> -		return false;
> +	u8 table = FIELD_GET(AWCC_THERMAL_TABLE_MASK, code);
> +	u8 mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, code);
>  
> -	if ((code & AWCC_THERMAL_MODE_MASK) >= AWCC_PROFILE_LAST)
> +	if (mode >= AWCC_PROFILE_LAST)
>  		return false;
>  
> -	if ((code & AWCC_THERMAL_TABLE_MASK) == AWCC_THERMAL_TABLE_LEGACY &&
> -	    (code & AWCC_THERMAL_MODE_MASK) >= AWCC_PROFILE_LEGACY_QUIET)
> +	if (table == AWCC_THERMAL_TABLE_LEGACY && mode >= AWCC_PROFILE_LEGACY_QUIET)
>  		return true;
>  
> -	if ((code & AWCC_THERMAL_TABLE_MASK) == AWCC_THERMAL_TABLE_USTT &&
> -	    (code & AWCC_THERMAL_MODE_MASK) <= AWCC_PROFILE_USTT_LOW_POWER)
> +	if (table == AWCC_THERMAL_TABLE_USTT && mode <= AWCC_PROFILE_USTT_LOW_POWER)
>  		return true;
>  
>  	return false;
> @@ -548,7 +546,7 @@ static int awcc_platform_profile_get(struct device *dev,
>  		return 0;
>  	}
>  
> -	if (!is_awcc_thermal_mode(out_data))
> +	if (!is_awcc_thermal_profile_id(out_data))
>  		return -ENODATA;
>  
>  	out_data &= AWCC_THERMAL_MODE_MASK;
> @@ -597,6 +595,7 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>  	u32 first_mode;
>  	u32 out_data;
>  	int ret;
> +	u8 id;
>  
>  	ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_SYSTEM_DESCRIPTION,
>  				       0, (u32 *) &sys_desc);
> @@ -615,12 +614,14 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>  		if (ret == -EBADRQC)
>  			break;
>  
> -		if (!is_awcc_thermal_mode(out_data))
> +		/* Some IDs have a BIT(8) flag that should be ignored */

I find the place of this comment odd. If you want to keep it, it should be 
next to the GENMASK() as in this place I'm trying to find the code that 
implements what you commented but its nowhere near here.

> +		id = FIELD_GET(AWCC_RESOURCE_ID_MASK, out_data);
> +		if (!is_awcc_thermal_profile_id(id))
>  			continue;
>  
> -		mode = out_data & AWCC_THERMAL_MODE_MASK;
> +		mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, id);
>  		profile = awcc_mode_to_platform_profile[mode];
> -		priv->supported_thermal_profiles[profile] = out_data;
> +		priv->supported_thermal_profiles[profile] = id;
>  
>  		set_bit(profile, choices);
>  	}
> 

-- 
 i.