linux-kernel - Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for vivobook fan profiles

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: <4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com>
Date: Wed, 05 Jun 2024 19:43:08 +1200
From: "Luke Jones" <luke@...nes.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Hans de Goede" <hdegoede@...hat.com>,
 "Mohamed Ghanmi" <mohamed.ghanmi@...com.tn>, corentin.chary@...il.com,
 platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for vivobook fan
 profiles



On Mon, 3 Jun 2024, at 10:29 PM, Ilpo Järvinen wrote:
> On Mon, 3 Jun 2024, Luke Jones wrote:
> > On Mon, 29 Apr 2024, at 10:20 PM, Hans de Goede wrote:
> > > On 4/21/24 9:43 PM, Mohamed Ghanmi wrote:
> > > > Add support for vivobook fan profiles wmi call on the ASUS VIVOBOOK
> > > > to adjust power limits.
> > > > 
> > > > These fan profiles have a different device id than the ROG series
> > > > and different order. This reorders the existing modes and adds a new
> > > > full speed mode available on these laptops.
> > > > 
> > > > As part of keeping the patch clean the throttle_thermal_policy_available
> > > > boolean stored in the driver struct is removed and
> > > > throttle_thermal_policy_dev is used in place (as on init it is zeroed).
> > > > 
> > > > Signed-off-by: Mohamed Ghanmi <mohamed.ghanmi@...com.tn>
> > > > Co-developed-by: Luke D. Jones <luke@...nes.dev>
> > > > Signed-off-by: Luke D. Jones <luke@...nes.dev>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > > > ---
> > > >  drivers/platform/x86/asus-wmi.c            | 93 ++++++++++++----------
> > > >  include/linux/platform_data/x86/asus-wmi.h |  1 +
> > > >  2 files changed, 51 insertions(+), 43 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > > > index 3c61d75a3..1f54596ca 100644
> > > > --- a/drivers/platform/x86/asus-wmi.c
> > > > +++ b/drivers/platform/x86/asus-wmi.c
> 
> > > > @@ -3747,7 +3753,10 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> > > >  return count;
> > > >  }
> > > >  
> > > > -// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
> > > > +/*
> > > > + * Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
> > > > + * Throttle thermal policy vivobook : 0 - default, 1 - silent, 2 - overboost, 3 - fullspeed
> > > > + */
> > > 
> > > throttle_thermal_policy_write() always expects normal (non vivobook) values and
> > > then translates those to vivo values, so this comment is not correct.
> > > 
> > > The only difference is that vivobook also has fullspeed, but the way userspace
> > > sees it 1/2 or silent/overspeed are not swapped, since the swapping is taking
> > > care of in throttle_thermal_policy_write().
> > > 
> > > Also the new fullspeed is not exported through the platform_profile interface,
> > > for setting values this is somewhat ok, but fullspeed can be set through
> > > sysfs, and this will then cause asus_wmi_platform_profile_get() to fail
> > > with -EINVAL, so this need to be fixed. Either map fullspeed to
> > > PLATFORM_PROFILE_PERFORMANCE in asus_wmi_platform_profile_get(), or add
> > > a new platform_profile value for this.
> > >
> > 
> > I would much prefer if "fullspeed" was not included at all unless it was 
> > an individual setting. It very rarely contributes anything good to the 
> > driver, and most certainly won't be of value in the platform_profile.
> > 
> > Otherwise, what is the status on this? 
> 
> Hi,
> 
> I was expecting an update that addresses Hans' review comment.
> 
> Luke, are you arguing that his comment is not valid and v3 is fine?
> 
> (In any case, I've summoned v3 back from archive into active patches in 
> patchwork so this doesn't get forgotten if v4 is not needed).
> 
> -- 
> i.

Hi.

I am saying I would like to see ASUS_THROTTLE_THERMAL_POLICY_FULLSPEED removed, or placed somewhere else such as in <sysfs>/asus-nb-wmi/hwmon/hwmon3/pwm1_enable.

It certainly shouldn't be included in platform_profile and I'm not keen on seeing it in thorttle_thermal_policy either.

A lot of this reasoning is now coming from the refactor I've just done of asus-wmi and the "features" such as this one to place them under firmware_attributes class and begin deprecation of them in asus_wmi. From what I've achieved so far with it it is much *much* more suitable than this ad-hoc style of adding features I've been doing here (I'll submit this work soon, it repalces the last patch series I did).

In light of the above I'm considering the possibility of removing throttle_thermal_policy completely to wholly favour the use of platform_profile. It doesn't make all that much sense to have two different things manipulating the same thing - and as such I don't think the "full speed fan" setting fits at all with platform_profile as it is not a performance tweak.

Mohamed, I would be happy to include the Vivo support so long as:
1. the fullspeed setting is removed
2. the modes map correctly to platform_profile

I hope this makes sense. Very sorry about the previous brief response (I was recovering from surgery trauma).

Cheers,
Luke.