linux-kernel - Re: [RFC] hwmon: pwm

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: <CAFqHKTmEauJ4RQUrn6pjX-hgLGZLNE8gaD5S41Uy0L0cNB4+mA@mail.gmail.com>
Date: Sun, 27 Oct 2024 11:29:43 -0700
From: Derek John Clark <derekjohn.clark@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>, 
	Cryolita PukNgae <cryolitia@...il.com>, linux-hwmon@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] hwmon: pwm_enable clarification

On Sun, Oct 27, 2024 at 11:16 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 10/27/24 10:48, Derek J. Clark wrote:
> > Greetings all,
> >
> > I am working with Cryolita to fix up the GPD driver she submitted recently:
> > https://lore.kernel.org/all/20240718-gpd_fan-v4-0-116e5431a9fe@gmail.com/
> >
> > We are currently having a discussion about the meaning of this part of the
> > documentation and are seeking some guidance from upstream.
> >  >> pwm[1-*]_enable
> >>              Fan speed control method:
> >>              0: no fan speed control (i.e. fan at full speed)
> >>              1: manual fan speed control enabled (using pwm[1-*])
> >>              2+: automatic fan speed control enabled
> >>              Check individual chip documentation files for automatic mode
> >>              details.
> >>              RW
> >
> > In oxp-sensors we took 0 to mean "no kernel control" so a setting of 0 is
> > technically "automatic" but fully controlled by the hardware with no
> > interaction from the driver. In her original driver draft she had taken this
>
> That is wrong. It should be (or have been) 2 or higher. Ah yes, I can see that
> the code sets fan control to automatic when oxp_pwm_disable() is called.
> Again, that is wrong. Congratulations to the submitters, you sneaked that by
> my review. That doesn't make it better. It is still wrong, and I call it "sneaky"
> because the function is not called "oxp_pwm_automatic()" or similar, it is
> called "oxp_pwm_disable(). Setting fan control to automatic does not disable
> fan control.
>
> My bad, I should have paid closer attention, and/or maybe not have trusted
> the submitters as much as I did. I guess I'll have to pay closer attention
> in the future.
>
> > literally to have the driver set the fan speed to 100% on this setting rather
> > then give back control to the hardware. My question is simply what is the
> > correct interpretation here? Ideally I would like to see this interface match
>
> It seems to me that the above text is well defined.
>
> > as existing userspace software is expecting 0 as hardware controlled and 1 as
> > manually controlled, but we also want to ensure this is correct before we
> > submit a v5.
> >
>
> Any such userspace expectations are simply wrong. The ABI definition is above
> and, again, it is well defined.
>
>         0: no fan speed control (i.e. fan at full speed)
>
> I don't really see any ambiguity in this text. This isn't about kernel control,
> it is an absolute statement. There is no "kernel" in "no fan speed control".
> "fan at full speed" should make this even more obvious.
>
> Guenter

Guenter,

I'll keep this in mind in the future, and I will send a patch soon to fix the
oxp_sensors driver. One final question, is a "0" setting mandatory?

Thanks for the quick reply.

Derek