Re: [PATCH net-next v5 2/2] net: pse-pd: add LED trigger support via notification path

[Carlo Szelinsky <github@szelinsky.de>]

Hi Jakub,

Thanks for the second pass! I went through all four points and
would love to clarify some points before moving on to v6. 
Replying inline again:

On Mon, 4 May 2026 18:57:59 -0700 Jakub Kicinski wrote:
> > +	pcdev->pi_led_trigs = devm_kcalloc(dev, pcdev->nr_lines,
> > +					   sizeof(*pcdev->pi_led_trigs),
> > +					   GFP_KERNEL);
>
> Since devm resources are released in strict LIFO order, and pi_led_trigs
> is allocated here after the regulators are registered in
> pse_controller_register(), will the pi_led_trigs array be freed before
> the regulators are unregistered on driver unbind?
>
> When the regulator core later unregisters the regulators and flushes
> pending disables, could it invoke pse_pi_disable() and call pse_led_update(),
> dereferencing the dangling pcdev->pi_led_trigs pointer?

Yes, seems to be real: devm LIFO frees pi_led_trigs before the 
regulators get unregistered. If a deferred disable fires during
regulator_unregister() (via flush_work on rdev->disable_work),
pse_pi_disable() runs and pse_led_update() walks freed memory.

But for one piece I got a question: the same path also touches
pcdev->pi. pse_pi_disable() calls _pse_pi_disable(), before:
pse_led_update(), and _pse_pi_disable() derefs pcdev->pi[id]
(via pse_pi_deallocate_pw_budget(&pcdev->pi[id]) and
pcdev->pi[id].pw_d). pcdev->pi is already freed by
pse_release_pis() in pse_controller_unregister(). So setting
pi_led_trigs NULL only fixes the LED half, or?

Should v6 also handle pcdev->pi here (NULL after
kfree, plus a guard in _pse_pi_disable)? Feels like the same
class of bug as 1/2's reorder, just on the regulator-cleanup
path. Or is a separate fix outside this patch series better?

> > +	for (i = 0; i < pcdev->nr_lines; i++) {
> > +		struct pse_pi_led_triggers *trigs = &pcdev->pi_led_trigs[i];
> [...]
> > +		trigs->delivering.name = devm_kasprintf(dev, GFP_KERNEL,
> > +							"pse-%s:port%d:delivering",
> > +							dev_id, i);
> [...]
> > +		ret = devm_led_trigger_register(dev, &trigs->delivering);
>
> Regulators are exposed before this code runs in pse_controller_register().
> What happens if a consumer calls regulator_enable() concurrently?
> [...] Since devm_led_trigger_register() hasn't initialized the trigger's
> internal led_cdevs list head yet, will led_trigger_event() attempt to
> iterate a zeroed memory list and cause a NULL pointer dereference?

Right, but the window is microseconds wide (between setting
trigs->delivering.name and devm_led_trigger_register() returning).
You'd need a consumer racing during probe to hit it, right?

The fix seems to be simple: move pse_led_triggers_register() before the
regulator loop in pse_controller_register(). of_load_pse_pis()
runs earlier so pi[]/pi[i].np is already filled in, which is
all the trigger loop needs. Regulators only get exposed after
triggers are fully registered, no race window left.

Question: Do you agree with that solution for v6?

> > +	ret = pse_led_triggers_register(pcdev);
> > +	if (ret) {
> > +		/* LED triggers are non-essential for power delivery; warn
> > +		 * and continue. [...]
> > +		dev_warn(pcdev->dev, "Failed to register LED triggers: %d\n",
> > +			 ret);
> > +		pcdev->pi_led_trigs = NULL;
> > +	}
>
> If pse_led_triggers_register() fails halfway through, the device probe
> still succeeds, which means devm cleanup will not run. Could the
> successfully registered LED triggers from earlier loop iterations remain
> registered indefinitely with the LED subsystem?

Would love to clarify this one with you as well: devm cleanup does still run
on driver unbind, since devm_led_trigger_register() attaches a
per-call release action. So the partially-registered triggers
stay in sysfs until unbind, but they're not leaked across it.
And with pi_led_trigs = NULL, pse_led_update() short-circuits
so the orphans never fire either. So practically they're inert
sysfs entries until unbind, not a leak.

That said, I agree the "warn and orphan" middle ground is a bit
weird. So which path should I follow:

1) treat LED reg failure as fatal: return ret, fail probe.
Smallest change, matches the rest of the function. The
"non-essential" framing was mine, happy to drop it.
2) wrap the registration in a devres group so partial
failures roll back the triggers we did manage to register.
3) leave as-is, since devm already cleans up on unbind.

I'd go with 1) since OOM during probe is fatal for most things
anyway, but happy to do 2) or 3) if you have a preference.

> > +		/* Update LEDs for described PIs regardless of consumer state.
> [...]
> > +		if (pcdev->no_of_pse_pi || pcdev->pi[i].np)
> > +			pse_led_update(pcdev, i);
>
> The docstring for pse_led_update() requires it to be called with pcdev->lock
> held. Does calling it here locklessly inside the event handler violate
> that locking contract?

From what I see, both callers of pse_handle_events() hold pcdev->lock
across the call:

* pse_isr() takes mutex_lock(&pcdev->lock) at line 1524, then
  calls pse_handle_events() at line 1531.
* pse_poll_worker() takes mutex_lock(&pcdev->lock) at line 1547,
  then calls pse_handle_events() at line 1551.

So pse_led_update() is running with the lock held in both paths.

Tbh I don't get completely what the AI is flagging here, or is
this a false positive? If false positive, I'd still add
lockdep_assert_held(&pcdev->lock) in pse_led_update() and
pse_handle_events() to make the contract explicit and catch any
future caller that forgets, but that would be documentation,
not a bug fix.

So plan for v6 [2/2], pending your answers:
* NULL pcdev->pi_led_trigs in pse_controller_unregister()
  (and possibly NULL pcdev->pi too, depending on your answer?)
* move pse_led_triggers_register() before the regulator loop
* add lockdep_assert_held() (assuming is a false positive)
* whichever option you pick for question 3?

Depending on what we do with [1/2], I'll roll v6 with your
decisions baked in.

Sorry for the long text...

Cheers,
Carlo