net: pse-pd: decouple controller lookup from MDIO probe

This is v7 of Corey's series [1]. It takes the PSE controller lookup out of the MDIO probe path, so a modular PSE controller driver no longer makes the PHY (and any DSA switch behind it) spin on -EPROBE_DEFER until the PSE module loads.

v6 [7] drew a large AI review [8], which I have now answered point by point in that thread. Paolo's reading was right: most of the [High] findings described the state of the tree between the old patches 3 and 5, which the old patch 5 then fixed. Rather than argue that in five changelogs, v7 folds those three patches into one, so the rtnl detour and the deferred release never exist at any commit. That also removes a real bisect hazard: the old patch 3 on its own hung lantiq_etop and sni_ave on probe, and the old patch 4 was what repaired it.

Per Documentation/process/maintainer-netdev.rst I ran LLM review over v7 before posting, more than once. Patches 3 and 4 exist because of what it found, and it changed patch 2 and the phy patch as well:

Patch 2 reorders pse_controller_unregister() around the new event, because a subscriber runs arbitrary teardown inside it. The controller is unlinked from pse_controller_list first, so a lookup racing the teardown resolves nothing rather than a controller whose pcdev->pi[] pse_release_pis() is about to free. v6 disclosed that as pre-existing and reachable only from phy registration; it is reachable from more than that here, because after the last patch a second controller registering runs a PSE_REGISTERED walk that calls of_pse_control_get() for every phy on mdio_bus_type, which is exactly the two-controller board I test on. disable_irq() moves up for the same reason: pse_isr() queues notifications and reaches pcdev->pi.

cancel_work_sync() moves the other way, to after the event rather than before it. 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 leave work queued behind it, racing the kfifo_free() below. Draining after it also stops the worker's own transient reference, taken by pse_control_find_by_id(), from becoming the last one once pse_release_pis() has freed the array.

[9] makes a related reordering for net, independently of any subscriber, so pse_controller_unregister() will conflict when [9] back-merges. The order is not identical: [9] leaves the unlink below cancel_work_sync() and pse_flush_pw_ds(), which it can, having no event to place. Here the event has to sit after the unlink and before the frees, and cancel_work_sync() after the event, so the unlink moves to the top. The merged function wants this order, which contains [9]'s fix:

    if (pcdev->irq)
            disable_irq(pcdev->irq);
    mutex_lock(&pse_list_mutex);
    list_del(&pcdev->list);
    mutex_unlock(&pse_list_mutex);
    blocking_notifier_call_chain(&pse_controller_notifier,
                                 PSE_UNREGISTERED, pcdev);
    cancel_work_sync(&pcdev->ntf_work);
    pse_flush_pw_ds(pcdev);
    pse_release_pis(pcdev);
    kfifo_free(&pcdev->ntf_fifo);

I am happy to send that as a follow-up on top of the merge if that is easier than carrying it in the conflict.

Kory, a specific ask on patch 2. The reason cancel_work_sync() sits below the event and not above it is that __pse_control_release() can re-enter your budget code: regulator_disable() on a PI that is still on runs _pse_pi_disable(), and with the static strategy that retries a pending port on the same power domain and, if the domain is still over budget, sheds a lower priority one through pse_disable_pi_pol() - which queues a notification and calls schedule_work() from inside the walk. I have tested that path rather than only reasoned about it, but the ordering rests on your design, so I would rather you looked at it than have it ride in unremarked.

Patch 4: each PSE PI regulator is registered with a "vpwr" supply. The regulator core deliberately treats an unresolved supply at registration as non-fatal, so pse_controller_register() completes and PSE_REGISTERED fires for a controller whose PIs cannot be handed out yet: regulator_get_exclusive() in pse_control_get_internal() resolves the supply itself and keeps returning -EPROBE_DEFER until the vpwr provider appears. Before this series the MDIO layer propagated that and deferred probe retried it. After it, phylib has no event left to retry on, and the port would silently lose PSE for good. Patch 4 checks every PI's supply before registering anything, so the PSE driver's own probe defers and deferred probe handles the ordering. It checks exactly the PIs the registration loop creates a regulator for, including a controller with no pse-pis node, and it follows both stages the core uses - the PI node, then the controller device - because a vpwr-supply written once on the controller node is invisible from the PI node but resolves at stage two. It stops short of the core's device_is_bound() gate, so a probe interleaving with the provider's own can still resolve late; the changelog says so.

Patch 3: that makes -EPROBE_DEFER an ordinary return from pse_controller_register(), which has no error unwind at all. The kfifo and the PI array plus its OF references are leaked on every failure, once today and on each retry after patch 4, and a partial pse_register_pw_ds() leaves devm-allocated power domains in the global xarray for the next registration to trip over. Patch 3 adds the unwind, at two depths: pse_pi_ops index pcdev->pi[], and the PI regulators are devm-registered, so once one exists the array cannot be freed here at all and stays leaked as it is today. It is released on the failures that happen while it exists and before the first PI regulator does - setup_pi_matrix() and the supply check - which is where the ordinary deferral now lands.

The same reviews caught two things in the phy patch. The error paths of phy_device_register() could leak a handle: device_add() puts the phy on the klist before its own later failure points, so a PSE_REGISTERED walk can attach one that nothing releases. A put at the out: label does not work, because by then device_add() has unwound the phy off the bus and the PSE_UNREGISTERED walk would miss it too. phydev->psec_detached now covers registration as well as removal, so no handle is attached in that window. And that flag is a plain bool rather than another bit in the flags word, since it is written under pse_phy_lock() while its neighbours are written under phydev->lock and rtnl.

No Fixes: tag. 5e82147de1cb ("net: mdiobus: search for PSE nodes by parsing PHY nodes") is the commit to blame, but this is a refactor across two subsystems plus a new export, and tagging it would invite a stable backport of all that to cure a probe-retry loop.

Two changelog errors from v6 are also fixed: netsec does not deadlock (its MDIO bus comes up in probe, not from ndo_init), and the module-unload rationale was backwards (try_module_get() pins the provider, so rmmod is refused before the unregister path ever runs).

How it works: pse_core gets a notifier chain (REGISTERED / UNREGISTERED). phylib subscribes, owns phydev->psec, and attaches the handle when the controller shows up instead of during probe. fwnode_mdio loses its PSE awareness, so no -EPROBE_DEFER leaves it and the probe-retry loop is gone.

