linux-kernel - Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1

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: <31eb2d27-fd00-4284-93f8-23504e14b47f@roeck-us.net>
Date: Mon, 1 Jul 2024 08:26:55 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and
 pwm1_enable mode 4

Quentin,

On 7/1/24 04:23, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 6/28/24 5:13 PM, Guenter Roeck wrote:
>> After setting fan1_target and setting pwm1_enable to 4,
>> the fan controller tries to achieve the requested fan speed.
>>
> 
> There's something in the docs (section `Software-RPM Control Mode (Fan Speed Regulator`) that rubs me the wrong way though.
> 
> """
> When the TACH-MODE bit (bit 1 of
> 0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% when the calculated desired value of duty
> cycle is less than 30%. Therefore, the TACH setting must be not greater than the value corresponding to the
> RPM for 30% duty cycle.
> """
> 

It turns out that the tach-mode bit is in reality the DC vs. pwm selector,
and defaults to DC. For pwm fans (4-bit fans), the bit should be set to 1.
That means that pwm1_mode should be supported to set the mode. I'll add a patch
for that.

> TACH-MODE is never modified in the driver, so its default value prevails: 0.
> 
> I'm wondering if there isn't something we need to do to make sure we're not under those 30% for TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 if current values are <30% duty cycle, or update them to be >=30%?
> 

It also says that the "the selected target speed must not be too low
to operate the fan", which makes sense. It also says that the requested
fan speed should not be below the speed translating to 30% duty cycle.
However, that is not a fixed value; it depends on the fan. Some fans may
operate at 500 rpm with a duty cycle of 30%, others at 3,000 rpm.
Looking at Figure 26, I don't think the value written into the pwm
register makes any difference in Software-RPM control mode.

With that in mind, the only thing we could do is to ensure that the
requested fan speed is within the configured low and high limits,
or in other words require the user to set the limits before writing
the target fan speed. That is a bit circular, though - the user
could still write the target speed and _then_ update the limits
to a value outside the requested limit. The best we could do would be
to sanitize settings when the mode is set to 4 and any of the limits
is changed, and return an error if an obviously wrong limit or target
speed is requested (target speed outside limit, or low limit >= high
limit). Do you think that would be worth the effort ?

Thanks,
Guenter