Message-ID: <528fed98-4fe0-41b6-b148-cf950866cdb3@roeck-us.net>
Date: Sun, 27 Oct 2024 13:05:36 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Derek John Clark <derekjohn.clark@...il.com>
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 10/27/24 12:10, Derek John Clark wrote:
> 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.
> 

I would most definitely prefer to get it fixed.

Thanks,
Guenter

Powered by blists - more mailing lists