On the tags: Jonas tested the v4 shape and Aleksander tested the v6 locking, which is unchanged here. Neither tested the fixes above. On the folded phy patch the code they exercised is intact, so I have kept their tags there. Jonas's tag also rides on patch 2, and that one did change in v7 - pse_controller_unregister() is reordered around the event - so it is the weakest of the three. Patch 1 only gained a kernel-doc correction. I would rather say so here than let it pass silently; happy to drop any of them if either would prefer.

Tested on a Realtek rtl9303 PoE switch with an HS104 PSE controller on i2c, with a PD drawing power on one port:

- clean boot, no probe-retry loop, the controller registers once
- rmmod is refused while a phy holds a handle
- i2c unbind: the notifier walk drops the handle and the port powers down, and ethtool reports no PSE attached
- i2c bind: the handle comes back and the PD is powered again
- six unbind/bind cycles, no warning, power domain index stable

Also exercised under QEMU, on arm64 under KASAN, PROVE_LOCKING and kmemleak. The device tree has a PSE controller, a second one whose vpwr provider never appears, and an MDIO bus with two phys, only one of which references a PI. Unbinding the controller detaches that phy's handle and rebinding re-attaches it; the other phy is never touched; unbinding the MDIO bus releases a live handle; and the controller with the missing supply defers instead of registering. A third controller puts its vpwr-supply on the controller node rather than the PI nodes, which the core resolves one stage later - that one has to defer too, and on the code before patch 4's second stage it registers instead. The new WARN_ON in patch 5 stays silent across six unbind cycles and kmemleak reports nothing.

The same test setup stages an over-budget static-priority domain, so that dropping the last reference really does reach pse_disable_pi_pol() and schedule_work() from inside the walk - the case patch 2's cancel_work_sync() placement exists for. I checked that with a dump_stack() rather than by reasoning about it: releasing phy1's handle in the walk lands in _pse_pi_disable(), the retry picks a pending port on the same domain, the domain is short, and a lower priority port is shed. No lockdep splat, which is the result I wanted most: that path re-enters the regulator core from a notifier callback, under the chain's rwsem and pse_phy_mutex.

Build matrix, all linking a real vmlinux: PHYLIB=y, PHYLIB=m (the config that failed to link in v5), PHYLIB=n, PSE_CONTROLLER=n, and CONFIG_OF=n.

Changes in v7:
- Patch 2: reorder pse_controller_unregister() around the event - unlink and disable_irq() before it, cancel_work_sync() after it, the frees last. The pse_control_head WARN_ON goes in patch 5 instead, with the walk that empties the list - in patch 2 an unbind would trip it, since the fwnode_mdio hook still hands out handles nothing releases.
- New patch 3: unwind the kfifo, the PI array and the power domains when controller registration fails.
- New patch 4: check every PI vpwr supply before registering the controller, so a consumer never meets a registered controller that can only answer -EPROBE_DEFER.
- Fold old patches 4 and 5 into the phy patch, so no intermediate commit carries the rtnl recursion or the deferred release.
- Hold phydev->psec_detached across registration too, make it a bool rather than a bitfield, and drop the unsafe release from phy_device_register()'s error path.
- Drop netsec from the deadlock list; fix the module-unload rationale; document that a transient attach error is no longer retried by deferred probe.
- Include <linux/notifier.h> in phy_device.c.
- Rebased on net-next.

Changes in v6:
- Fix a v5 build regression: the mutex moved into pse_core, since net/ethtool is always in vmlinux while PHYLIB is tristate.
- Fold phy_device_register_locked() back into phy_device_register().

Changes in v5:
- Replace rtnl with a dedicated mutex in the PSE attach path.
- Put phydev->psec back in phy_device_remove().

Changes in v4:
- Add Tested-by from Jonas Jelonek. No code changes.

Changes in v3:
- Drop patch 1 (regulator handle fix); it went to net separately [2].

v1 was an RFC by Corey [3].

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
[4] https://lore.kernel.org/netdev/[email protected]/
[5] https://lore.kernel.org/netdev/[email protected]/
[6] https://lore.kernel.org/netdev/[email protected]/
[7] https://lore.kernel.org/netdev/[email protected]/
[8] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
[9] https://lore.kernel.org/netdev/[email protected]/

Carlo Szelinsky (2):
net: pse-pd: unwind allocations when controller registration fails
net: pse-pd: check the PI vpwr supply before registering the
controller

Corey Leavitt (3):
net: pse-pd: add notifier chain for controller lifecycle events
net: pse-pd: fire lifecycle events on controller register/unregister
net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio
hook

 drivers/net/mdio/fwnode_mdio.c |  34 -----
 drivers/net/phy/phy_device.c   | 139 ++++++++++++++++++-
 drivers/net/pse-pd/pse_core.c  | 243 +++++++++++++++++++++++++++++++--
 include/linux/phy.h            |   7 +
 include/linux/pse-pd/pse.h     |  65 +++++++++
 net/ethtool/pse-pd.c           |  16 ++-
 6 files changed, 452 insertions(+), 52 deletions(-)

base-commit: e3bfd25626b44b6fa61a13c17178922171d519ce

2.43.0

From: Corey Leavitt <[email protected]>

Introduce a blocking notifier chain that allows other subsystems to be informed when a PSE controller is registered or unregistered, and provide pse_register_notifier() / pse_unregister_notifier() as the subscriber interface.

Subsequent patches will use this to let the phy subsystem own the phydev->psec lifecycle directly, decoupling PSE lookup from fwnode_mdiobus_register_phy() and removing the probe-time -EPROBE_DEFER coupling that currently exists between mdio, phy and pse-pd when the PSE controller driver is modular.

A blocking chain (rather than atomic) is used because callbacks sleep. The phy subscriber added later takes a mutex, and of_pse_control_get() reaches the controller over i2c. The contract also lets a subscriber take rtnl, which is why a controller driver must not hold it across pse_controller_register() or pse_controller_unregister().

The enum pse_controller_event is placed outside the IS_ENABLED(CONFIG_PSE_CONTROLLER) guard so that subscribers compiled into a kernel without PSE support can still reference the event values in dead-code paths without breaking the build.

This patch is pure infrastructure: nothing fires events yet, and nothing subscribes. No observable behavior change.

---
 drivers/net/pse-pd/pse_core.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pse-pd/pse.h    | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

index a5e6d7b26b9f..84c734ed4553 100644
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/ethtool.h>
 #include <linux/ethtool_netlink.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>

