linux-kernel - 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: <CAAcybusjm00hv+W-pKaNpPedXuWeTu5mo=i6TKnTJgVurO_ryg@mail.gmail.com>
Date: Wed, 10 Sep 2025 12:11:10 -0700
From: Gregory Fuchedgi <gfuchedgi@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, 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 Mon, Sep 8, 2025 at 11:02 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> 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.

Alright, then. I'll try to find some time in the next few months to port this
over to a separate driver in pse-pd. And then remove the in*_enable from hwmon
one. Even if it's there by mistake, probably shouldn't fix it until there's an
alternative in place.

>
> 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.