netdev - Re: [PATCH v3 0/2] hwmon: (tps23861) add class restrictions and semi-auto mode support

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] [day] [month] [year] [list]

Message-ID: <9e4db8d7-c99f-46f3-9ddb-00b0a9261d86@roeck-us.net>
Date: Mon, 8 Sep 2025 11:02:05 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Gregory Fuchedgi <gfuchedgi@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Robert Marko <robert.marko@...tura.hr>,
 Luka Perkov <luka.perkov@...tura.hr>, Jean Delvare <jdelvare@...e.com>,
 Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, Kory Maincent <kory.maincent@...tlin.com>,
 Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 0/2] hwmon: (tps23861) add class restrictions and
 semi-auto mode support

On 9/8/25 09:39, Gregory Fuchedgi wrote:
> On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@...gutronix.de> wrote:
>>
>> On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
>>> +Cc: pse-pd maintainers and netdev mailing list
>>>
>>> On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
>>>> This patch series introduces per-port device tree configuration with poe
>>>> class restrictions. Also adds optional reset/shutdown gpios.
>>>>
>>>> Tested with hw poe tester:
>>>>    - Auto mode tested with no per-port DT settings as well as explicit port
>>>>      DT ti,class=4. Tested that no IRQ is required in this case.
>>>>    - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>>>>    - Tested current cut-offs in Semi-Auto mode.
>>>>    - On/off by default setting tested for both Auto and Semi-Auto modes.
>>>>    - Tested fully disabling the ports in DT.
>>>>    - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>>>>      with reset only, as well as with neither reset nor shutdown.
>>>>
>>>> Signed-off-by: Gregory Fuchedgi <gfuchedgi@...il.com>
>>>
>>> This entire series makes me more and more unhappy. It is not the responsibility
>>> of the hardware monitoring subsystem to control power. The hardware monitoring
>>> subsystem is for monitoring, not for control.
>>>
>>> Please consider adding a driver for this chip to the pse-pd subsystem
>>> (drivers/net/pse-pd). As it turns out, that subsystem already supports
>>> tps23881. This is a similar chip which even has a similar register set.
>>>
>>> This driver could then be modified to be an auxiliary driver of that driver.
>>> Alternatively, we could drop this driver entirely since the pse-pd subsystem
>>> registers the chips it supports as regulator which has its own means to handle
>>> telemetry.
>> Yes, Guenter is right. This driver belongs to the pse-pd framework.
> No disagreement here in principle. However, the current hwmon driver
> already implements power control and exposes it via in*_enable sysfs
> files. I found this a bit odd, but I don't write drivers often.
> My understanding of Guenter's suggestion is that it would require breaking
> this userspace API?
> 

If the enable attributes enable power to the ports, that code and functionality
is simply wrong. It should only enable (or have enabled) power _monitoring_.
As such, changing that would from my perspective be a bug fix.

And, yes, that slipped my attention when reviewing the original code.
Sorry to have to say that, but I am not perfect.

>  From a quick look at the tps23881 datasheet I can see that it is
> similar, however, it is quite different in the context of this patch.
> tps23881 (unlike tps23861) has Port Power Allocation register that can
> limit poe power class. This register can be set prior to
> detection/classification. So the extra complexity of an interrupt
> handler that decides whether to enable the power may not be required.
> 
> Perhaps it still makes sense to merge these drivers, but I don't have
> time or hardware to do it at the moment.

I didn't suggest to merge the tps23881 and tps23861 drivers; I just pointed out
that they have a similar register set.

The point here is that a hardware monitoring driver should limit itself
to hardware monitoring. Actual control should, for example, be implemented
through the regulator or thermal subsystems.

Guenter