Re: [PATCH v2 05/10] platform/x86: alienware-wmi-wmax: Improve platform profile probe

["Kurt Borja" <kuurtb@gmail.com>]

On Tue Mar 4, 2025 at 10:40 AM -05, Ilpo Järvinen wrote:
> On Tue, 25 Feb 2025, Kurt Borja wrote:
>
>> Get and store the AWCC system description in alienware_awcc_setup()
>> instead of awcc_platform_profile_probe() and add a check for integer
>> overflows to avoid misbehaviors.
>> 
>> While at it, replace set_bit() with it's non-atomic version __set_bit()
>> because `choices` belong to this thread only.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>  .../platform/x86/dell/alienware-wmi-wmax.c    | 61 ++++++++++++++-----
>>  1 file changed, 46 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index 4a8335d90b5d..965b427f8f0a 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -37,6 +37,9 @@
>>  #define AWCC_THERMAL_MODE_MASK			GENMASK(3, 0)
>>  #define AWCC_RESOURCE_ID_MASK			GENMASK(7, 0)
>>  
>> +/* Arbitrary limit based on supported models */
>> +#define AWCC_MAX_RES_COUNT			16
>> +
>>  static bool force_platform_profile;
>>  module_param_unsafe(force_platform_profile, bool, 0);
>>  MODULE_PARM_DESC(force_platform_profile, "Forces auto-detecting thermal profiles without checking if WMI thermal backend is available");
>> @@ -211,6 +214,17 @@ struct wmax_u32_args {
>>  
>>  struct awcc_priv {
>>  	struct wmi_device *wdev;
>> +	union {
>> +		u32 system_description;
>> +		struct {
>> +			u8 fan_count;
>> +			u8 temp_count;
>> +			u8 unknown_count;
>> +			u8 profile_count;
>> +		} __packed;
>> +		u8 res_count[4];
>> +	} __packed;
>
> I don't like it how __packed is now creeping its way into priv structure 
> too. It doesn't even seem useful in this case as all those are naturally 
> aligned and take 4 bytes, AFAICT. Why did you add it?

You are right, I think alignment here is already guaranteed. Alignment
is still a bit confusing for me.

I'll drop it for v3.

-- 
 ~ Kurt

>
>> +
>>  	struct device *ppdev;
>>  	u8 supported_profiles[PLATFORM_PROFILE_LAST];
>>  };
>> @@ -614,38 +628,41 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>>  	enum platform_profile_option profile;
>>  	struct awcc_priv *priv = drvdata;
>>  	enum awcc_thermal_profile mode;
>> -	u8 sys_desc[4];
>> -	u32 first_mode;
>> +	u8 id, offset = 0;
>>  	u32 out_data;
>>  	int ret;
>> -	u8 id;
>>  
>> -	ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_SYSTEM_DESCRIPTION,
>> -				       0, (u32 *) &sys_desc);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	first_mode = sys_desc[0] + sys_desc[1];
>> -
>> -	for (u32 i = 0; i < sys_desc[3]; i++) {
>> -		ret = awcc_op_get_resource_id(priv->wdev, i + first_mode, &out_data);
>> +	/*
>> +	 * Thermal profile IDs are listed last at offset
>> +	 *	fan_count + temp_count + unknown_count
>> +	 */
>> +	for (unsigned int i = 0; i < ARRAY_SIZE(priv->res_count) - 1; i++)
>> +		offset += priv->res_count[i];
>>  
>> +	for (unsigned int i = 0; i < priv->profile_count; i++) {
>> +		ret = awcc_op_get_resource_id(priv->wdev, i + offset, &out_data);
>>  		if (ret == -EIO)
>>  			return ret;
>>  
>> +		/*
>> +		 * Some devices report an incorrect number of thermal profiles
>> +		 * so the resource ID list may end prematurely
>> +		 */
>>  		if (ret == -EBADRQC)
>>  			break;
>>  
>>  		/* Some IDs have a BIT(8) flag that should be ignored */
>>  		id = FIELD_GET(AWCC_RESOURCE_ID_MASK, out_data);
>> -		if (!is_awcc_thermal_profile_id(id))
>> +		if (!is_awcc_thermal_profile_id(id)) {
>> +			dev_dbg(&priv->wdev->dev, "Unmapped thermal profile ID 0x%02x\n", id);
>>  			continue;
>> +		}
>>  
>>  		mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, id);
>>  		profile = awcc_mode_to_platform_profile[mode];
>>  		priv->supported_profiles[profile] = id;
>>  
>> -		set_bit(profile, choices);
>> +		__set_bit(profile, choices);
>>  	}
>>  
>>  	if (bitmap_empty(choices, PLATFORM_PROFILE_LAST))
>> @@ -655,7 +672,7 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>>  		priv->supported_profiles[PLATFORM_PROFILE_PERFORMANCE] =
>>  			AWCC_THERMAL_MODE_GMODE;
>>  
>> -		set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
>> +		__set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
>>  	}
>>  
>>  	return 0;
>> @@ -686,6 +703,20 @@ static int alienware_awcc_setup(struct wmi_device *wdev)
>>  	if (!priv)
>>  		return -ENOMEM;
>>  
>> +	ret = awcc_thermal_information(wdev, AWCC_OP_GET_SYSTEM_DESCRIPTION,
>> +				       0, &priv->system_description);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Sanity check */
>> +	for (unsigned int i = 0; i < ARRAY_SIZE(priv->res_count); i++) {
>> +		if (priv->res_count[i] > AWCC_MAX_RES_COUNT) {
>> +			dev_err(&wdev->dev, "Malformed system description: 0x%08x\n",
>> +				priv->system_description);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
>>  	priv->wdev = wdev;
>>  	dev_set_drvdata(&wdev->dev, priv);
>>  
>>