Received-SPF: pass (google.com: domain of linux-kernel+bounces-26477-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Feedback-ID: i5ec1447f:Fastmail
Date: Tue, 16 Jan 2024 09:25:31 +1300
From: Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH] platform/x86: asus-wmi: Re-enable custom fan curves after
 setting throttle_thermal_policy
To: Hans de Goede <hdegoede@redhat.com>
Cc: Andrei Sabalenka <mechakotik@gmail.com>, corentin.chary@gmail.com,
	ilpo.jarvinen@linux.intel.com, acpi4asus-user@lists.sourceforge.net,
	platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Message-Id: <JQKB7S.8ATKNVGHLV1L@ljones.dev>
In-Reply-To: <e776db0e-2376-415b-8688-f166118d4007@redhat.com>
References: <20240115122315.10250-1-mechakotik@gmail.com>
	<e776db0e-2376-415b-8688-f166118d4007@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed


On Mon, Jan 15 2024 at 13:38:16 +01:00:00, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 1/15/24 13:22, Andrei Sabalenka wrote:
>>  When changing throttle_thermal_policy, all the custom fan curves 
>> are getting disabled. This patch re-enables all the custom fan 
>> curves that were enabled before changing throttle_thermal_policy.
>> 
>>  I believe it makes asus-wmi sysfs interface more convenient, as it 
>> allows userspace to manage fan curves independently from 
>> platform_profile and throttle_thermal_policy. At the kernel level, 
>> custom fan curves should not be tied to "power profiles" scheme in 
>> any way, as it gives the user less freedom of controlling them.
> 
> Setting a high performance power-profile typically also involves 
> ramping up
> the fans harder. So I don't think this patch is a good idea.
> 
> If you really want this behavior then you can always re-enable the 
> custom
> curve after changing the profile.
> 
> Luke, do you have any opinion on this?

I see some misconceptions that should be addressed:
1. ASUS themselves set separate fan curves per "platform profile", both 
standard and custom
2. fan curves are not tied to platform profiles, they are tied to the 
throttle_thermal_policy, and this is actually done in the acpi - so the 
code here is a mirror of that
3. platform-profiles are tied to throttle_thermal_policy

There is no lack of user control at all, a decent tool (like asusctl) 
can set fan curves without issues but it's perhaps not convenient for 
manually setting via a script etc.

The main reason that a curve is disabled for the policy being switched 
to is for safety. It was a paranoid choice I made at the time. The 
kernel (and acpi) can't guarantee that a user set a reasonable default 
for that policy so the safest thing is to force an explicit re-enable 
of it.

Having said that: I know that the curve was previously set for that 
profile/policy and in theory should be fine plus it is already used by 
the user, it is also not possible to set a curve for a different 
profile to the one a user is currently in -  this is forced in ACPI as 
you can set only the curve for the profile you are in (the kernel code 
also mirrors this).

So this patch should be fine.

Signed-off-by: Luke D. Jones <luke@ljones.dev>

> 
>> 
>>  Signed-off-by: Andrei Sabalenka <mechakotik@gmail.com>
>>  ---
>>   drivers/platform/x86/asus-wmi.c | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 18be35fdb..c2e38f6d8 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -3441,13 +3441,28 @@ static int 
>> throttle_thermal_policy_write(struct asus_wmi *asus)
>>   		return -EIO;
>>   	}
>> 
>>  -	/* Must set to disabled if mode is toggled */
>>  -	if (asus->cpu_fan_curve_available)
>>  -		asus->custom_fan_curves[FAN_CURVE_DEV_CPU].enabled = false;
>>  -	if (asus->gpu_fan_curve_available)
>>  -		asus->custom_fan_curves[FAN_CURVE_DEV_GPU].enabled = false;
>>  -	if (asus->mid_fan_curve_available)
>>  -		asus->custom_fan_curves[FAN_CURVE_DEV_MID].enabled = false;
>>  +	/* Re-enable fan curves after profile change */
>>  +	if (asus->cpu_fan_curve_available && 
>> asus->custom_fan_curves[FAN_CURVE_DEV_CPU].enabled) {
>>  +		err = fan_curve_write(asus, 
>> &asus->custom_fan_curves[FAN_CURVE_DEV_CPU]);
>>  +		if (err) {
>>  +			pr_warn("Failed to re-enable CPU fan curve: %d\n", err);
>>  +			return err;
>>  +		}
>>  +	}
>>  +	if (asus->gpu_fan_curve_available && 
>> asus->custom_fan_curves[FAN_CURVE_DEV_GPU].enabled) {
>>  +		err = fan_curve_write(asus, 
>> &asus->custom_fan_curves[FAN_CURVE_DEV_GPU]);
>>  +		if (err) {
>>  +			pr_warn("Failed to re-enable GPU fan curve: %d\n", err);
>>  +			return err;
>>  +		}
>>  +	}
>>  +	if (asus->mid_fan_curve_available && 
>> asus->custom_fan_curves[FAN_CURVE_DEV_MID].enabled) {
>>  +		err = fan_curve_write(asus, 
>> &asus->custom_fan_curves[FAN_CURVE_DEV_MID]);
>>  +		if (err) {
>>  +			pr_warn("Failed to re-enable MID fan curve: %d\n", err);
>>  +			return err;
>>  +		}
>>  +	}
>> 
>>   	return 0;
>>   }
>