mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Kyle Swenson <kyle.swenson@est.tech>
To: "netdev-bot+sashiko@kernel.org" <netdev-bot+sashiko@kernel.org>
Cc: "o.rempel@pengutronix.de" <o.rempel@pengutronix.de>,
	"kory.maincent@bootlin.com" <kory.maincent@bootlin.com>,
	"andrew+netdev@lunn.ch" <andrew+netdev@lunn.ch>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"robh@kernel.org" <robh@kernel.org>,
	"krzk+dt@kernel.org" <krzk+dt@kernel.org>,
	"conor+dt@kernel.org" <conor+dt@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Roland Kovács" <roland.kovacs@est.tech>,
	"David Nyström" <david.nystrom@est.tech>
Subject: Re: [PATCH net-next v3 2/2] net: pse-pd: Add LTC4266 PSE controller driver
Date: Mon, 5 Oct 2026 06:00:52 +0000	[thread overview]
Message-ID: <asM8rcKC1n1Tn4JD@est-xps15> (raw)
In-Reply-To: <179080386247.434549.15817042587071985098@kernel.org>

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?

> [ ... ]
> 
> > +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.

> [ ... ]
> 
> > +static int ltc4266_pi_get_pw_class(struct pse_controller_dev *pcdev, int id)
> > +{
> > +	int ret = ltc4266_port_get_class(ltc4266_pi_port(pcdev, id));
> > +
> 
> [ ... ]
> 
> > +	if (ret == 0)
> > +		ret = 3;
> > +	if (ret < 0)
> > +		ret = 0;
> > +	return ret;
> > +}
> 
> [Severity: Medium]
> Does this report the wrong power class to userspace?
> 
> A PD that has a valid detection signature but no class signature (statp
> class code 0x6, which is class 0 in ltc4266_class_map[]) is reported as
> class 3.
> 
> Every negative result from ltc4266_port_get_class() becomes 0. That
> covers regmap I2C errors, -EAGAIN (classification not finished), -EINVAL
> (detection not good) and -ERANGE.
> 
> pse_ethtool_get_status() stores the value in status->c33_pw_class, and
> pse_fill_reply() only emits it when it is greater than zero:
> 
> net/ethtool/pse-pd.c:pse_fill_reply() {
>     ...
> 	if (st->c33_pw_class > 0 &&
> 	    nla_put_u32(skb, ETHTOOL_A_C33_PSE_PW_CLASS,
>     ...
> }
> 
> So ethtool --show-pse would show class 3 for a class 0 PD, and an I2C
> failure would look the same as no PD at all. Other drivers, such as
> tps23881_pi_get_pw_class(), return the raw class and propagate errors.
> 
> ltc4266_class_pw[] and pi_get_pw_req already treat class 0 like class 3
> for power budgeting. Could this callback report the real class instead?
> If the rewrite stays, could the commit message mention it?

So the issue I was trying to work around with this was where the ethtool
--show-pse command will omit the field if it's zero.  Since the TPS23881
driver seems to return the raw class, including zero, I'll follow suit
and adjust this in v4.

> 
> [ ... ]
> 
> > +static int ltc4266_pi_set_pw_limit(struct pse_controller_dev *pcdev,
> > +				   int id, int max_mw)
> > +{
> 
> [ ... ]
> 
> > +	class = ltc4266_port_get_class(port);
> > +	if (class >= 0) {
> > +		icut = ltc4266_pw_limit_to_icut(port, max_mw, class);
> > +		if (icut < 0)
> > +			return icut;
> > +
> > +		ret = ltc4266_port_set_icut(port, icut);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	port->pw_limit = max_mw;
> > +
> > +	return 0;
> 
> [Severity: Medium]
> Should a regmap_read() failure inside ltc4266_port_get_class() be treated
> the same as "not classified" here?

Well, my intent was that if we don't know the class of the PD connected,
how can we set an accurate power limit?  

> 
> On a powered port, a transient I2C error skips ltc4266_port_set_icut(),
> but port->pw_limit is still updated and 0 is returned.

We'll skip to the port->pw_limit ( which is the admin power limit, set
by the admin, and should be independent of the PD's class (if any)). 

> pse_ethtool_set_pw_limit() has already updated the power budget and only
> rolls it back on an error return. The core budget and the reported limit
> would then show the new value while the hardware keeps enforcing the old
> I_CUT.
>
I think the real bug is that port->pw_limit isn't always set like it
should be, so in v4 I'll re-arrange this such that the port->pw_limit is
always set, and then I'll check if there's a PD on the port that's been
classified and set the hardware limit according to the class after
assigning the pw_limit.

> [ ... ]
> 
> > +static int ltc4266_pi_nominal_uv(struct ltc4266 *ltc4266, struct device_node *np)
> > +{
> > +	struct regulator *vpwr;
> > +	int uv;
> > +
> > +	vpwr = of_regulator_get_optional(ltc4266->dev, np, "vpwr");
> > +	if (IS_ERR(vpwr)) {
> > +		/* -ENODEV means the PI describes no vpwr-supply at all, which
> > +		 * is the case the fallback exists for. -EPROBE_DEFER means the
> > +		 * rail _is_ described but has not registered yet, so the
> > +		 * fallback is wrong for it. Asking for a probe retry is not an
> > +		 * option from here: we run from setup_pi_matrix(), and
> > +		 * pse_controller_register() unwinds neither its notification
> > +		 * fifo nor its pse_pi array when that fails, so every retry
> > +		 * would leak. Warn instead so the assumed voltage is visible.
> > +		 */
> > +		if (PTR_ERR(vpwr) == -EPROBE_DEFER)
> > +			dev_warn(ltc4266->dev,
> > +				 "%pOF: vpwr-supply is not registered yet, assuming %d uV\n",
> > +				 np, LTC4266_VPORT_NOMINAL_UV);
> > +
> > +		return LTC4266_VPORT_NOMINAL_UV;
> > +	}
> 
> [Severity: Medium]
> Does swallowing -EPROBE_DEFER here actually avoid the leak described in
> the comment?
> 
> ltc4266_setup_pi_matrix() still has many other error returns that go
> through the same unwind gap in pse_controller_register(): DT validation in
> ltc4266_get_of_channels() and ltc4266_map_pis(), -ENOMEM, and I2C errors
> from ltc4266_port_init().
> 
> drivers/net/pse-pd/pse_core.c:pse_controller_register() {
>     ...
> 	if (pcdev->ops->setup_pi_matrix) {
> 		ret = pcdev->ops->setup_pi_matrix(pcdev);
> 		if (ret)
> 			return ret;
> 	}
>     ...
> }
> 
> In the deferred case, the 50 V nominal voltage stays for the whole life
> of the binding. ltc4266_pw_limit_to_icut() uses it whenever an admin limit
> is set on an unpowered port.
> 
> The deferral can happen in practice. fw_devlink only creates
> SYNC_STATE_ONLY links for suppliers referenced from child nodes, such as
> the pse-pi nodes, so the supply does not hold back this probe.
> 
> Could the unwind in pse_controller_register() be fixed in a prerequisite
> patch? Alternatively, the supplies could be resolved before
> devm_pse_controller_register() so that probe can return -EPROBE_DEFER.
> 

Fixing the EPROBE_DEFER leak in the pse_core seems like a better option
so I'll send a fix for that separately to get other people's thoughts.

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

> [ ... ]
> 
> > +		if (!pi->pairset[0].np)
> > +			return dev_err_probe(ltc4266->dev, -EINVAL,
> > +					     "%pOF has no pairsets\n", pi->np);
> 
> [Severity: Low]
> The lltc,ltc4266.yaml binding gives pairsets maxItems: 1 for each pse-pi
> node but does not list it as required. pse-controller.yaml only requires
> reg and #pse-cells, and of_load_pse_pis() also accepts a PI without
> pairsets.
> 
> Should the binding list pairsets as required? Otherwise a DT that passes
> dt_binding_check fails probe here.

Yeah, it probably should since a pse-pi node with no pairsets doesn't
make a lot of sense.

> 
> [ ... ]
> 
> > +	ret = ltc4266_get_of_channels(ltc4266, chans);
> > +	if (!ret)
> > +		ret = ltc4266_map_pis(ltc4266, chans);
> > +
> > +	for (i = 0; i < LTC4266_MAX_PORTS; i++)
> > +		of_node_put(chans[i].np);
> > +
> > +	if (ret)
> > +		return ret;
> 
> [Severity: Medium]
> This is a pre-existing issue, but every error return from
> ltc4266_setup_pi_matrix() runs into it.
> 
> When setup_pi_matrix() fails, pse_controller_register() returns without
> calling kfifo_free(&pcdev->ntf_fifo) or pse_release_pis(), and
> devm_pse_controller_register() only calls devres_free(). Would each
> failed probe leak the pcdev->pi array and the of_node references on the
> PI and pairset nodes?
> 

This is the same leak issue I plan on fixing separately with it's own
Fixes: tag.

> [ ... ]
> 
> > +		if (detevn & LTC4266_EVN_HI(chan)) {
> > +			int class = ltc4266_port_get_class(port);
> > +
> > +			if (class >= 0) {
> > +				notifs[i] |= ETHTOOL_C33_PSE_EVENT_CLASSIFICATION;
> > +				*notifs_mask |= BIT(i);
> > +			}
> > +		}
> 
> [Severity: Medium]
> Should this check whether the port is already powered before reporting a
> classification event?
>
> For an admin-enabled PI, pse_set_config_isr() calls
> _pse_pi_delivery_power_sw_pw_ctrl() on every CLASSIFICATION event.
> pse_pi_allocate_pw_budget_static_prio() does not check for an existing
> allocation: it requests budget again and overwrites pi->pw_allocated_mW.
> ltc4266_pi_enable() then rewrites ILIM, ICUT and PWRPB on the live port.
> 
> Here is one way this could happen. pse_pi_enable() powers the port while
> holding pcdev->lock, and a class event is latched at the same time. The
> IRQ thread waits for pcdev->lock and then reads the stale DETEVN for a
> port that is now powered. A classification that completes between the
> DETEVN_COR read and the PWRPB write in the ISR would have the same
> effect.
> 
> Could this leak the first budget allocation and shut off lower-priority
> ports for no reason? It depends on whether the chip raises a new class
> event for an unchanged result while MCONF.detchg is set.

It sounds to me, based off this analysis, the problem should be fixed in
the core, not this driver.

> 
> > +	}
> > +
> > +done:
> > +	return ltc4266_enable_interrupts(ltc4266);
> > +
> > +err:
> > +	/* (Attempt to) clear any remaining event registers that we might've
> > +	 * missed in the event a previous read has failed.
> > +	 */
> > +	ret = regmap_write(ltc4266->regmap, LTC4266_REG_RSTPB, LTC4266_RSTPB_INTCLR);
> > +	if (ret)
> > +		dev_warn(&ltc4266->client->dev, "Failed to clear pending interrupts, err=%d\n",
> > +			 ret);
> > +
> > +	return ltc4266_enable_interrupts(ltc4266);
> > +}
> 
> [Severity: Medium]
> Can this error path lose hardware events?
> 

Yes, seems like if there's a transient I2C error reading one of the
event registers, the remaining will get lost.

> PWREVN_COR, FLTEVN_COR, TSEVN_COR and DETEVN_COR clear on read. When a
> later read fails, the latches read before it (for example a tCUT bit in
> fltevn or a PG-lost bit in pwrevn) have already been cleared, and their
> values are thrown away.
> 
> The RSTPB_INTCLR write then clears the latched events on every channel,
> including ones that were never read. ltc4266_enable_interrupts() normally
> returns 0, so pse_isr() sees success with an empty mask and does nothing.

The intent here with the INTCLR bit was to avoid a interrupt storm if
the I2C bus is in a more permanent failure state and we're not able to
clear all the event registers.  It's easy enough to test the "persistent
failure" case on an I2C bus, but a transient (i.e. a single) I2C failure
is harder for me to test.  I'll think on this and come up with something
that balances the two extremes.

> 
> The reverse case is also a problem. If the INTMASK write in
> ltc4266_enable_interrupts() fails after decoding, pse_isr() throws away
> the notifications because of the nonzero return:
> 
> drivers/net/pse-pd/pse_core.c:pse_isr() {
>     ...
> 	ret = desc->map_event(irq, pcdev, h->notifs, h->notifs_mask);
> 	if (ret || bitmap_empty(h->notifs_mask, pcdev->nr_lines)) {
> 		mutex_unlock(&pcdev->lock);
> 		return IRQ_NONE;
>     ...
> }
> 
> INTMASK also stays at 0 from the ltc4266_disable_interrupts() call at
> entry, so no further interrupts would arrive.
> 
> With detchg set, a lost classification is not raised again. A lost fault
> or PG-loss leaves the core thinking the port is delivering, with its
> budget still allocated.
> 
> Is there any way for the driver to resync after one I2C error in the IRQ
> thread?
> 
> > +
> > +static const struct regmap_config ltc4266_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = 0x5F,
> > +};
> 
> [Severity: Low]
> Should a precious_reg callback mark the clear-on-read event registers
> (0x03, 0x05, 0x07, 0x09, 0x0B) as precious? Without it, a regmap debugfs
> registers dump reads them and clears pending detect, class and fault
> latches before the IRQ handler sees them. A readable_reg callback would
> also stop the dump from reading the reserved addresses in the gaps of the
> register map.
> 
> [ ... ]
> 
> > +	ltc4266->pcdev.owner = THIS_MODULE;
> > +	ltc4266->pcdev.ops = &ltc4266_ops;
> > +	ltc4266->pcdev.dev = &client->dev;
> > +	ltc4266->pcdev.types = ETHTOOL_PSE_C33;
> > +	ltc4266->pcdev.nr_lines = LTC4266_MAX_PORTS;
> > +	ltc4266->pcdev.supp_budget_eval_strategies = PSE_BUDGET_EVAL_STRAT_STATIC;
> 

Ack, easy enough to add.

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

> [Severity: High]
> This is a pre-existing issue in the PSE core, but boards with more than
> one LTC4266 are likely to hit it.

Reading more below, it seems like any board with more than one PSE
controller are likely to hit it assuming they're sharing a supply.

> 
> devm_pse_alloc_pw_d() allocates the power domain with
> devm_kzalloc(pcdev->dev) and publishes it in the global pse_pw_d_map.
> pse_register_pw_ds() then gives the same object to any other controller
> whose PI supply matches:
> 
> drivers/net/pse-pd/pse_core.c:pse_register_pw_ds() {
>     ...
> 			if (regulator_is_equal(pw_d->supply, rdev->supply)) {
> 				present = true;
> 				pcdev->pi[i].pw_d = pw_d;
>     ...
> }
> 
> Consider all PIs pointing at one shared supply, as in the binding
> example, or sharing the dummy regulator when vpwr-supply is omitted.
> Unbinding the first controller frees the memory through devres, but the
> xarray entry and the other controller's pointers remain.
> 
> Would the second controller's pse_flush_pw_ds() then read pw_d->id and
> call kref_put_mutex() on freed memory? pse_isr()->
> pse_pw_d_is_sw_pw_control() would use the same dangling pointer.

Seems like it.  Perhaps another pse-core fix.
> 
> [Severity: Medium]
> This is a pre-existing issue, but the teardown order in
> pse_controller_unregister() looks unsafe:
> 
> drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
>     ...
> 	pse_flush_pw_ds(pcdev);
> 	pse_release_pis(pcdev);
> 	if (pcdev->irq)
> 		disable_irq(pcdev->irq);
> 	cancel_work_sync(&pcdev->ntf_work);
>     ...
> }
> 
> pse_send_ntf_worker() takes a psec reference with
> pse_control_find_by_id() and drops it with pse_control_put(). If that is
> the last reference, __pse_control_release() reads
> psec->pcdev->pi[psec->id].admin_state_enabled after pse_release_pis() has
> freed the array.
> 
> Could the work be cancelled before the PI array is released?

Yeah, and it probably should be.

> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927212929.593366-1-kyle.swenson%40est.tech

So just to summarize, for me mostly, I'll change several things in this
driver:

- ltc4266_pi_get_pw_class() will return the raw class to match
  tps23881_pi_get_pw_class().
- ltc4266_pi_set_pw_limit() will set port->pw_limit first.
- I'll rework the error path in ltc4266_map_event() to balance clearing
  stuck event registers on a persistent I2C failure against not
  discarding events on a single transient one.
- The lltc,ltc4266.yaml binding will list pairsets as required under
  pse-pi.
- Mark the COR registers "precious."

But I'd contend the remaining fixes should be in the pse core, and I'll
push a series for that before re-spinning this series on top:

- Unwind pse_pi array and the notification fifo allocation when
  pse_controller_register() fails after of_load_pse_pis(), instead of
  leaking them on probe failure.
- Return -EPROBE_DEFER from pse_register_pw_ds() when a PI's supply is
  described but has not registered yet.
- Fix the pse_power_domain use-after-free on unbind when multiple
  drivers share a supply and the first one unbinds.
- Stop the notification worker before releasing the PI array in
  pse_controller_unregister().
- Fix the try_module_get reference leak
- Fix the deadlock in the pse_core that can happen when PI enable
  races with map_event.
- Fix the possible double budget allocation in the PSE core.


Thanks for your credits, it's appreciated.

Cheers,
Kyle

pw-bot: cr

  reply	other threads:[~2026-10-05  6:00 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-27 21:29 [PATCH net-next v3 0/2] " Kyle Swenson
2026-09-27 21:29 ` [PATCH net-next v3 1/2] dt-bindings: net: pse-pd: Add bindings for LTC4266 PSE Controller Kyle Swenson
2026-09-30 21:31   ` netdev-bot+sashiko
2026-09-27 21:29 ` [PATCH net-next v3 2/2] net: pse-pd: Add LTC4266 PSE controller driver Kyle Swenson
2026-09-30 21:31   ` netdev-bot+sashiko
2026-10-05  6:00     ` Kyle Swenson [this message]
2026-10-05  9:42       ` Kory Maincent

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=asM8rcKC1n1Tn4JD@est-xps15 \
    --to=kyle.swenson@est.tech \
    --cc=andrew+netdev@lunn.ch \
    --cc=conor+dt@kernel.org \
    --cc=davem@davemloft.net \
    --cc=david.nystrom@est.tech \
    --cc=devicetree@vger.kernel.org \
    --cc=edumazet@google.com \
    --cc=kory.maincent@bootlin.com \
    --cc=krzk+dt@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev-bot+sashiko@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=o.rempel@pengutronix.de \
    --cc=pabeni@redhat.com \
    --cc=robh@kernel.org \
    --cc=roland.kovacs@est.tech \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®