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

[Kyle Swenson <kyle.swenson@est.tech>]

Hello Kory,

On Mon, Oct 05, 2026 at 11:42:04AM +0200, Kory Maincent wrote:
> Hello Kyle,
> 
> On 10/5/26 08:00, Kyle Swenson wrote:
> > Hello,
> > 
> > On Wed, Sep 30, 2026 at 09:31:02PM +0000, netdev-bot+sashiko@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 9 potential
> > > issue(s) to consider.
> > > 
> > > Critical: 0 · High: 1 (2) · Medium: 6 (3) · Low: 2
> > > 
> > > If you believe none of the issues are legitimate, please reply to each
> > > issue of Medium severity or higher and explain why the AI is wrong.
> > > If any of them are legitimate and you plan to respin, please let patchwork
> > > know by including "pw-bot: cr" as a separate line at the end of your reply
> > > (one such reply per series is enough).
> > > 
> > > - [High] If a PI's vpwr-supply is described in DT but not yet registered
> > >    when the LTC4266 probes, ltc4266_pi_nominal_uv() carries on (warn plus…
> > > - [Medium] ltc4266_pi_get_pw_class() reports the wrong power class to
> > >    userspace.
> > > - [Medium] ltc4266_pi_nominal_uv() swallows -EPROBE_DEFER from
> > >    of_regulator_get_optional().
> > > - [Medium] ltc4266_map_event() loses hardware events on every error exit.
> > > - [Medium] ltc4266_pi_set_pw_limit() treats every negative
> > >    ltc4266_port_get_class() result as 'not classified', including…
> > > - [Medium] ltc4266_map_event() reports
> > >    ETHTOOL_C33_PSE_EVENT_CLASSIFICATION whenever the detevn high nibble…
> > > - [Medium] ltc4266_pi_disable() is carried out by ltc4266_port_init(),
> > >    whose first write (RSTPB = BIT(chan)) removes power from the port.
> > > - [Low] The LTC4266 binding sets maxItems: 1 for pse-pi 'pairsets' but
> > >    does not list it as required, and pse-controller.yaml requires only…
> > > - [Low] ltc4266_regmap_config sets only reg_bits, val_bits and
> > >    max_register (0x5F), with no precious_reg or readable_reg callbacks.
> > > 
> > > Pre-existing issues:
> > > - [High] This problem wasn't introduced by this patch, but the new driver
> > >    makes it easy to hit.
> > > - [High] This problem wasn't introduced by this patch, but the new driver
> > >    opts into the affected path.
> > > - [Medium] This problem wasn't introduced by this patch, but: when
> > >    ops->setup_pi_matrix() fails, pse_controller_register() returns…
> > > - [Medium] This problem wasn't introduced by this patch, but:
> > >    pse_controller_unregister() calls pse_release_pis() (kfree(pcdev->pi))…
> > > - [Medium] This problem wasn't introduced by this patch, but:
> > >    pse_control_get_internal() calls try_module_get(pcdev->owner) and…
> > > 
> > > --
> > > 
> > > net: pse-pd: Add LTC4266 PSE controller driver
> > > 
> > > This commit adds a new I2C driver for the Linear Technology LTC4266 PoE PSE
> > > controller and hooks it into the PSE controller core. The driver runs the
> > > chip in semi-auto mode and maps PSE PIs to delivery channels from the
> > > device tree. It uses the chip interrupt together with the static power
> > > budget evaluation strategy.
> > > 
> > > > diff --git a/drivers/net/pse-pd/ltc4266.c b/drivers/net/pse-pd/ltc4266.c
> > > > new file mode 100644
> > > > index 0000000000000..37dea467811a4
> > > > --- /dev/null
> > > > +++ b/drivers/net/pse-pd/ltc4266.c
> > > > @@ -0,0 +1,1386 @@
> > > 
> > > [ ... ]
> > > 
> > > > +static int ltc4266_pi_disable(struct pse_controller_dev *pcdev, int id)
> > > > +{
> > > > +	struct ltc4266_port *port = ltc4266_pi_port(pcdev, id);
> > > > +
> > > > +	/* Resetting the port (RSTPB, issued at the start of ltc4266_port_init)
> > > > +	 * removes power, disables detection and classification, and clears the
> > > > +	 * port's status register. Re-init the port so that detection and
> > > > +	 * classification can happen again.
> > > > +	 */
> > > > +	return ltc4266_port_init(port);
> > > > +}
> > > 
> > > [Severity: Medium]
> > > What happens if one of the writes after the RSTPB reset in
> > > ltc4266_port_init() fails?
> > > 
> > > The first write in ltc4266_port_init() has already removed power from the
> > > port. A later failure in the OPMD, HPEN, HPMD, TLIM, DISENA or DETPB write
> > > is still returned as a failed disable, and _pse_pi_disable() then skips
> > > the budget release:
> > > 
> > > drivers/net/pse-pd/pse_core.c:_pse_pi_disable() {
> > >      ...
> > > 	ret = ops->pi_disable(pcdev, id);
> > > 	if (ret)
> > > 		return ret;
> > > 
> > > 	pse_pi_deallocate_pw_budget(&pcdev->pi[id]);
> > >      ...
> > > }
> > > 
> > > Would the core then keep budget allocated and admin_state_enabled set for
> > > a port that has no power? DETPB was never written, so detection stays off.
> > > No later classification interrupt would arrive to resync the state.
> > > 
> > 
> > Yes, it appears the TPS23881 driver has this issue as well.  I can see
> > the contract go in one of two directions: drivers only return non-zero
> > if the port wasn't able to be disabled enough to give back power budget
> > or the core's _pse_pi_disable should always free the budget after
> > calling pi_disable and then return the error at the end of the
> > _pse_pi_disable.
> > 
> > Kory, Oleksij, what are your thoughts?
> 
> Mmh that's indeed a policy we need to decide.
> My first thought was in favor of always freeing the budget with a warning
> message. But after a second though we need to decide what is the harmless.
> Deallocating the budget while the PI is still power on could lead to an
> hardware over budget which mean there could be smoke.
> So I think the first option is the safer. It would be good to add kdoc and a
> warning to explain this.
> 

Yeah, I agree the first option is probably safest- PSE drivers should
return non-zero if they are unable to disable the port to the point it
no longer powers a device and in that case the pse_core will not free up
the budget.  If a driver encounters an error after the point the PI is
disabled (for example, this driver's port_init phase), the driver
callback will still return 0 but maybe should emit a warning message
since (depending on the error) the port won't be able to re-power
devices later.

Let me know if I misunderstood, and I'll wait a bit before posting this
fix to give other people a chance to think about it and comment.

> > > [ ... ]
> > > 
> > > > +static int ltc4266_pi_get_admin_state(struct pse_controller_dev *pcdev, int id,
> > > > +				      struct pse_admin_state *admin_state)
> > > > +{
> > > 
> > > [ ... ]
> > > 
> > > > +	ret = regmap_read(port->ltc4266->regmap, LTC4266_REG_STATPWR, &val);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > 
> > > [Severity: Medium]
> > > This isn't a bug introduced by this patch, but an I2C error returned here
> > > reaches a module reference leak that already exists in the PSE core.
> > > pse_control_get_internal() calls try_module_get(pcdev->owner) and then
> > > does this:
> > > 
> > > drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
> > >      ...
> > > 	ret = pse_pi_is_hw_enabled(pcdev, index);
> > > 	if (ret < 0)
> > > 		goto free_psec;
> > >      ...
> > > }
> > > 
> > > That jump skips the put_module label. If an I2C error happens while a PHY
> > > is acquiring its PSE control, does the ltc4266 module stay pinned?
> > > 
> > 
> > Sure seems like it.  It appears like it'd also stay pinned in a few of
> > the other goto free_psec; statements after the try_module_get.  I'll
> > plan on pushing a separate patch with an appropriate Fixes: tag later,
> > since I'm hesitant to mix it in with a v4 of this series.
> 
> If there are fixes to be sent please send them standalone to net and with
> the fixes tag.

Yep, will do.  

> 
> > > [Severity: High]
> > > Continuing with a deferred vpwr-supply also has a functional effect. Can
> > > the PI end up with no PSE power domain for good?
> > > 
> > > If the supply is not registered yet, regulator_resolve_supply() returns
> > > -EPROBE_DEFER and regulator_register() leaves rdev->supply NULL for the PI
> > > regulator. pse_register_pw_ds() runs only once, from
> > > pse_controller_register():
> > > 
> > > drivers/net/pse-pd/pse_core.c:pse_register_pw_ds() {
> > >      ...
> > > 		if (!rdev || !rdev->supply)
> > > 			continue;
> > >      ...
> > > }
> > > 
> > > So pi->pw_d stays NULL even after the supply resolves, and
> > > pse_pw_d_is_sw_pw_control() returns false for this PI. Two things follow.
> > > 
> > > First, pse_isr() never calls pse_set_config_isr(). The CLASSIFICATION and
> > > DISCONNECTION events from ltc4266_map_event() then never power the port
> > > or re-arm it.
> > > 
> > > Second, on admin enable pse_pi_enable() calls ltc4266_pi_enable()
> > > directly. That fails in ltc4266_port_get_class() with -EINVAL or -EAGAIN
> > > when no PD has been classified.
> > > 
> > > The chip runs in semi-auto mode and never powers a port without a PWRPB
> > > write. Would a PD that is plugged in later stay unpowered until the
> > > driver is rebound?
> > > 
> > > This only affects a supply that is described but deferred. When
> > > vpwr-supply is missing entirely, the dummy regulator is used and a power
> > > domain is still created.
> > > 
> > 
> > This seems correct to me, and I'm pretty sure the cleanest option is to
> > fix the pse-core.
> 
> It seems there are still several flaws in the PSE core.
> If you have the time, please fix them, but tell me if you don't. I will
> tackle them.

I'll make time to fix them, and I'll let you know if I'm running out
time or won't get to some of them.  I've got a few of the fixes locally,
just need to get them through my testing and internal review.

> 
> > > [Severity: High]
> > > This isn't a bug introduced by this patch, but choosing the static budget
> > > strategy exposes a lock ordering problem that already exists in the PSE
> > > core.
> > > 
> > > pse_isr() holds pcdev->lock and ends up taking the supply regulator lock:
> > > 
> > > pse_isr()
> > >    pse_set_config_isr()
> > >      _pse_pi_delivery_power_sw_pw_ctrl()
> > >        pse_pi_allocate_pw_budget_static_prio()
> > >          regulator_request_power_budget(pw_d->supply)
> > >            regulator_lock(rdev)
> > > 
> > > An admin enable running at the same time takes the locks in the opposite
> > > order:
> > > 
> > > regulator_enable(psec->ps)
> > >    regulator_lock_dependent()    (PI rdev, then its supply rdev)
> > >      _regulator_enable()
> > >        pse_pi_is_enabled() / pse_pi_enable()
> > >          mutex_lock(&pcdev->lock)
> > > 
> > > Can a classification interrupt on one PI, racing with an admin enable of
> > > another PI on the same supply, deadlock both the IRQ thread and the
> > > ethtool task? The ww_mutex scheme cannot break a cycle that includes the
> > > plain pcdev->lock mutex.
> > > 
> > > > +
> > > > +	ret = devm_pse_controller_register(ltc4266->dev, &ltc4266->pcdev);
> > > > +	if (ret)
> > > > +		return dev_err_probe(&client->dev, ret,
> > > > +				     "Failed to register PSE controller\n");
> > > 
> > 
> > Yes, this is a deadlock I've hit in my testing, and it's unclear to me
> > why lockdep doesn't seem to catch it.  This is another thing I'll fix
> > separately but it'll take some time for me to get it figured out.
> 
> Meh there is still a lock issue, again related to the regulator lock.
> It's painful.

Painful indeed. I'll work though it and post RFC for the lock change
when I've got something that's worth sharing.  If you've got ideas
already on how to fix it, I'm all ears.

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

Thanks,
Kyle