@@ -23,6 +24,39 @@ static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
 static DEFINE_MUTEX(pse_pw_d_mutex);
 
+static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
+
+/**
+ * pse_register_notifier - register a callback for PSE controller events
+ * @nb: notifier block to register
+ *
+ * See enum pse_controller_event for events fired and their subscriber
+ * contract. Callbacks run in process context; they may sleep, take
+ * rtnl, and call of_pse_control_get(). The chain fires synchronously,
+ * so a PSE controller driver's probe/unbind path must not hold any
+ * such lock when calling pse_controller_register() or
+ * pse_controller_unregister().
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_register_notifier);
+
+/**
+ * pse_unregister_notifier - unregister a previously registered callback
+ * @nb: notifier block previously passed to pse_register_notifier()
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_unregister_notifier);
+
 /**
  * struct pse_control - a PSE control
  * @pcdev: a pointer to the PSE controller device

index 4e5696cfade7..bc5d36bcd993 100644
@@ -21,6 +21,7 @@ struct net_device;
 struct phy_device;
 struct pse_controller_dev;
 struct netlink_ext_ack;
+struct notifier_block;
 
 /* C33 PSE extended state and substate. */
 struct ethtool_c33_pse_ext_state_info {

@@ -337,6 +338,25 @@ enum pse_budget_eval_strategies {
 	PSE_BUDGET_EVAL_STRAT_DYNAMIC	= 1 << 2,
 };
 
+/**
+ * enum pse_controller_event - PSE controller lifecycle events
+ *
+ * Event data in callbacks is always a pointer to the struct
+ * pse_controller_dev firing the event.
+ *
+ * @PSE_REGISTERED: controller added to pse_controller_list and
+ *	resolvable by of_pse_control_get().
+ * @PSE_UNREGISTERED: controller already taken off pse_controller_list, so
+ *	no longer resolvable, but still valid to dereference for the
+ *	duration of the callback. Subscribers holding pse_control
+ *	references targeting it must drop them before returning and must
+ *	not acquire new references for it.
+ */
+enum pse_controller_event {
+	PSE_REGISTERED,
+	PSE_UNREGISTERED,
+};
+
 #if IS_ENABLED(CONFIG_PSE_CONTROLLER)
 int pse_controller_register(struct pse_controller_dev *pcdev);
 void pse_controller_unregister(struct pse_controller_dev *pcdev);

@@ -366,6 +386,9 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
 
+int pse_register_notifier(struct notifier_block *nb);
+int pse_unregister_notifier(struct notifier_block *nb);
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node,

@@ -416,6 +439,16 @@ static inline bool pse_has_c33(struct pse_control *psec)
 	return false;
 }
 
+static inline int pse_register_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int pse_unregister_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
 #endif
 
 #endif

2.43.0

From: Corey Leavitt <[email protected]>

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/[email protected]/

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.

---
 drivers/net/pse-pd/pse_core.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

index 84c734ed4553..56cecf60c5c4 100644
@@ -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

pse_controller_register() allocates the notification kfifo and, through of_load_pse_pis(), the PI array plus an OF reference per described PI. Every failure after that point simply returns: none of it is freed. kfifo_free() and pse_release_pis() only run from pse_controller_unregister(), which a failed registration never reaches, and devm_pse_controller_register() drops only its own devres cookie. pcdev->pi is a plain allocation, so nothing else will ever free it, and all five in-tree controller drivers keep pse_controller_dev inside their private data, whose last pointer goes away with the failed probe.

A partial pse_register_pw_ds() is worse than a leak. The power domains it already created are devm-allocated but live in the global pse_pw_d_map, so the failed probe frees them while that xarray still points at them, and the next controller to register walks into freed memory in regulator_is_equal().

Unwind at two depths, because the PI array cannot always be freed here. pse_pi_ops.is_enabled(), .enable() and .disable() all index pcdev->pi[], and the PI regulators are devm-registered on pcdev->dev, so from the first successful devm_pse_pi_regulator_register() until devres unwinds the failed probe there are live regulators whose ops would follow a freed pointer. regulator_late_cleanup() and the "state" class attribute both reach those ops. So the array is released on the failures that happen while it exists and before the first PI regulator does: setup_pi_matrix() and the supply check. A driver's setup_pi_matrix() may well have registered devm regulators of its own by the time it fails - pd692x0 registers its managers there - but those do not index pcdev->pi[], so they do not constrain this. Earlier than that there is nothing to release - pcdev->pi is still NULL, or of_load_pse_pis() has already freed it - and from the registration loop onwards the array has to stay, so those failures only free the kfifo and flush the power domains, leaving it leaked exactly as it is today rather than handing those regulators a dangling pointer. An allocation failure in the loop's first iteration leaks it too, where releasing would still have been safe, but the rule stays simple enough to read. Freeing it safely needs the NULL-and-guard treatment the pending net teardown fix adds to the regulator ops, which is not in net-next.

of_load_pse_pis() already releases the PI array on its own failures, so that path only needs the kfifo.

pcdev->pi is cleared at the release_pis label and deliberately not inside pse_release_pis() itself. The label is the one place the array is freed while no PI regulator exists. pse_controller_unregister() calls the same helper with every PI regulator still registered - the devres node for devm_pse_controller_register() is added after them, so it is released first - and pse_pi_is_enabled() indexes pcdev->pi[] unguarded behind the regulator "state" attribute. Clearing the pointer in the helper would turn that pre-existing read of freed memory into a NULL dereference.

---
 drivers/net/pse-pd/pse_core.c | 37 +++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

index 56cecf60c5c4..16d75b4babf3 100644
@@ -1092,17 +1092,18 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	    !pcdev->ops->pi_get_pw_status) {
 		dev_err(pcdev->dev,
 			"Mandatory status report callbacks are missing");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free_kfifo;
 	}
 
 	ret = of_load_pse_pis(pcdev);
 	if (ret)
-		return ret;
+		goto free_kfifo;
 
 	if (pcdev->ops->setup_pi_matrix) {
 		ret = pcdev->ops->setup_pi_matrix(pcdev);
 		if (ret)
-			return ret;
+			goto release_pis;
 	}
 
 	/* Each regulator name len is pcdev dev name + 7 char +

@@ -1110,7 +1111,12 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	 */
 	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
 
