Re: [PATCH v5 1/1] asus-wmi: Add support for platform_profile

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 8/18/21 9:25 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2021. augusztus 18., szerda 21:07 keltezéssel, Luke D. Jones írta:
>> Add initial support for platform_profile where the support is
>> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>>
>> Because throttle_thermal_policy is used by platform_profile and is
>> writeable separately to platform_profile any userspace changes to
>> throttle_thermal_policy need to notify platform_profile.
>>
>> In future throttle_thermal_policy sysfs should be removed so that
>> only one method controls the laptop power profile.
>>
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>> ---
>>  drivers/platform/x86/Kconfig    |   1 +
>>  drivers/platform/x86/asus-wmi.c | 130 +++++++++++++++++++++++++++++++-
>>  2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index d12db6c316ea..46dec48a36c1 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> [...]
>> +static int platform_profile_set(struct platform_profile_handler *pprof,
>> +				enum platform_profile_option profile)
>> +{
>> +	struct asus_wmi *asus;
>> +	int tp;
>> +
>> +	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>> +
>> +	switch (profile) {
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>> +		break;
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> +		break;
>> +	case PLATFORM_PROFILE_QUIET:
>> +		tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	asus->throttle_thermal_policy_mode = tp;
>> +	return throttle_thermal_policy_write(asus);
> 
> Here the new mode will be stored even if activating it fails, no?

Yes I noticed this too. This is the same way how writing to
the throttle_thermal_policy sysfs file works, so this is consistent.

I know it is a bit weird, but normally the throttle_thermal_policy_write()
call never fails (throttle_thermal_policy_store does not check it at all).

And even if it fails, what does that mean we certainly did make the ACPI
call maybe the profile was actually changed but then some later ACPI code
setting up the return buffer failed ?  Or maybe the ACPI code did actually
never get around to forwarding the value to e.g. the EC ?

We basically don't know ...

IOW I think this is fine as is, it is a bit weird but it is consistent
with the existing callers of throttle_thermal_policy_write().

Regards,

Hans