Re: [PATCH 2/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies

[Armin Wolf <W_Armin@gmx.de>]

Am 26.10.24 um 11:56 schrieb Hans de Goede:

> Hi Armin,
>
> On 25-Oct-24 9:15 PM, Armin Wolf wrote:
>> When changing the thermal policy using the platform profile API,
>> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>>
>> However everywhere else a normal thermal policy is stored inside this
>> variable, potentially confusing the platform profile.
> You say "potentially confusing the platform profile", but did you
> spot any actual issues when reviewing the code ?
>
Yes, for example:

1. User sets thermal policy to "1" (overboost) through the throttle_thermal_policy sysfs attr.

2. "1" gets stored inside throttle_thermal_policy_mode.

3. Platform profile will now think that ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO (1) is set.

4. Platform profile reports current mode as PLATFORM_PROFILE_QUIET.

=> error!

>> Fix this by always storing normal thermal policy values inside
>> throttle_thermal_policy_mode and only do the conversion when writing
>> the thermal policy to hardware.
>>
>> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> The problem with this approach is that it changes the order in which
> we step through the modes in throttle_thermal_policy_switch_next()
> after this change we have:
>
> Normal Asus: balanced -> performance -> silent -> balanced -> etc.
> Vivobook:    balanced -> silent -> performance -> balanced -> etc.
>
> where if we see "silent" as lower performance then the other 2,
> the vivobook order is a bit weird.
>
> I wonder if this is a big enough issue to really worry about it
> though; and I do like the cleanup / simpler code.

I think users expect the normal Asus switching behavior, but you are right
that this change should be explained inside the commit message.

>
> Note that this also causes a behavior change in
> fan_curve_get_factory_default() as I mentioned in my cover-letter.

Since fan_curve_get_factory_default() was introduced before commit bcbfcebda2cb,
i think this patches actually fixes the behavior of this function.

>
> I think that that behavior change might be a good thing to do actually,
> but at a minimum it needs to be documented in the commit msg.
>
> Regards,
>
> Hans
>
I will wait till the maintainer of asus-wmi can clarify whether or not
fan_curve_get_factory_default() expects normal Asus thermal policy values
or not.

Once we resolved this i will send an updated series.

Thanks,
Armin Wolf

>> ---
>>   drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
>>   1 file changed, 21 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index ab9342a01a48..ce60835d0303 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
>>   /* Throttle thermal policy ****************************************************/
>>   static int throttle_thermal_policy_write(struct asus_wmi *asus)
>>   {
>> -	u8 value = asus->throttle_thermal_policy_mode;
>>   	u32 retval;
>> +	u8 value;
>>   	int err;
>>
>> +	if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
>> +		switch (asus->throttle_thermal_policy_mode) {
>> +		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> +			break;
>> +		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> +			break;
>> +		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> +			value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		value = asus->throttle_thermal_policy_mode;
>> +	}
>> +
>>   	err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
>>   				    value, &retval);
>>
>> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>>   static DEVICE_ATTR_RW(throttle_thermal_policy);
>>
>>   /* Platform profile ***********************************************************/
>> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
>> -{
>> -	bool vivo;
>> -
>> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> -	if (vivo) {
>> -		switch (mode) {
>> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
>> -		}
>> -	}
>> -
>> -	return mode;
>> -}
>> -
>> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
>> -{
>> -	bool vivo;
>> -
>> -	vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
>> -
>> -	if (vivo) {
>> -		switch (mode) {
>> -		case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>> -		case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
>> -			return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>> -		}
>> -	}
>> -
>> -	return mode;
>> -}
>> -
>>   static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>>   					enum platform_profile_option *profile)
>>   {
>> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
>>   	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>>   	tp = asus->throttle_thermal_policy_mode;
>>
>> -	switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
>> +	switch (tp) {
>>   	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>   		*profile = PLATFORM_PROFILE_BALANCED;
>>   		break;
>> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
>>   		return -EOPNOTSUPP;
>>   	}
>>
>> -	asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
>> +	asus->throttle_thermal_policy_mode = tp;
>>   	return throttle_thermal_policy_write(asus);
>>   }
>>
>> --
>> 2.39.5
>>
>