-	/* Register PI regulators */
+	/* Register PI regulators. Once one of these exists, pse_pi_ops index
+	 * pcdev->pi[] and nothing here can unregister it again, so the array
+	 * must outlive this function. Failures below therefore unwind to
+	 * free_kfifo and deliberately leak it, as they already do today,
+	 * rather than hand the live regulators a freed pointer.
+	 */
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		char *reg_name;
 

@@ -1119,20 +1125,22 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 			continue;
 
 		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
-		if (!reg_name)
-			return -ENOMEM;
+		if (!reg_name) {
+			ret = -ENOMEM;
+			goto free_kfifo;
+		}
 
 		snprintf(reg_name, reg_name_len, "pse-%s_pi%d",
 			 dev_name(pcdev->dev), i);
 
 		ret = devm_pse_pi_regulator_register(pcdev, reg_name, i);
 		if (ret)
-			return ret;
+			goto free_kfifo;
 	}
 
 	ret = pse_register_pw_ds(pcdev);
 	if (ret)
-		return ret;
+		goto flush_pw_ds;
 
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);

@@ -1142,6 +1150,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 				     PSE_REGISTERED, pcdev);
 
 	return 0;
+
+flush_pw_ds:
+	pse_flush_pw_ds(pcdev);
+	goto free_kfifo;
+
+release_pis:
+	pse_release_pis(pcdev);
+	pcdev->pi = NULL;
+
+free_kfifo:
+	kfifo_free(&pcdev->ntf_fifo);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
 

2.43.0

Each PSE PI regulator is registered with a "vpwr" supply, which the bindings expect the board to provide via vpwr-supply in the PI node. The regulator core deliberately treats an unresolved supply at registration time as non-fatal: it logs, registers a bus device so the supply can be picked up later, and lets registration succeed.

That leaves pse_controller_register() completing for a controller whose PIs cannot be handed out. regulator_get_exclusive() in pse_control_get_internal() resolves the supply itself and returns -EPROBE_DEFER until the provider appears, so of_pse_control_get() keeps failing for this PI even though the controller is registered and discoverable. A consumer that only retries on controller registration - as the phy layer does after the next patch - then never gets its PI.

Check every PI's supply first, before any regulator of ours is registered, so the error surfaces in the PSE driver's own probe and deferred probe retries it there. The check skips exactly the PIs the registration loop skips, so a controller with no pse-pis node - where every PI has a NULL np but a regulator is still created for it - is covered too, through the controller device rather than a PI node.

The check follows both stages regulator_resolve_supply() uses: the PI's own node, then the controller device. Both are needed. of_get_regulator() reads the node it is handed and otherwise walks that device's children, so a vpwr-supply written once on the controller node - covering every PI
- is invisible from the PI node, while the core finds it at stage two. Checking only the PI node would let exactly the case this patch exists to prevent slip through unreported.

It is still not a full replica. The core also falls back to a dummy under have_full_constraints(), which does not matter here because it bails on an explicit -EPROBE_DEFER first, and it defers when the provider's parent is not yet bound, which regulator_get_optional() does not check. So a PSE probe interleaving with the provider's own probe can still pass the check and resolve late. That window is narrower than the one being closed - a provider that has not registered at all - and closing it properly means asking the core about rdev->supply after registration, which the unwind constraints above do not allow.

Doing all the checks before the registration loop also keeps the common deferral on the unwind path that can release the PI array: a controller can describe its PIs on different providers - ti,tps23881.yaml puts pse-pi@0 on vpwr1 and pse-pi@1 on vpwr2 - so "one PI resolves, the next defers" is the ordinary case, and checking inside the registration loop would leave registered regulators behind on every retry.

This widens what a missing provider costs. Before, only the PI on that supply failed, and only when a consumer asked for it; now the controller itself defers, so a board whose PIs sit on different providers loses PSE on all of them until the last one appears. That is the normal contract for a regulator consumer, and the alternative - a controller that advertises PIs it cannot hand out - is the bug being fixed. A provider that never appears leaves the controller unbound rather than half working, which deferred probe reports in the usual way.

---
 drivers/net/pse-pd/pse_core.c | 72 +++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

index 16d75b4babf3..457eef5784f8 100644
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>
+#include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/rtnetlink.h>

@@ -860,6 +861,67 @@ static const struct regulator_ops pse_pi_ops = {
 	.set_current_limit = pse_pi_set_current_limit,
 };
 
+/* The regulator core treats an unresolved "vpwr" supply as non-fatal and
+ * retries it on its own later, which would leave this controller able to
+ * register while of_pse_control_get() still fails with -EPROBE_DEFER for
+ * this PI, and nothing to retry the consumer's attach. Check the supply
+ * up front so the PSE driver's own probe defers instead.
+ *
+ * of_regulator_get_optional() reports a PI with no vpwr-supply described
+ * as -ENODEV rather than falling back to the dummy regulator, and has a
+ * stub for CONFIG_OF=n. Those PIs keep their existing behaviour: the
+ * regulator core resolves them to the dummy when the PI regulator is
+ * registered.
+ */
+static int pse_pi_check_supply(struct pse_controller_dev *pcdev, int id)
+{
+	struct regulator *supply;
+	int ret;
+
+	/* Skip exactly the PIs the registration loop below skips, so every
+	 * regulator that will be created is covered.
+	 */
+	if (!pcdev->no_of_pse_pi && !pcdev->pi[id].np)
+		return 0;
+
+	/* Follow both stages regulator_resolve_supply() will use for the
+	 * regulator about to be registered: the PI's own node first, then
+	 * the controller device. Only -ENODEV moves on to the second stage;
+	 * a provider that has not registered yet gives -EPROBE_DEFER, which
+	 * is the case worth catching here.
+	 */
+	if (pcdev->pi[id].np) {
+		supply = of_regulator_get_optional(pcdev->dev,
+						   pcdev->pi[id].np, "vpwr");
+		if (!IS_ERR(supply)) {
+			regulator_put(supply);
+			return 0;
+		}
+
+		ret = PTR_ERR(supply);
+		if (ret != -ENODEV)
+			return dev_err_probe(pcdev->dev, ret,
+					     "PI %d: failed to get vpwr supply\n",
+					     id);
+	}
+
+	/* A vpwr-supply on the controller node covers every PI, and is
+	 * where a PI without a node of its own resolves too.
+	 */
+	supply = regulator_get_optional(pcdev->dev, "vpwr");
+	if (!IS_ERR(supply)) {
+		regulator_put(supply);
+		return 0;
+	}
+
+	ret = PTR_ERR(supply);
+	if (ret == -ENODEV)
+		return 0;
+
+	return dev_err_probe(pcdev->dev, ret,
+			     "PI %d: failed to get vpwr supply\n", id);
+}
+
 static int
 devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
 			       char *name, int id)

