linux-kernel - Re: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE controller driver

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

Message-ID: <20250521234935.7dad7f72@kmaincent-XPS-13-7390>
Date: Wed, 21 May 2025 23:49:35 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: Piotr Kubik <piotr.kubik@...ran.com>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, Andrew Lunn
 <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE controller
 driver

On Wed, 21 May 2025 08:04:23 +0000
Piotr Kubik <piotr.kubik@...ran.com> wrote:

> On 5/19/25 11:54, Kory Maincent wrote:
> > On Fri, 16 May 2025 13:07:18 +0000
> > Piotr Kubik <piotr.kubik@...ran.com> wrote:
> >   
> >> From: Piotr Kubik <piotr.kubik@...ran.com>
> >>
> >> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
> >> controller.
> >>
> >> Based on the TPS23881 driver code.
> >>
> >> Driver supports basic features of Si3474 IC:
> >> - get port status,
> >> - get port power,
> >> - get port voltage,
> >> - enable/disable port power.
> >>
> >> Only 4p configurations are supported at this moment.  

...

> >> +
> >> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Give time for transition to complete */
> >> +	ssleep(1);  
> > 
> > 1s sleep?! It is a lot. Why do you need this? Does it comes from the
> > datasheet? 
> 
> This comes from my experience. I didn't find it in a datasheet.
> I agree this seems a lot, but for 500ms sometimes ports were not powered up. 
> I think I'll give a try to another register and instead PB_POWER_ENABLE 
> I will try to use PB_RESET in combination with PORT_MODE as this seems
> promising.
> 
> btw. Regarding power enable/disable, I think you may have same issue in
> tps23881 as I had here as tps looks very similar to si3474.
> For Si3474 POWER_STATUS register cannot be used as an admin state register as
> it holds actual power interface status (powered/not powered) instead of its
> administrative state (enabled/disabled). 
> Ethtool in this approach was showing for both Admin state and Detection
> status always the same state - actual status.
> PB_POWER_ENABLE register cannot be used for this purpose as well as it is a
> write-only register. That's why I used PORT_MODE register, it acts like an
> admin state holder in my implementation.

Indeed I figured that the power status of the tps23881 can not be really
considered as an admin_state as described in the standard. For example, it
doesn't automatically power off in case of PD unplugged.
That's why I fixed it in the current budget evaluation strategy patch series.
The admin_state is now managed by software and the the PSE core will power on
the port if it catches a classification interrupt event or if a PD was already
plugged and classify.
https://lore.kernel.org/netdev/20250520-feature_poe_port_prio-v11-12-bbaf447e1b28@bootlin.com/

If the Si3474 behaves the same maybe you should rebase your patch on my series.

But waiting this long won't be ok, as we have rtnl lock acquired here.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com