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: <JQKB7S.8ATKNVGHLV1L@ljones.dev>
Date: Tue, 16 Jan 2024 09:25:31 +1300
From: Luke Jones <luke@...nes.dev>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Andrei Sabalenka <mechakotik@...il.com>, corentin.chary@...il.com,
	ilpo.jarvinen@...ux.intel.com, acpi4asus-user@...ts.sourceforge.net,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: asus-wmi: Re-enable custom fan curves after
 setting throttle_thermal_policy



On Mon, Jan 15 2024 at 13:38:16 +01:00:00, Hans de Goede 
<hdegoede@...hat.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@...nes.dev>

> 
>> 
>>  Signed-off-by: Andrei Sabalenka <mechakotik@...il.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;
>>   }
> 



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