@@ -1111,6 +1173,16 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	 */
 	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
 
+	/* Check every PI supply before registering any regulator: a provider
+	 * that has not probed yet is the ordinary -EPROBE_DEFER case, and
+	 * unwinding it must not leave PI regulators behind.
+	 */
+	for (i = 0; i < pcdev->nr_lines; i++) {
+		ret = pse_pi_check_supply(pcdev, i);
+		if (ret)
+			goto release_pis;
+	}
+
 	/* Register PI regulators. Once one of these exists, pse_pi_ops index
 	 * pcdev->pi[] and nothing here can unregister it again, so the array
 	 * must outlive this function. Failures below therefore unwind to

2.43.0

From: Corey Leavitt <[email protected]>

fwnode_mdiobus_register_phy() resolved the pses phandle during PHY registration and stashed the result in phydev->psec. With a modular PSE controller driver that lookup returns -EPROBE_DEFER until the module is loaded, so the PHY - and any DSA switch behind it - keeps bouncing off deferred probe. On some boards that shows up as a boot-time probe-retry storm and PHYs that never register at all.

Transfer ownership of phydev->psec to phylib and drive it from the PSE controller lifecycle notifier instead:

- On PSE_REGISTERED: walk mdio_bus_type and attach phydev->psec for every registered phy whose psec is still NULL. This is the "phy was enumerated before the PSE controller loaded" case, the root cause of the retry storm.

- On PSE_UNREGISTERED: walk it again and release every phydev->psec that targets the departing controller, before pse_release_pis() frees pcdev->pi. Without this a phy still holding a reference would cause a use-after-free in __pse_control_release()'s pcdev->pi[psec->id] access.

- phy_device_register() attaches once for a controller that is already registered. The phydev->psec check in phy_try_attach_pse() makes the two paths idempotent.

fwnode_find_pse_control() and its call site go away, and fwnode_mdio drops the PSE header. No PSE-originated -EPROBE_DEFER leaves the MDIO layer any more, so the retry storm is gone.

The attach happens after device_add() has made the phy visible on mdio_bus_type, and takes no rtnl. Two reasons it must not:

- Binding a phy that itself provides an SFP cage reaches sfp_bus_add_upstream() via phy_probe() -> phy_setup_ports() -> phy_sfp_probe(), and that takes rtnl_lock(). Reported on RTL8214FC.

- Some drivers register their MDIO bus from ndo_init - lantiq_etop via ltq_etop_init(), sni_ave via ave_init() - which register_netdevice() already calls under rtnl, and mdiobus_scan() reaches phy_device_register() from there.

phydev->psec is therefore serialised by a dedicated mutex, taken through pse_phy_lock(). It lives in pse_core rather than phylib because net/ethtool is always built into vmlinux while PHYLIB is tristate, so a phylib export would be unresolved with CONFIG_PHYLIB=m or =n; PSE_CONTROLLER is bool, so pse_core is always reachable. The ethtool PSE paths take the same mutex, so the use-after-free that rtnl used to close stays closed. Lock order is rtnl -> pse_phy_mutex -> pse_list_mutex -> pcdev->lock.

phy_device_remove() releases phydev->psec synchronously, while the phy is still on the bus. A phy that has been device_del()'d but is still pinned - an attached netdev, or an SFP module phy waiting for phy_device_free() - is off the mdio_bus_type klist and so invisible to the PSE_UNREGISTERED walk, and a deferred release would then touch a pcdev->pi[] the controller has already freed. Releasing before device_del() means the walk and the release contend for the same mutex, so whichever runs second sees NULL.

phydev->psec_detached closes two windows around that, by telling phy_try_attach_pse() to skip the phy. It is a plain bool rather than another bit in the flags word: it is written under pse_phy_lock(), while neighbours such as suspended and sysfs_links are written under phydev->lock and rtnl, and a shared storage unit would make those read-modify-write updates race. bus_for_each_dev() still reaches a phy until bus_remove_device() takes it off the klist, which is well into device_del(), so a PSE_REGISTERED walk could otherwise attach a fresh handle just after the release, with nothing left to free it. The same applies while the phy is being registered: device_add() puts it on the bus before its own later failure points, and its unwind takes it back off, so a handle attached in that window would be missed by the PSE_UNREGISTERED walk as well. The flag is therefore held until registration has actually succeeded, and set again from phy_device_remove().

One note on behaviour. of_pse_control_get() reaches the hardware - pse_pi_is_hw_enabled() calls pi_get_admin_state(), an i2c transaction on tps23881, si3474, pd692x0 and realtek-pse-mcu - and that error used to propagate out of fwnode_mdiobus_register_phy() and get retried by deferred probe. Now a transient failure leaves the port without PSE until the controller registers again; it is reported with phydev_warn() rather than dropped silently. -EPROBE_DEFER stays silent because the notifier retries it at PSE_REGISTERED time, and the preceding patch makes sure an already registered controller cannot keep returning it.

The UNREGISTERED walk does not help rmmod either:
pse_control_get_internal() takes try_module_get(pcdev->owner) per handle, so while a phy holds one the unload is refused before the module exit path runs. What the walk covers is driver unbind and device removal, where pse_controller_unregister() runs with the module loaded.

What makes the UNREGISTERED walk safe is the teardown order the preceding patch establishes. By the time it runs the controller is already off pse_controller_list and the interrupt is off, so nothing new can reach the PI array; pcdev->pi and the power domains are still live, which the release path needs because dropping the last reference can disable a PI that is still energised; and the notification worker is drained after the walk rather than before it, both because that release path can queue work of its own and because the worker's own transient pse_control reference must not become the last one once the array has been freed.

Now that the walk empties pcdev->pse_control_head on unregister, warn if anything is still on it afterwards. Nothing can add one at that point:
the controller is off the list, the irq is off and the worker is drained. It is quiet because of_pse_control_get() has a single caller and the walk covers it, and it fires if a consumer that does not subscribe is ever added.

The warning belongs here rather than with the reordering it sits in, because until this patch the fwnode_mdio hook gives every matching phy a handle for its lifetime and nothing releases it on controller unbind - so an unbind between the two would trip it on any board where a phy resolved a PI.

---
 drivers/net/mdio/fwnode_mdio.c |  34 --------
 drivers/net/phy/phy_device.c   | 139 ++++++++++++++++++++++++++++++++-
 drivers/net/pse-pd/pse_core.c  |  68 ++++++++++++++++
 include/linux/phy.h            |   7 ++
 include/linux/pse-pd/pse.h     |  32 ++++++++
 net/ethtool/pse-pd.c           |  16 ++--
 6 files changed, 256 insertions(+), 40 deletions(-)

index ba7091518265..7bd979b59f49 100644
@@ -11,33 +11,11 @@
 #include <linux/fwnode_mdio.h>
 #include <linux/of.h>
 #include <linux/phy.h>
-#include <linux/pse-pd/pse.h>
 
 MODULE_AUTHOR("Calvin Johnson <[email protected]>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
 
-static struct pse_control *
-fwnode_find_pse_control(struct fwnode_handle *fwnode,
-			struct phy_device *phydev)
-{
-	struct pse_control *psec;
-	struct device_node *np;
-
-	if (!IS_ENABLED(CONFIG_PSE_CONTROLLER))
-		return NULL;
-
-	np = to_of_node(fwnode);
-	if (!np)
-		return NULL;
-
-	psec = of_pse_control_get(np, phydev);
-	if (PTR_ERR(psec) == -ENOENT)
-		return NULL;
-
-	return psec;
-}
-
 static struct mii_timestamper *
 fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
 {

@@ -118,7 +96,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 				struct fwnode_handle *child, u32 addr)
 {
 	struct mii_timestamper *mii_ts = NULL;
-	struct pse_control *psec = NULL;
 	struct phy_device *phy;
 	bool is_c45;
 	u32 phy_id;

@@ -159,14 +136,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 			goto clean_phy;
 	}
 
-	psec = fwnode_find_pse_control(child, phy);
-	if (IS_ERR(psec)) {
-		rc = PTR_ERR(psec);
-		goto unregister_phy;
-	}
-
-	phy->psec = psec;
-
 	/* phy->mii_ts may already be defined by the PHY driver. A
 	 * mii_timestamper probed via the device tree will still have
 	 * precedence.

@@ -176,9 +145,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 
 	return 0;
 
-unregister_phy:
-	if (is_acpi_node(child) || is_of_node(child))
-		phy_device_remove(phy);
 clean_phy:
 	phy_device_free(phy);
 clean_mii_ts:

index 7c5098a0dd6c..ef5b5e6f4e0f 100644
@@ -666,6 +666,12 @@ struct phy_oatc14_sqi_capability {
  * @master_slave_state: Current master/slave configuration
  * @mii_ts: Pointer to time stamper callbacks
  * @psec: Pointer to Power Sourcing Equipment control struct
+ * @psec_detached: Set while phylib will not accept a PSE handle for this
+ *	phy, either because registration has not completed or because it has
+ *	already released @psec, so the PSE_REGISTERED notifier walk skips it.
+ *	Not a bitfield: once the phy is on the bus it is written under
+ *	pse_phy_lock() while other flags are written under phydev->lock or
+ *	rtnl, and they must not share a storage unit
  * @ports: List of PHY ports structures
  * @n_ports: Number of ports currently attached to the PHY
  * @max_n_ports: Max number of ports this PHY can expose

@@ -807,6 +813,7 @@ struct phy_device {
 	struct net_device *attached_dev;
 	struct mii_timestamper *mii_ts;
 	struct pse_control *psec;
+	bool psec_detached;
 
 	struct list_head ports;
 	int n_ports;

index bc5d36bcd993..16181f8a2c97 100644
@@ -386,9 +386,23 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
 
+bool pse_control_matches_pcdev(struct pse_control *psec,
+			       struct pse_controller_dev *pcdev);
+
 int pse_register_notifier(struct notifier_block *nb);
 int pse_unregister_notifier(struct notifier_block *nb);
 
+void pse_phy_lock(void);
+void pse_phy_unlock(void);
+
+#ifdef CONFIG_LOCKDEP
+void pse_phy_lock_assert_held(void);
+#else
+static inline void pse_phy_lock_assert_held(void)
+{
+}
+#endif
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node,

@@ -439,6 +453,12 @@ static inline bool pse_has_c33(struct pse_control *psec)
 	return false;
 }
 
+static inline bool pse_control_matches_pcdev(struct pse_control *psec,
+					     struct pse_controller_dev *pcdev)
+{
+	return false;
+}
+
 static inline int pse_register_notifier(struct notifier_block *nb)
 {
 	return 0;

@@ -449,6 +469,18 @@ static inline int pse_unregister_notifier(struct notifier_block *nb)
 	return 0;
 }
 
+static inline void pse_phy_lock(void)
+{
+}
+
+static inline void pse_phy_unlock(void)
+{
+}
+
+static inline void pse_phy_lock_assert_held(void)
+{
+}
+
 #endif
 
 #endif

index 757c9e0cc856..654325946aaa 100644
@@ -71,7 +71,9 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
+	pse_phy_lock();
 	ret = pse_get_pse_attributes(phydev, info->extack, data);
+	pse_phy_unlock();
 
 	ethnl_ops_complete(dev);
 

@@ -281,9 +283,12 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
+
+	pse_phy_lock();
+
 	ret = ethnl_set_pse_validate(phydev, info);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (tb[ETHTOOL_A_PSE_PRIO]) {
 		unsigned int prio;

@@ -291,7 +296,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		prio = nla_get_u32(tb[ETHTOOL_A_PSE_PRIO]);
 		ret = pse_ethtool_set_prio(phydev->psec, info->extack, prio);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {

@@ -301,7 +306,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		ret = pse_ethtool_set_pw_limit(phydev->psec, info->extack,
 					       pw_limit);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	/* These values are already validated by the ethnl_pse_set_policy */

