Message-ID: <CAFqHKTmhfYOAu9t6oZBQqQ1227Ot=Q4G_13-FHb=DmvACB+Xqg@mail.gmail.com>
Date: Sun, 27 Oct 2024 12:10:07 -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, 
	open list <linux-kernel@...r.kernel.org>, 
	Joaquín Ignacio Aramendía <samsagax@...il.com>
Subject: Re: [RFC] hwmon: pwm_enable clarification

On Sun, Oct 27, 2024 at 11:59 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 10/27/24 11:29, Derek John Clark wrote:
> > 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?
> >
>
> No, if the hardware can not support it it doesn't make sense to mandate it.
> If the hardware does not support disabling fan control, one option would be to
> set it to manual and set the fan speed to 100%, but that isn't mandatory.

Okay, thanks for the insight.

> Note though that people can now argue that fixing the driver would be an ABI
> violation in itself, because after all there is some userspace code which assumes
> that setting pwm_enable to 0 fro this driver would enable automatic mode (while
> other more generic applications reasonably expect fan control to be disabled if
> the value is set to 0). In other words, fixing one ABI violation creates another.
> That is why I am really unhappy about this.
>
> Guenter

Understandable, and I apologize for not understanding during the initial driver
development. Joaquín maintains the most prominent userspace application to use
this interface and I'm in contact with the two other developers who
use it as well.
We can certainly correct this oversight and get everyone on-board with
the correct
ABI, unless you prefer we keep it as is.

Derek

Powered by blists - more mailing lists