On 10/27/24 12:10, Derek John Clark wrote:
On Sun, Oct 27, 2024 at 11:59 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 10/27/24 11:29, Derek John Clark wrote:
On Sun, Oct 27, 2024 at 11:16 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxx/
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.
I would most definitely prefer to get it fixed.
Thanks,
Guenter