@@ -319,10 +324,11 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		 */
 		ret = pse_ethtool_set_config(phydev->psec, info->extack,
 					     &config);
-		if (ret)
-			return ret;
 	}
 
+out:
+	pse_phy_unlock();
+
 	/* Return errno or zero - PSE has no notification */
 	return ret;
 }

2.43.0

More feedback from clashiko. Low prio remarks, items addressed later in the series and ask for fixes tag could be ignored, but still a few things that look real there.

/P

Hey guys,
One process question first. I do run an LLM review before posting, as maintainer-netdev.rst asks. I ran the review prompts (masoncl) locally against a frontier model and still got a different set of findings than the bot reported here. If there is a better way to run it so the two line up, I would be glad to hear it - right now I cannot tell whether I am missing setup or just reading different output.

On the review itself: it turned up something in code I added in v7, so I am answering per patch below. v8 follows with the fixes.

Four findings I fixed in code, one I disagree with and say why, one is real but belongs in a net fix rather than here, five I have conceded in the changelogs, and five are pre-existing and want separate net patches rather than growing this series. Everything I claim to have checked, I checked against the tree rather than reasoning from the changelogs.

pw-bot: cr

patch 4, [High] sibling-PI supply lookup
----------------------------------------

Correct. of_get_regulator() first reads the vpwr-supply phandle from the node it is given. If there is none, it calls of_get_child_regulator(dev->of_node), which walks every node under that device. I passed pcdev->dev as the device and the PI node as the node. So for a PI with no supply of its own, the walk went over the whole controller and found a sibling PI's supply, and the check passed. The core does not do this in its first stage. There it looks at the PI node only.

