[PATCH net-next v7 2/5] net: pse-pd: fire lifecycle events on controller register/unregister

[Carlo Szelinsky]

From: Corey Leavitt <corey@xxxxxxxxxxxx>

Hook the newly-introduced pse_controller_notifier chain so that
pse_controller_register() fires PSE_REGISTERED after the controller
has been added to pse_controller_list (i.e. is now resolvable by
of_pse_control_get()), and pse_controller_unregister() fires
PSE_UNREGISTERED once it has been taken back off, while pcdev and
everything a subscriber's pse_control points at are still valid to
dereference.

No subscriber exists yet, so the event itself does nothing; a later
change wires the phy subsystem in as the first one. The reordering below
is not a no-op, though - it closes a teardown race that is reachable
today, with no subscriber involved.

Unregistration is reordered around that event, because a subscriber runs
arbitrary teardown inside it.

The controller is unlinked first. of_pse_control_get() walks
pse_controller_list and dereferences pcdev->pi[] through
of_pse_match_pi(), so a lookup racing the teardown could otherwise reach
an array that pse_release_pis() has already freed. Subscribers are handed
pcdev as the event data and do not need it on the list, so taking it off
first costs nothing and closes that race for every caller, not just the
ones this series adds.

disable_irq() moves up for the same reason: pse_isr() queues
notifications and reaches pcdev->pi, and nothing after it re-enables the
interrupt.

cancel_work_sync() moves up, above the frees, but stays below the event.
A subscriber dropping the last pse_control reference reaches
__pse_control_release(), which calls regulator_disable() if the PI is
still on. With the static budget strategy that retries any port on the
same power domain waiting for power, and if the domain is still over
budget it sheds a lower priority port through pse_disable_pi_pol(),
which queues a notification and calls schedule_work(). Draining the
worker before the walk would therefore leave work queued behind it,
racing the kfifo_free() below.

That retry does more than queue work: _pse_pi_delivery_power_sw_pw_ctrl()
calls ops->pi_enable(), so a port on the controller being torn down can
be energised from inside the event. The driver is still bound at that
point - the walk runs before pse_release_pis() and before devres
unwinds the PI regulators - so the call is legal, but it is worth
naming rather than leaving to be discovered.

Draining after the walk also keeps the worker's own transient
pse_control reference - taken by pse_control_find_by_id() - from becoming
the last one after pse_release_pis() has freed the array.

The frees move the other way. pse_flush_pw_ds() and pse_release_pis()
are the first two statements today, ahead of disable_irq() and
cancel_work_sync(); they end up last here. That ordering is what the
race fix consists of: until now pse_isr() and the worker could both
reach pcdev->pi[] after pse_release_pis() had freed it. They also have
to stay below the event, because the release path it runs reads
pcdev->pi[] and pi->pw_d->supply.

The series at

  https://lore.kernel.org/netdev/20260813200653.980170-1-github@xxxxxxxxxxxx/

makes a related reordering for net, independently of any subscriber, and
the two will conflict when it back-merges. The order there is not
identical: it leaves the unlink below cancel_work_sync() and
pse_flush_pw_ds(), having no event to place. The merged function wants
the order here, which contains that fix.

Signed-off-by: Corey Leavitt <corey@xxxxxxxxxxxx>
Signed-off-by: Carlo Szelinsky <github@xxxxxxxxxxxx>
Tested-by: Jonas Jelonek <jelonek.jonas@xxxxxxxxx>
---
 drivers/net/pse-pd/pse_core.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 84c734ed4553..56cecf60c5c4 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -1138,6 +1138,9 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	list_add(&pcdev->list, &pse_controller_list);
 	mutex_unlock(&pse_list_mutex);
 
+	blocking_notifier_call_chain(&pse_controller_notifier,
+				     PSE_REGISTERED, pcdev);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
@@ -1148,15 +1151,36 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
-	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
+	/* Stop the interrupt first: pse_isr() queues notifications and
+	 * reaches pcdev->pi, and nothing below re-enables it.
+	 */
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
-	cancel_work_sync(&pcdev->ntf_work);
-	kfifo_free(&pcdev->ntf_fifo);
+
+	/* Unlink before the event: of_pse_control_get() walks
+	 * pse_controller_list and dereferences pcdev->pi[] through
+	 * of_pse_match_pi(), so no lookup may still reach this controller
+	 * once its teardown starts. Subscribers are handed pcdev as the
+	 * event data, so the notifier does not need it on the list.
+	 */
 	mutex_lock(&pse_list_mutex);
 	list_del(&pcdev->list);
 	mutex_unlock(&pse_list_mutex);
+
+	blocking_notifier_call_chain(&pse_controller_notifier,
+				     PSE_UNREGISTERED, pcdev);
+
+	/* After the event, not before. A subscriber dropping the last
+	 * pse_control reference reaches __pse_control_release() ->
+	 * regulator_disable() -> _pse_pi_disable(), which can end up in
+	 * pse_disable_pi_pol() and queue a notification of its own, so a
+	 * cancel_work_sync() placed above the walk would not stay drained.
+	 */
+	cancel_work_sync(&pcdev->ntf_work);
+
+	pse_flush_pw_ds(pcdev);
+	pse_release_pis(pcdev);
+	kfifo_free(&pcdev->ntf_fifo);
 }
 EXPORT_SYMBOL_GPL(pse_controller_unregister);
 
-- 
2.43.0