Message-Id: <DFPDCIVCEI8B.19QMW0HTA0PE0@gmail.com>
Date: Thu, 15 Jan 2026 13:19:41 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Derek J. Clark" <derekjohn.clark@...il.com>, "Rong Zhang" <i@...g.moe>,
 "Kurt Borja" <kuurtb@...il.com>
Cc: "Mark Pearson" <mpearson-lenovo@...ebb.ca>, "Armin Wolf"
 <W_Armin@....de>, "Hans de Goede" <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Guenter
 Roeck" <linux@...ck-us.net>, <platform-driver-x86@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v9 7/7] platform/x86: lenovo-wmi-other: Add HWMON for
 fan reporting/tuning

On Thu Jan 15, 2026 at 12:45 PM -05, Derek J. Clark wrote:
> On January 15, 2026 8:57:42 AM PST, Rong Zhang <i@...g.moe> wrote:
>>Hi Kurt,
>>
>>On Thu, 2026-01-15 at 08:58 -0500, Kurt Borja wrote:
>>> On Thu Jan 15, 2026 at 8:03 AM -05, Rong Zhang wrote:
>>> > Hi Kurt,
>>> > 
>>> > On Wed, 2026-01-14 at 19:16 -0500, Kurt Borja wrote:
>>> > > Hi Rong,
>>> > > 
>>> > > On Wed Jan 14, 2026 at 7:27 AM -05, Rong Zhang wrote:
>>> > > > Register an HWMON device for fan reporting/tuning according to
>>> > > > Capability Data 00 (capdata00) and Fan Test Data (capdata_fan) provided
>>> > > > by lenovo-wmi-capdata. The corresponding HWMON nodes are:
>>> > > > 
>>> > > >  - fanX_enable: enable/disable the fan (tunable)
>>> > > >  - fanX_input: current RPM
>>> > > >  - fanX_max: maximum RPM
>>> > > >  - fanX_min: minimum RPM
>>> > > >  - fanX_target: target RPM (tunable)
>>> > > > 
>>> > > > Information from capdata00 and capdata_fan are used to control the
>>> > > > visibility and constraints of HWMON attributes. Fan info from capdata00
>>> > > > is collected on bind, while fan info from capdata_fan is collected in a
>>> > > > callback. Once all fan info is collected, register the HWMON device.
>>> > > > 
>>> > > > Signed-off-by: Rong Zhang <i@...g.moe>
>>> > > > Reviewed-by: Derek J. Clark <derekjohn.clark@...il.com>
>>> > > > ---
>>> > > 
>>> > > ...
>>> > > 
>>> > > > diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
>>> > > > index 821282e07d93c..bd1d733ff286d 100644
>>> > > > --- a/Documentation/wmi/devices/lenovo-wmi-other.rst
>>> > > > +++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
>>> > > > @@ -31,6 +31,8 @@ under the following path:
>>> > > >  
>>> > > >    /sys/class/firmware-attributes/lenovo-wmi-other/attributes/<attribute>/
>>> > > >  
>>> > > > +Additionally, this driver also exports attributes to HWMON.
>>> > > > +
>>> > > >  LENOVO_CAPABILITY_DATA_00
>>> > > >  -------------------------
>>> > > >  
>>> > > > @@ -39,6 +41,11 @@ WMI GUID ``362A3AFE-3D96-4665-8530-96DAD5BB300E``
>>> > > >  The LENOVO_CAPABILITY_DATA_00 interface provides various information that
>>> > > >  does not rely on the gamezone thermal mode.
>>> > > >  
>>> > > > +The following HWMON attributes are implemented:
>>> > > > + - fanX_enable: enable/disable the fan (tunable)
>>> > > 
>>> > > I was testing this series and I'm a bit confused about fanX_enable.
>>> > 
>>> > Thanks for testing!
>>> 
>>> Thanks for working on this!
>>> 
>>> > 
>>> > > Judging by this comment and also by taking a quick look at the code, it
>>> > > looks like writting 0 to this attribute disables the fan.
>>> > > 
>>> > > This is however (per hwmon ABI documentation [1]) not how this attribute
>>> > > should work. IIUC, it is intended for devices which can disable the fan
>>> > > sensor, not the actual fan.
>
> I agree, it's just for disabling the reporting of the rpm, not for disabling the fan. I didn't catch this before.
>
>>> > 
>>> > Hmm, what a confusing name :-/
>>> > 
>>> > > I fail to see how this feature is useful and I also find it dangerous
>>> > > for this to be exposed by default, considering the same could be
>>> > > achieved with the relaxed module parameter, which at least tells the
>>> > > user to be careful.
>>> > 
>>> > Makes sense. I will remove the attribute and mention such behavior in
>>> > the module parameter.
>>> 
>>> Also, it would be worth to mention that writting 0 to the fanY_target
>>> attribute is auto mode, right?
>>
>>Ah, yes.
>>
>>> I was testing the fanX_target attribute and it does work as intended,
>>> i.e. the fan speed changes to the desired speed. However, every time I
>>> write to this attribute I'm getting -EIO error and it always reads 0.
>>> 
>>> For example:
>>> 
>>> 	$ echo 3550 | sudo tee fan*_target
>>> 	3550
>>> 	tee: fan1_target: Input/output error
>>> 	tee: fan2_target: Input/output error
>>> 	$ cat fan*_target
>>> 	0
>>> 	0
>>> 
>>> But as I said, the fans do reach the desired speed (an approximation of
>>> it):
>>> 
>>> 	$ cat fan*_input
>>> 	3500
>>> 	3500
>>> 
>>> This is a bit weird, but I haven't look in depth into it. I will find
>>> some time to do it later. This happens on a 83KY (Legion 7 16IAX1)
>>> laptop.
>>
>>I'd like to fix it in the next revision. Looking forward to your
>>progress in debugging :-)
>>
>>It seems to me that the corresponding ACPI method did not return 1 on
>>success. There should be some clues in the decompiled ASL code. Could
>>you attach it in your reply? The HWMON implementation was developed
>>mostly based on my understanding on the decompiled ASL code of my
>>device. I'd like to check the one of your device as a cross-reference
>>to see if there are more potential bugs.
>>
>>> As it seems that you can change the RPM in 100 increments,
>>
>>Yeah. That's also true on my device, but I am not sure if all devices
>>with the interface behave like this. I'd prefer not making such an
>>assumption.
>>
>
> fanY_div maybe makes sense here, defaulting to the known 100 and adjustable with a DMI quirk table if we find devices that do 200/50/20/etc.?
>
>>> maybe you
>>> could look into the pwmY attributes [1]. I think it is a better fit for
>>> this feature because pwmY_enable allows you to select between manual and
>>> auto fan control [2], and I believe some user-space tools already use
>>> this attribute.
>>
>>For pwmY, I don't see the point why the kernel should adapt to
>>userspace tools... If we don't have to, don't be overfit.
>>
>>For pwmY_enable, it is only a good choice if we adopt pwmY. It's weird
>>to mix pwmY_enable and fanY_target.
>>
>>@Derek, may I ask for your opinion here? Should we adopt pwmY*?
>>
>
> All the platform drivers I've written used pwmY/pwmY_enable and fanY_input for reporting speed. There is additionally a pwmY_enable mode that sets the fan to max speed. TBS, I think it's a fundamentally different interface.
>
>>> On the implementation you can use fixp_linear_interpolate() [3] to
>>> convert between and from pwm duty cycle.
>>
>>If we adopt this, I could imagine three ways to create a PWM-RPM curve:
>>(1) PWM[0,255] => RPM[min,max]
>>(2) PWM[0,255] => RPM[0,max]
>>(3) PWM[0,255] => RPM[0,a_large_number]
>>
>
> I don't believe this is canonical for pwm, normally you set a fixed speed with pwmY, let the BIOS handle it with auto, or set to max speed with the enable function. Fan curves would need to be under pwmY_auto_point_pwm[X|Z], but they're usually paired with tempY_auto_point_pwm[X|Y] to set a speed at a given temperature, not necessary to restrict a min/max.
>
> Docs: https://docs.kernel.org/hwmon/sysfs-interface.html
>
> The nature of pwm is that a fixed pulse width determines a fixed fan speed. It is necessarily a single set value. If you want a range, fanY_[min|max] seems the way to go.
>
> Using pwmY* will also collide with the fan curve patch I'm adding after this series since newer devices have a 10 value speed Y at temp X curve built into the WMI interface. Not insurmountable, but I'd rather avoid the conflict so we're not matching on GUID or something like that.
>
> Cheers,
> Derek
>

You're right. It would not be possible to enforce limits when using
pwmY*. We should stick with fanY_target, but the 0 value auto mode
should be documented.


-- 
Thanks,
 ~ Kurt

Powered by blists - more mailing lists