v8 takes the first stage only once the PI's own phandle resolves, so the walk is never reached. I resolve the phandle instead of only testing that the property is there, so a broken phandle cannot slip into the walk either.

One part of the finding is too strong, and my first v8 comment had the same mistake. The core does reach that sibling walk, but in its second stage, through regulator_dev_lookup(pcdev->dev). So the gate does not stop the sibling walk. What it stops is a supply on the controller node being hidden by a sibling's.

v8 also treats -EPERM as resolved. _regulator_get_common() returns it when someone else holds the provider exclusively. The provider is still there, and regulator_resolve_supply() never checks this, so failing here would block a registration the core would have finished.

patch 4, [Medium] deferral after setup_pi_matrix()
--------------------------------------------------

The leak is real. pd692x0 claims a power budget in setup_pi_matrix(). It frees it only from that function's own error labels or from remove(), and neither runs when registration fails later.

But the suggested fix does not work. I tried it. microchip,pd692x0.yaml has

  pse-pi@0 { vpwr-supply = <&manager0>; };

and manager0's regulator is registered by pd692x0_register_manager_regulator(dev, name, manager[i].node), with rconfig.of_node set to that manager node. That call comes from pd692x0_setup_pi_matrix(). So checking the supply before setup_pi_matrix() asks for a regulator that only this driver can create. It returns -EPROBE_DEFER every time and pd692x0 never probes.

I added a controller of that shape to my test setup and ran it both ways. With the checks moved early it logs "PI 0: failed to get vpwr supply" and stays in deferred probe. With them where they are it registers. So the loop stays after setup_pi_matrix(), and the v8 changelog says what that placement does not clean up.

For pd692x0 the new deferral does not fire anyway: the supplies its PIs name are the manager regulators setup_pi_matrix() has just registered. A board that points a pd692x0 PI at an external provider would hit the leak, and that cleanup belongs in pd692x0.

patch 2, [High] worker re-queued after cancel_work_sync()
---------------------------------------------------------

Right, and the changelog said more than it should. At that commit every phy still gets its handle from fwnode_mdio, and only phy_device_remove() releases it. So an ethtool path can queue work after the drain. The tree behaves that way today and this patch does not make it worse. It closes once the phy patch releases those handles in the event and takes pse_phy_lock() on both sides. v8 says that instead of implying the race is already closed.

patch 2, [Low] disable_irq() and devres ordering
------------------------------------------------

Right. tps23881 is the only in-tree user of devm_pse_irq_helper(), and it requests the irq after devm_pse_controller_register(). devres unwinds in reverse, so free_irq() runs first and the handler is already stopped before this disable_irq(). The move matters for a driver that requests its irq earlier, instead of leaving the ordering to devres. Comment and changelog both reworded.

While there: devm_pse_irq_helper() sets pcdev->irq even when devm_request_threaded_irq() fails, so disable_irq() can be called on an irq that was never requested. Pre-existing, and on my list below.

patch 3, [High] flush_pw_ds() vs a concurrent controller
--------------------------------------------------------

Real. I am not fixing it here. Reason below.

kref_put_mutex() drops the count outside pse_pw_d_mutex. And pse_register_pw_ds() takes a new reference with a plain kref_get(), after an xa_for_each() under that mutex. So another controller on the same supply can take a reference while this unwind drops the last one. The count then goes 2->1, nothing is released, and devres frees the pw_d anyway, because that memory belongs to the controller that created it.

Closing that window does not fix the real problem. A power domain can be shared by two controllers, but its memory belongs to the one that created it, through devm. So any time a controller with a shared domain goes away, that memory is freed while the other controller still points at it. This is true with or without this patch.

The fix is to move the domain out of devm and use kref_get_unless_zero() in the lookup. That is a net fix. It does not belong in a patch that is only meant to stop the common failure leaking.

What this patch does fix happens every time today: a controller fails to register, leaves its domains in pse_pw_d_map, and the next controller reads them in regulator_is_equal(). v8 says which case is left open.

patch 3, [Medium] pi[i].pw_d left dangling
------------------------------------------

Fixed in v8. pse_flush_pw_ds() clears it now.

The domain is devm memory of whichever controller created it, so it can go as soon as that probe unwinds. But pse_pi_is_enabled() still reads pi[].pw_d through the regulator "state" attribute, and the PI regulators are only unregistered after us. So the pointer had to go.

Before relying on NULL there I checked every place that reads pi[].pw_d. They are all guarded, and pse_pw_d_is_sw_pw_control() returns false for NULL, so a read in that window now falls back to the hardware state instead of touching freed memory.

patch 3, [Medium] OF references on the paths that keep pi[]
-----------------------------------------------------------

Accurate, and my changelog invited that reading.

The OF references are taken per PI in of_load_pse_pis() and dropped only by pse_release_pis(). So they travel with the array: recovered wherever release_pis is reached, and leaked together with the array on the three paths that end at free_kfifo. Same trade as the array, and v8 now says so plainly instead of listing the references among the things it fixes.

One more thing in that changelog was simply wrong. I wrote that pse_release_pis() only runs from pse_controller_unregister(). of_load_pse_pis() calls it on its own error path too. Fixed.

patch 4/5, [Medium] -EPROBE_DEFER no longer retried
---------------------------------------------------

