linux-kernel - Re: [PATCH 2/3] hwmon: (nct6775) Fix logic error for PWM enable

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]

Date:   Thu, 16 Nov 2023 00:48:42 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     "xingtong.wu" <xingtong_wu@....com>
Cc:     Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, xingtong.wu@...mens.com,
        tobias.schaffner@...mens.com, gerd.haeussler.ext@...mens.com
Subject: Re: [PATCH 2/3] hwmon: (nct6775) Fix logic error for PWM enable

On Thu, Nov 16, 2023 at 04:36:39PM +0800, xingtong.wu wrote:
> 
> On 2023/11/16 16:07, Guenter Roeck wrote:
> > On Thu, Nov 16, 2023 at 10:23:29AM +0800, Xing Tong Wu wrote:
> >> From: Xing Tong Wu <xingtong.wu@...mens.com>
> >>
> >> The determination of the "pwm_enable" should be based solely on the mode,
> >> regardless of the pwm value.
> >> According to the specification, the default values for pwm and pwm_enable
> >> are 255 and 0 respectively. However, there is a bug in the code where the
> >> fan control is actually enabled, but the file "pwm_enable" incorrectly
> >> displays "off", indicating that fan control is disabled. This contradiction
> >> needs to be addressed and resolved.
> >> Solution: Update the logic so that "pwm_enable" is determined by mode + 1,
> >> remove the "off" value for "pwm_enable" since it is not specified in the
> >> documentation.
> > 
> > The chip specification is irrelevant. What is relevant is the hwmon ABI,
> > which says
> > 
> > What:           /sys/class/hwmon/hwmonX/pwmY_enable
> > Description:
> >                 Fan speed control method:
> > 
> >                 - 0: no fan speed control (i.e. fan at full speed)
> > 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I think this description may lead to misunderstanding. There are certain
> fans that cannot be controlled and operate at full speed while system is
> running. However, there are also fans whose speed can be controlled, even
> if they are currently set to full speed. In this particular situation, it
> would be better to inform the user that the fan can still be controlled
> despite being at full speed.
> How do you think that?

We need to be consistent. Yes, it might be arguable that we should
not return 0 if fan control can not be disabled by the chip, but that would
effectively be a behavioral change. We don't know if there is some userspace
program which expects to be able to disable fan control completely (and,
when doing so, setting fan speed to its maximum). Given that, I don't think
it is feasible to change the behavior.

Guenter