Conceded. v8 no longer says a PI "can resolve late" - that implied something recovers it, and nothing does. There is no retry until the next PSE_REGISTERED or a phy re-registration.

fw_devlink does not close this window for the form the bindings document. A vpwr-supply in a pse-pi node links the pse-pi node, not the controller, so the controller only gets a SYNC_STATE_ONLY proxy link from it, and device_links_check_suppliers() does not hold its probe for those. Only a vpwr-supply on the controller node delays the probe.

patch 5/5, [Medium] ports powered down when a PSE probe fails late
----------------------------------------------------------------

Real, and now documented.

The REGISTERED walk runs inside pse_controller_register(), so handles get attached part way through the PSE driver's probe. For a PI the hardware already has powered, pse_control_get_internal() records admin_state_enabled. If a later probe step then fails - tps23881 requests its irq after registration - devres runs the UNREGISTERED walk, the last reference goes, and __pse_control_release() disables the PI.

So a port that was up before the driver loaded gets powered down when that driver fails to finish probing. It is the same thing a clean unbind does, and the alternative is leaving a port owned by a controller that is going away.

patch 5/5, [Medium] fw_devlink leaves the phy on genphy
-----------------------------------------------------

"pses" is an enforcing supplier binding. So once the MDIO deferral goes, the phy registers while its own driver is still held in device_links_check_suppliers(). If a MAC attaches in that window, phy_attach_direct() falls back to genphy, and device_bind_driver() -> device_links_force_bind() binds it past the pending link. Nothing re-probes the phy later, so it stays on genphy.

v8 adds a drivers/of patch marking the link FWLINK_FLAG_IGNORE. It sits before the phy patch, so no commit carries the window. Rob, Saravana:
that one is yours.

It drops the device link completely rather than relaxing it - fw_devlink_create_devlink() returns early and the fwnode link is deleted at the consumer's device_add().

Two things do ride on that link, and I had the changelog wrong about this at first, so to be explicit: the link is managed. The fwnode link sits on the phy's own node, so fw_devlink_create_devlink() takes its "con->fwnode == link->consumer" branch and asks for fw_devlink_flags, which defaults to FW_DEVLINK_FLAGS_RPM; that has neither DL_FLAG_STATELESS nor DL_FLAG_SYNC_STATE_ONLY, so device_link_add() promotes it to DL_FLAG_MANAGED and device_links_unbind_consumers() acts on it. So unbinding the PSE controller releases the phy's driver today, and DL_FLAG_AUTOPROBE_CONSUMER probes the phy when the controller binds.

Both are given up on purpose: the notifier does the attach and detach now and does not tear the phy driver down, and with the deferral gone there is nothing for an autoprobe to wait on. DL_FLAG_PM_RUNTIME is the one that really had no effect, and there is no dpm or devices_kset ordering to lose.

That also makes patch 5 not a no-op on its own. The MDIO path registers the phy before it looks the PI up, so the link exists and goes active as soon as the controller binds - the cascade therefore goes away at patch 5 rather than at the phy patch. It also stops holding the phy's driver back during the MDIO retries, so with patch 5 alone the driver probes inside device_add() and is removed again on each retry until the controller binds; the phy patch removes the retries. The changelog says all of this now.

phy_try_attach_pse() comment
----------------------------

Separately: the comment still said that any error other than -ENOENT or -EPROBE_DEFER "means a broken binding". That is not true. -ENOMEM lands there, and so does -EPERM from regulator_get_exclusive() when the PI is already held exclusively - _regulator_get_common() returns that from its "if (rdev->exclusive)" test, before it ever looks at open_count. I agreed this was wrong in the v6 thread but only fixed the changelog back then. The comment is fixed in v8, and it now also says that -EPROBE_DEFER from a registered controller, while its vpwr provider is not yet bound, is not retried.

Pre-existing, and I would rather send these as separate net patches than grow this series
-----------------------------------------------------------------------

All five verified against the base this series sits on, e3bfd25626b4:

- pse_controller_register() calls kfifo_alloc() with pcdev->nr_lines before the 0 -> 1 fixup, and __kfifo_alloc_node() rejects a rounded size below 2. pse_regulator.c never sets nr_lines, so it stays 0 and podl-pse-regulator cannot register at all on net-next today. Any driver with one PI fails the same way, and realtek-pse-mcu-core.c can reach that since it accepts max_ports == 1 from the MCU. I ran into this building a one-PI test controller. Fixes: ffef61d6d273.

- devm_pse_irq_helper() sets pcdev->irq even when devm_request_threaded_irq() fails. pse_controller_unregister() then calls disable_irq() on an irq that was never requested.

- pse_control_get_internal() leaks a module reference: the !pi_get_admin_state and pse_pi_is_hw_enabled() < 0 paths goto free_psec, which sits below put_module. The retry walk makes it easier to hit.

- psec->attached_phydev is a raw pointer with no device reference and is never cleared. pse_send_ntf_worker() reaches it through pse_control_get_netdev(), which dereferences psec->attached_phydev->attached_dev under rtnl_lock() but not pse_phy_lock().

I had this wrong when I first looked at the two-phys-on-one-PI variant, so to correct myself: no second handle is created, but not because the second get is refused. pse_control_get_internal() walks pcdev->pse_control_head first and returns a matching psec with a kref_get(), before any regulator_get_exclusive() runs, so the handle is shared rather than rejected - and it keeps attached_phydev pointing at the first phy, which is exactly the case raised. It behaves identically before this series, since fwnode_mdiobus_register_phy() made the same call for each phy, so it is not a regression here, but it is reachable and the net fix should cover it.

- pse_release_pis() frees pcdev->pi while the PI regulators are still registered - devm_pse_controller_register() adds its devres node after them, so unregister runs first and the regulator "state" attribute can read freed memory. This one is already covered by my pending net series:
https://lore.kernel.org/netdev/[email protected]/

Thanks for the review. v8 follows this mail.

Carlo

Hey Carlo,

In my experience, review prompts are not enough. The limitations are the agent and actual model which are running it. I personally already run own sashiko instance backed with gemini API (the same variant as the public generic sashiko). Since running sashiko is expensive, i run multi agent claude setup [1], to stress pre-review patches before sashiko round.

Netdev is now running own sashiko-netdev, which is running a multimodel sashiko, which is even more expensive and finds even more things. Privately I can't afford this.

[1] https://claude.com/blog/building-multi-agent-systems-when-and-how-to-use-them

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |