[PATCH net-next v7 0/5] net: pse-pd: decouple controller lookup from MDIO probe

[PATCH net-next v7 0/5] net: pse-pd: decouple controller lookup from MDIO probe

[Carlo Szelinsky]

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.

Tested-by: Carlo Szelinsky <github@szelinsky.de>

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/20260620112440.1734404-1-github@szelinsky.de/
[2] https://lore.kernel.org/netdev/20260624204017.2752934-1-github@szelinsky.de/
[3] https://lore.kernel.org/netdev/20260423-pse-notifier-decouple-v1-0-86ed750a9d62@leavitt.info/
[4] https://lore.kernel.org/netdev/20260630091125.3162481-1-github@szelinsky.de/
[5] https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
[6] https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/
[7] https://lore.kernel.org/netdev/20260906153102.959217-1-github@szelinsky.de/
[8] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
[9] https://lore.kernel.org/netdev/20260813200653.980170-1-github@szelinsky.de/

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

[PATCH net-next v7 1/5] net: pse-pd: add notifier chain for controller lifecycle events

[Carlo Szelinsky]

From: Corey Leavitt <corey@leavitt.info>

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.

Signed-off-by: Corey Leavitt <corey@leavitt.info>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
Tested-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/net/pse-pd/pse_core.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pse-pd/pse.h    | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index a5e6d7b26b9f..84c734ed4553 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -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
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 4e5696cfade7..bc5d36bcd993 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -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

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

[Carlo Szelinsky]

From: Corey Leavitt <corey@leavitt.info>

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@szelinsky.de/

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@leavitt.info>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
Tested-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 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

[PATCH net-next v7 3/5] net: pse-pd: unwind allocations when controller registration fails

[Carlo Szelinsky]

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.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 37 +++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 56cecf60c5c4..16d75b4babf3 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -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

[PATCH net-next v7 4/5] net: pse-pd: check the PI vpwr supply before registering the controller

[Carlo Szelinsky]

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.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 72 +++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 16d75b4babf3..457eef5784f8 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -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

[PATCH net-next v7 5/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

[Carlo Szelinsky]

From: Corey Leavitt <corey@leavitt.info>

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.

Reported-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Closes: https://lore.kernel.org/netdev/e00048dd-1ed3-40c3-9912-59bccf015ad5@gmail.com/
Reported-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Closes: https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/
Signed-off-by: Corey Leavitt <corey@leavitt.info>
Co-developed-by: Carlo Szelinsky <github@szelinsky.de>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
Tested-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Tested-by: Aleksander Jan Bajkowski <olek2@wp.pl>
---
 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(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index ba7091518265..7bd979b59f49 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -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 <calvin.johnson@oss.nxp.com>");
 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:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5b13a74e2fa9..971d9326d226 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/netdevice.h>
+#include <linux/notifier.h>
 #include <linux/phy.h>
 #include <linux/phylib_stubs.h>
 #include <linux/phy_led_triggers.h>
@@ -1012,9 +1013,111 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 }
 EXPORT_SYMBOL(get_phy_device);
 
+/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
+ * Caller must hold pse_phy_lock(). A missing phandle (-ENOENT) or a
+ * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
+ * retries the latter at PSE_REGISTERED time. Any other error means a broken
+ * binding and is warned about, but left non-fatal so the phy still registers.
+ *
+ * A phy with psec_detached set is skipped: it is either not registered yet
+ * or on its way out, and nothing would release a handle attached now.
+ */
+static void phy_try_attach_pse(struct phy_device *phydev)
+{
+	struct pse_control *psec;
+	struct device_node *np;
+
+	pse_phy_lock_assert_held();
+
+	np = phydev->mdio.dev.of_node;
+	if (!np)
+		return;
+
+	if (phydev->psec || phydev->psec_detached)
+		return;
+
+	psec = of_pse_control_get(np, phydev);
+	if (IS_ERR(psec)) {
+		if (PTR_ERR(psec) != -EPROBE_DEFER && PTR_ERR(psec) != -ENOENT)
+			phydev_warn(phydev, "failed to get PSE control: %pe\n",
+				    psec);
+		return;
+	}
+
+	phydev->psec = psec;
+}
+
+static int phy_pse_attach_one(struct device *dev, void *data)
+{
+	pse_phy_lock_assert_held();
+
+	if (dev->type != &mdio_bus_phy_type)
+		return 0;
+
+	phy_try_attach_pse(to_phy_device(dev));
+	return 0;
+}
+
+static int phy_pse_detach_one(struct device *dev, void *data)
+{
+	struct pse_controller_dev *pcdev = data;
+	struct phy_device *phydev;
+	struct pse_control *psec;
+
+	pse_phy_lock_assert_held();
+
+	if (dev->type != &mdio_bus_phy_type)
+		return 0;
+
+	phydev = to_phy_device(dev);
+	psec = phydev->psec;
+	if (!psec || !pse_control_matches_pcdev(psec, pcdev))
+		return 0;
+
+	phydev->psec = NULL;
+	pse_control_put(psec);
+	return 0;
+}
+
+static int phy_pse_notifier_event(struct notifier_block *nb,
+				  unsigned long event, void *data)
+{
+	switch (event) {
+	case PSE_REGISTERED:
+		pse_phy_lock();
+		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
+				 phy_pse_attach_one);
+		pse_phy_unlock();
+		return NOTIFY_OK;
+	case PSE_UNREGISTERED:
+		pse_phy_lock();
+		bus_for_each_dev(&mdio_bus_type, NULL, data,
+				 phy_pse_detach_one);
+		pse_phy_unlock();
+		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
+	}
+}
+
+static struct notifier_block phy_pse_notifier __read_mostly = {
+	.notifier_call = phy_pse_notifier_event,
+};
+
 /**
  * phy_device_register - Register the phy device on the MDIO bus
  * @phydev: phy_device structure to be added to the MDIO bus
+ *
+ * phydev->psec is attached after device_add() has made the phy visible on
+ * mdio_bus_type, so that a concurrent PSE notifier walk and the attach can
+ * never leave the phy unattached. Neither step takes rtnl: keeping
+ * device_add() out of rtnl avoids deadlocking when binding a phy that itself
+ * provides an SFP cage (phy_probe() -> phy_sfp_probe() ->
+ * sfp_bus_add_upstream() takes rtnl), and pse_phy_lock() rather than rtnl
+ * guards the attach so a bus registered from ndo_init (which already holds
+ * rtnl) does not recurse on it.
+ *
+ * Return: 0 on success, negative error code on failure.
  */
 int phy_device_register(struct phy_device *phydev)
 {
@@ -1034,12 +1137,25 @@ int phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	/* Keep the PSE_REGISTERED walk off this phy until registration has
+	 * actually succeeded. device_add() puts the phy 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 and left with no owner.
+	 */
+	phydev->psec_detached = true;
+
 	err = device_add(&phydev->mdio.dev);
 	if (err) {
 		phydev_err(phydev, "failed to add\n");
 		goto out;
 	}
 
+	pse_phy_lock();
+	phydev->psec_detached = false;
+	phy_try_attach_pse(phydev);
+	pse_phy_unlock();
+
 	return 0;
 
  out:
@@ -1061,8 +1177,22 @@ EXPORT_SYMBOL(phy_device_register);
  */
 void phy_device_remove(struct phy_device *phydev)
 {
+	struct pse_control *psec;
+
 	unregister_mii_timestamper(phydev->mii_ts);
-	pse_control_put(phydev->psec);
+
+	/* Detach synchronously, before the phy leaves the bus, so the put cannot
+	 * outlive the PSE controller: an off-bus but still-pinned phy is missed
+	 * by the PSE_UNREGISTERED walk. pse_phy_lock() serialises against that
+	 * walk, and psec_detached keeps the PSE_REGISTERED walk from attaching a
+	 * new handle in the window before device_del() takes the phy off the bus.
+	 */
+	pse_phy_lock();
+	psec = phydev->psec;
+	phydev->psec = NULL;
+	phydev->psec_detached = true;
+	pse_control_put(psec);
+	pse_phy_unlock();
 
 	device_del(&phydev->mdio.dev);
 
@@ -3976,8 +4106,14 @@ static int __init phy_init(void)
 	if (rc)
 		goto err_c45;
 
+	rc = pse_register_notifier(&phy_pse_notifier);
+	if (rc)
+		goto err_genphy;
+
 	return 0;
 
+err_genphy:
+	phy_driver_unregister(&genphy_driver);
 err_c45:
 	phy_driver_unregister(&genphy_c45_driver);
 err_ethtool_phy_ops:
@@ -3994,6 +4130,7 @@ static int __init phy_init(void)
 
 static void __exit phy_exit(void)
 {
+	pse_unregister_notifier(&phy_pse_notifier);
 	phy_driver_unregister(&genphy_c45_driver);
 	phy_driver_unregister(&genphy_driver);
 	rtnl_lock();
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 457eef5784f8..20d33f7af743 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -25,8 +25,55 @@ static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
 static DEFINE_MUTEX(pse_pw_d_mutex);
 
+/* Serialises phydev->psec against the PSE controller lifecycle notifier and
+ * the ethtool PSE paths, in place of rtnl. The attach must not take rtnl: an
+ * MDIO bus registered from ndo_init (e.g. lantiq_etop) calls
+ * phy_device_register() with rtnl already held, so taking rtnl for the attach
+ * would deadlock. It lives here rather than in phylib because PSE_CONTROLLER
+ * is bool, so pse_core is always built into vmlinux and net/ethtool can call
+ * these directly; phylib is tristate and must not be linked against from
+ * built-in code. Lock order: rtnl -> pse_phy_mutex -> pse_list_mutex ->
+ * pcdev->lock.
+ */
+static DEFINE_MUTEX(pse_phy_mutex);
+
 static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
 
+/**
+ * pse_phy_lock - serialise access to phydev->psec
+ *
+ * Held by the PSE controller lifecycle notifier, by the phy attach and detach
+ * paths and by the ethtool PSE paths. The PSE_UNREGISTERED walk clears
+ * phydev->psec and drops the phy's reference under this lock, so anything that
+ * attaches, detaches or dereferences phydev->psec must hold it across the
+ * whole access.
+ */
+void pse_phy_lock(void)
+{
+	mutex_lock(&pse_phy_mutex);
+}
+EXPORT_SYMBOL_GPL(pse_phy_lock);
+
+/**
+ * pse_phy_unlock - release the lock taken by pse_phy_lock()
+ */
+void pse_phy_unlock(void)
+{
+	mutex_unlock(&pse_phy_mutex);
+}
+EXPORT_SYMBOL_GPL(pse_phy_unlock);
+
+#ifdef CONFIG_LOCKDEP
+/**
+ * pse_phy_lock_assert_held - assert that pse_phy_lock() is held
+ */
+void pse_phy_lock_assert_held(void)
+{
+	lockdep_assert_held(&pse_phy_mutex);
+}
+EXPORT_SYMBOL_GPL(pse_phy_lock_assert_held);
+#endif
+
 /**
  * pse_register_notifier - register a callback for PSE controller events
  * @nb: notifier block to register
@@ -1271,6 +1318,13 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	 */
 	cancel_work_sync(&pcdev->ntf_work);
 
+	/* Every handle should be gone here: subscribers drop theirs in the
+	 * event above, and the worker's transient one goes with the drain.
+	 * The controller is off the list and the irq is off, so nothing can
+	 * add one. Anything left is a holder nobody accounted for.
+	 */
+	WARN_ON(!list_empty(&pcdev->pse_control_head));
+
 	pse_flush_pw_ds(pcdev);
 	pse_release_pis(pcdev);
 	kfifo_free(&pcdev->ntf_fifo);
@@ -2132,3 +2186,17 @@ bool pse_has_c33(struct pse_control *psec)
 	return psec->pcdev->types & ETHTOOL_PSE_C33;
 }
 EXPORT_SYMBOL_GPL(pse_has_c33);
+
+/**
+ * pse_control_matches_pcdev - Test whether a pse_control targets a controller
+ * @psec: pse_control obtained from of_pse_control_get()
+ * @pcdev: PSE controller to compare against
+ *
+ * Return: %true if @psec was obtained from @pcdev, %false otherwise.
+ */
+bool pse_control_matches_pcdev(struct pse_control *psec,
+			       struct pse_controller_dev *pcdev)
+{
+	return psec->pcdev == pcdev;
+}
+EXPORT_SYMBOL_GPL(pse_control_matches_pcdev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c5098a0dd6c..ef5b5e6f4e0f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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;
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index bc5d36bcd993..16181f8a2c97 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -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
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 757c9e0cc856..654325946aaa 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · 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] At this commit, pse_controller_unregister() unlinks the
  controller, fires PSE_UNREGISTERED (which has no subscriber yet),…
- [Low] The patch fixes a teardown use-after-free that exists today, but
  it is presented as a feature patch.
- [Low] The new comment above disable_irq() in
  pse_controller_unregister(), and the commit message's claim that…

Pre-existing issues:
- [High] pse_controller_unregister() frees pcdev->pi (pse_release_pis()
  -> kfree) while the per-PI regulators that index that array are still…

--

net: pse-pd: fire lifecycle events on controller register/unregister

This commit fires PSE_REGISTERED and PSE_UNREGISTERED on the
pse_controller_notifier chain from pse_controller_register() and
pse_controller_unregister(). It also reorders pse_controller_unregister()
so that the IRQ is disabled and the controller unlinked first. The event
fires next, then the notification worker is drained, and pw_ds,
pcdev->pi and the kfifo are freed last.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 84c734ed4553e..56cecf60c5c4c 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -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);

[Severity: Low]
Is this comment accurate for the in-tree users of devm_pse_irq_helper()?
The same question applies to the commit message claim that "until now
pse_isr() and the worker could both reach pcdev->pi[] after
pse_release_pis() had freed it".

tps23881_i2c_probe() registers the controller before it requests the IRQ:

drivers/net/pse-pd/tps23881.c:tps23881_i2c_probe() {
    ...
	ret = devm_pse_controller_register(dev, &priv->pcdev);
    ...
	return tps23881_setup_irq(priv, client->irq);
}

The call chain tps23881_setup_irq() -> devm_pse_irq_helper() ->
devm_request_threaded_irq() therefore adds the IRQ devres after the
controller's devres.

Devres unwinds in LIFO order, so free_irq() runs first and waits for any
threaded pse_isr(). Only then does devm_pse_controller_release() call
pse_controller_unregister(). By the time this disable_irq() runs, the
line has no handler, and the call only raises the disable depth.

So devres ordering, not disable_irq(), is what keeps pse_isr() away from
the frees here. Could the comment say so, so that a later change doesn't
rely on disable_irq() as the guard?

Separately, and not introduced by this patch, devm_pse_irq_helper() sets
pcdev->irq even when devm_request_threaded_irq() fails:

	if (ret)
		dev_err(pcdev->dev, "Failed to request IRQ %d\n", irq);

	pcdev->irq = irq;
	return ret;

> -	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);

[Severity: Low]
This reorder fixes a use-after-free that already exists before this
patch. It comes under a feature subject, though, with no Fixes: or
Cc: stable tags.

Before this patch, pse_controller_unregister() called pse_release_pis()
first, which does kfree(pcdev->pi), and called list_del(&pcdev->list)
last. Meanwhile of_pse_control_get() holds pse_list_mutex, walks
pse_controller_list and reads r->pi[i].np through of_pse_match_pi():

	ret = of_pse_match_pi(r, args.np);

A PSE lookup that races a controller unbind could therefore read the
freed pcdev->pi[]. One example is a PHY registered through
fwnode_mdiobus_register_phy().

The commit message says the reordering "closes a teardown race that is
reachable today, with no subscriber involved". It points to the net
series at

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

but also says the order there "leaves the unlink below cancel_work_sync()
and pse_flush_pw_ds()".

Does that net series also close the lookup-vs-free race? If it doesn't,
should the reorder go into a separate net patch with Fixes: tags for
9be9567a7c59 ("net: pse-pd: Add support for PSE PIs") and ffef61d6d273
("net: pse-pd: Add support for reporting events")? If it does, could the
commit message say that the referenced series covers the lookup race?

> +
> +	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);

[Severity: High]
Can the work be queued again after this cancel_work_sync() at this point
in the series?

PSE_UNREGISTERED has no subscriber yet, so nothing stops the existing
pse_control holders. phydev->psec is taken in fwnode_mdio.c and only put
in phy_device_remove(), so it outlives the unregister.

The ethtool PSE paths run under rtnl only, and
pse_controller_unregister() never takes rtnl or pcdev->lock. That seems
to allow this sequence:

CPU1 (ethtool)                         CPU2 (unbind)
pse_ethtool_set_prio()
                                       pse_controller_unregister()
                                         cancel_work_sync()
  pse_pw_d_retry_power_delivery()
    pse_disable_pi_pol()
      kfifo_in_spinlocked(&pcdev->ntf_fifo, ...)
      schedule_work(&pcdev->ntf_work)
                                         pse_release_pis()
                                         kfifo_free(&pcdev->ntf_fifo)

pse_ethtool_set_prio() also reads pcdev->pi[psec->id].pw_d, and
pse_ethtool_get_status() reads pcdev->pi[] as well. A late
pse_control_put() -> __pse_control_release() reads the freed array too:

	if (psec->pcdev->pi[psec->id].admin_state_enabled)
		regulator_disable(psec->ps);

Before this patch the same stale-holder problem existed, and pi[] was
freed even earlier, so this is not a new regression. Still, the commit
message describes the worker-vs-free race as closed, and it looks
reachable at this commit.

The last patch in the series, "net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook", appears to resolve this. Its
PSE_UNREGISTERED handler (phy_pse_detach_one) releases every phydev->psec
under pse_phy_lock(), and the ethtool PSE paths take the same lock. With
that in place, cancel_work_sync() drains everything queued before the
walk completes.

Could this commit message say that the worker race is only fully closed
once that patch is applied?

> +
> +	pse_flush_pw_ds(pcdev);
> +	pse_release_pis(pcdev);

[Severity: High]
This is a pre-existing issue, but can the PI regulators still reach
pcdev->pi[] after it is freed here?

devm_pse_pi_regulator_register() registers the PI regulators during
pse_controller_register():

	rdev = devm_regulator_register(pcdev->dev, rdesc, &rconfig);

devm_pse_controller_register() only adds its own release after
pse_controller_register() returns:

	*pcdevp = pcdev;
	devres_add(dev, pcdevp);

Every in-tree driver passes dev == pcdev->dev. On unbind, devres
therefore runs devm_pse_controller_release() ->
pse_controller_unregister() -> pse_release_pis() -> kfree(pcdev->pi)
before the regulator devres entries unregister the PI regulators.

In that window, reading /sys/class/regulator/*/state goes through
state_show() -> _regulator_is_enabled() -> pse_pi_is_enabled():

	mutex_lock(&pcdev->lock);
	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {

This reads pcdev->pi[id].pw_d from freed memory and then dereferences
it. pcdev->lock does not exclude pse_release_pis(), and pcdev->pi is not
set to NULL on this path.

The commit message says the frees "end up last". That is true inside
pse_controller_unregister(), but not across the full devres teardown.

The next patch in the series, "net: pse-pd: unwind allocations when
controller registration fails", acknowledges this read of freed memory
but does not fix it.

> +	kfifo_free(&pcdev->ntf_fifo);
>  }
>  EXPORT_SYMBOL_GPL(pse_controller_unregister);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de

Re: [PATCH net-next v7 3/5] net: pse-pd: unwind allocations when controller registration fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 2 · 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] pse_flush_pw_ds() runs after pse_register_pw_ds() has already
  dropped pse_pw_d_mutex, so the new flush_pw_ds unwind can race with…
- [Medium] The new flush_pw_ds unwind in pse_controller_register()
  releases the power domains while the PI regulators are still…
- [Medium] Failures in the PI-regulator loop (devm_kzalloc of reg_name,
  devm_pse_pi_regulator_register()) and in pse_register_pw_ds() still…
- [Low] The patch fixes a use-after-free and several leaks that are in
  released kernels, but it has no Fixes: tag, no stable Cc, and it goes…
- [Low] The commit message says the PI array is released on
  'setup_pi_matrix() and the supply check', but at this commit…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  pse_controller_register() calls kfifo_alloc(&pcdev->ntf_fifo,…
- [High] This problem wasn't introduced by this patch, but
  pse_controller_unregister() frees pcdev->pi through pse_release_pis()…

--

net: pse-pd: unwind allocations when controller registration fails

This patch adds error unwinding to pse_controller_register(). A failed
registration now frees the notification kfifo and flushes any partially
created power domains. The PI array is released only on failures that
happen before the first PI regulator is registered.

> 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().

[Severity: Low]
This fixes a use-after-free and several leaks that exist in released
kernels. However, the patch has no Fixes: tags and no stable Cc, and it
goes to net-next inside a feature series. Should it carry tags such as:

Fixes: 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")
Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Fixes: 29e28d1d7a16 ("net: pse-pd: Add support for setup_pi_matrix callback")
Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")

The first covers the pse_register_pw_ds() use-after-free, the second the
kfifo leak, and the last two the PI array and OF reference leaks. The
unwind depends on the rest of the series only through a
blocking_notifier_call_chain() context line.

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

[Severity: Low]
At this commit, setup_pi_matrix() is the only failure that jumps to
release_pis. pse_controller_register() has no supply check yet. The
supply check that unwinds to release_pis comes in the next patch:

"net: pse-pd: check the PI vpwr supply before registering the controller"

Could the message for this patch describe only the code it contains?

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 56cecf60c5c4c..16d75b4babf34 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -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;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but just above this hunk the
kfifo is sized before nr_lines is normalized:

pse_controller_register() {
    ...
	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
	...
	if (!pcdev->nr_lines)
		pcdev->nr_lines = 1;
    ...
}

pse_reg_probe() in drivers/net/pse-pd/pse_regulator.c never sets
nr_lines, so kfifo_alloc() is passed 0. __kfifo_alloc_node() then calls
roundup_pow_of_two(0), which evaluates 1UL << BITS_PER_LONG. That shift is
undefined, and either possible result fails the size < 2 check, so
kfifo_alloc() returns -EINVAL.

Does this mean the podl-pse-regulator controller can never finish
pse_controller_register()?

Moving the normalization above kfifo_alloc() would not be enough on its
own, because a capacity of 1 is also rejected. The fifo size would need
to be at least 2.

[ ... ]

> @@ -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;
>  	}

[Severity: Medium]
Do these failure paths leak the OF node references taken in
of_load_pse_pis()?

Three paths end at free_kfifo: the reg_name allocation failure, the
devm_pse_pi_regulator_register() failure and the flush_pw_ds path. On
all of them, the of_node_get() on each PI node and the of_parse_phandle()
reference on each pairset are never dropped.

devm_pse_controller_register() frees only its devres cookie, so these
references leak on every failed probe. The commit message lists "an OF
reference per described PI" among the leaks it fixes.

The reason given for keeping the array does not seem to cover these
references:

- none of the pse_pi_ops touch pi[].np or pairset[].np
- the regulator core takes its own reference with
  of_node_get(config->of_node)
- of_pse_match_pi() is reachable only through pse_controller_list, which
  a failed controller never joins

Could the node references be put, and their pointers cleared, on these
paths without freeing the array?

Also, as the commit message notes, a reg_name failure in the first
iteration happens before any PI regulator exists. Releasing the whole
array would still be safe there.

>  
>  	ret = pse_register_pw_ds(pcdev);
>  	if (ret)
> -		return ret;
> +		goto flush_pw_ds;

[Severity: High]
Can the flush_pw_ds unwind race with another controller that registers
on the same supply?

pse_register_pw_ds() drops pse_pw_d_mutex at its out: label while the
domains it already created are still published in pse_pw_d_map.
pse_flush_pw_ds() runs only afterwards, outside that lock:

CPU1 (controller A)                  CPU2 (controller B, same supply)
pse_register_pw_ds()
  devm_pse_alloc_pw_d() for PI 0
    /* published in pse_pw_d_map */
  later iteration fails
  out: mutex_unlock(&pse_pw_d_mutex)
                                     pse_register_pw_ds()
                                       regulator_is_equal() matches A's pw_d
                                       pcdev->pi[i].pw_d = pw_d;
                                       kref_get(&pw_d->refcnt);
flush_pw_ds:
  pse_flush_pw_ds()
    kref_put_mutex()  /* 2 -> 1 */
probe fails, devres frees pw_d

The refcount only drops from 2 to 1, so __pse_pw_d_release() never runs
and there is no xa_erase() or regulator_put(). The pw_d lifetime comes
from devres on A's device, not from the kref. After the failed probe,
both pse_pw_d_map and B's pi[i].pw_d point at freed memory.

Would the next registration then still read freed memory in
regulator_is_equal()? B's pse_pw_d_is_sw_pw_control() and
pse_pi_allocate_pw_budget*() would also read the freed domain. That looks
like the same use-after-free the commit message describes.

Does this need the unwind to happen inside the mutex-held section of
pse_register_pw_ds()? The alternative would be to stop devm-allocating
the shared, refcounted domain.

[ ... ]

> @@ -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;

[Severity: Medium]
Does this leave pcdev->pi[i].pw_d dangling while the PI regulators are
still live?

pse_flush_pw_ds() calls __pse_pw_d_release(), which does
regulator_put(pw_d->supply) and xa_erase(). pcdev->pi[i].pw_d is not
cleared.

The pw_d itself was devm_kzalloc()ed after the PI regulators were
devm-registered. When the failed probe unwinds, devres therefore frees it
before it unregisters the regulators.

In that window, the regulator state attribute or regulator_late_cleanup()
can reach:

pse_pi_is_enabled()
    pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)
        pw_d->budget_eval_strategy   /* freed */

pse_pi_deallocate_pw_budget() and pse_pi_allocate_pw_budget_static_prio()
would also still use the already released pw_d->supply.

The devres ordering window existed before this patch. What is new is that
the flush drops the supply while pi[].pw_d still points at it.

The comment added above the regulator loop says the live regulators must
not be handed a freed pointer. Would setting pcdev->pi[i].pw_d = NULL
after pse_flush_pw_ds() on this path cover it? The ops already treat a
NULL pw_d as no domain.

> +
> +release_pis:
> +	pse_release_pis(pcdev);
> +	pcdev->pi = NULL;

[Severity: High]
This is a pre-existing issue, and the commit message mentions it, but
pse_controller_unregister() still ends with:

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	kfifo_free(&pcdev->ntf_fifo);

devm_pse_controller_register() adds its devres node after the PI
regulators and pw_d allocations. On unbind, devres therefore calls
pse_controller_unregister() first, and pse_release_pis() frees pcdev->pi
while every PI regulator is still registered:

devm_pse_controller_release()
    pse_controller_unregister()
        pse_release_pis()
            kfree(pcdev->pi)

Can a read of /sys/class/regulator/regulator.N/state (mode 0444) that
races with the unbind then reach this, before devres unregisters the
regulators?

pse_pi_is_enabled()
    pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)

That would dereference a pw_d read from the freed array. The commit
message defers the fix to a pending net teardown patch, and the problem
is still present at the end of this series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de

Re: [PATCH net-next v7 4/5] net: pse-pd: check the PI vpwr supply before registering the controller

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 0

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] In pse_pi_check_supply() (drivers/net/pse-pd/pse_core.c), the
  first lookup does not search the same places the regulator core…
- [Medium] The new check loop in pse_controller_register()
  (pse_core.c:1180-1183) runs after pcdev->ops->setup_pi_matrix().
- [Medium] Between the check and the registration, nothing holds the
  provider, and the second resolution applies a condition the check…

--

net: pse-pd: check the PI vpwr supply before registering the controller

This commit adds pse_pi_check_supply(), which looks up each PI's vpwr
supply before any PI regulator is registered. It checks the PI node
first and then the controller device. If a lookup fails with
-EPROBE_DEFER or another error, pse_controller_register() fails, so the
PSE driver's own probe defers instead of registering a controller whose
PIs cannot be handed out.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 16d75b4babf34..457eef5784f8b 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -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)
> +{

[ ... ]

> +	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;
> +		}

[Severity: High]
Can this stage-one lookup pick up a sibling PI's supply instead of this
PI's?

of_regulator_get_optional(pcdev->dev, pi_np, "vpwr") ends up in
of_get_regulator(). When the node it is given has no match, it falls
back to the device's node, not to that node:

drivers/regulator/of_regulator.c:of_get_regulator() {
    ...
	regnode = of_parse_phandle(node, prop_name, 0);
	if (regnode)
		return regnode;

	regnode = of_get_child_regulator(dev->of_node, prop_name);
    ...
}

Here dev->of_node is the controller node. If pse-pi@N has no vpwr-supply
of its own, the fallback walks pse-pis and every sibling pse-pi node and
returns the first vpwr-supply it finds.

The core resolves the PI regulator's supply in a different order. The
PI rdev's of_node is the PI node. regulator_resolve_supply() first calls
regulator_dt_lookup(&rdev->dev, ...), which searches only the PI
subtree. It then calls regulator_dev_lookup(pcdev->dev, ...), which
reads the controller node's own vpwr-supply first.

Take a DT where the controller has vpwr-supply = <&main> and main has
not probed yet, pse-pi@0 has vpwr-supply = <&aux> and aux is registered,
and pse-pi@1 has no vpwr-supply:

pse_pi_check_supply(pcdev, 1)
  of_regulator_get_optional(pcdev->dev, pi1_np, "vpwr")
    of_get_child_regulator(controller_np, ...) finds &aux via pse-pi@0
  regulator_put(); return 0;

devm_pse_pi_regulator_register(pcdev, ..., 1)
  regulator_register()
    regulator_resolve_supply()
      resolves to &main -> -EPROBE_DEFER, ignored

The controller then fires PSE_REGISTERED, but regulator_get_exclusive()
in pse_control_get_internal() keeps returning -EPROBE_DEFER for PI 1.

With the later phy patch in this series, phy_try_attach_pse() drops
-EPROBE_DEFER without a warning and retries only on PSE_REGISTERED. Does
that port then never get its PSE control? This looks like the
controller-level vpwr-supply case that the commit message says this
patch closes.

The comment above the function says that of_regulator_get_optional()
"reports a PI with no vpwr-supply described as -ENODEV". That doesn't
hold when any sibling PI has a vpwr-supply.

Would it work to call of_regulator_get_optional() only when
of_property_present(pcdev->pi[id].np, "vpwr-supply") is true, and
otherwise go straight to the controller-level lookup? Doing the lookup
through a device whose of_node is the PI node would also work.

> +
> +		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;
> +	}

[Severity: Medium]
Is there still a window here where the check passes but PI registration
defers?

regulator_put() drops the supply handle right away. Nothing holds the
provider until devm_pse_pi_regulator_register() resolves the supply
again.

That second resolution also runs a check that the consumer-get path used
here skips:

drivers/regulator/core.c:regulator_resolve_supply() {
    ...
	if (r->dev.parent && r->dev.parent != rdev->dev.parent) {
		if (!device_is_bound(r->dev.parent)) {
			put_device(&r->dev);
			ret = -EPROBE_DEFER;
			goto out;
		}
    ...
}

regulator_register() then ignores the result:

		ret = regulator_resolve_supply(rdev);
		if (ret)
			rdev_dbg(rdev,
				 "unable to resolve supply (ignoring): %pe\n",
				 ERR_PTR(ret));

Two cases pass pse_pi_check_supply() here:

- a provider whose rdev is registered but whose probe has not finished,
  for example with async probing or several regulators registered in a
  loop
- a provider that unregisters its rdev after this check, for example
  because its own probe later defers

In both cases pse_controller_register() adds the controller to the list
and fires PSE_REGISTERED. of_pse_control_get() for that PI still returns
-EPROBE_DEFER.

The commit message calls this a narrower, known window. However, the
final phy patch in the series ("net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook") ignores -EPROBE_DEFER and retries
only on PSE_REGISTERED. Its commit message says "the preceding patch
makes sure an already registered controller cannot keep returning it".

Doesn't this window break that guarantee and leave the PHY without its
PSE control when it is hit?

[ ... ]

> @@ -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;
> +	}

[Severity: Medium]
Does this new failure exit leak what setup_pi_matrix() has already done?

This loop runs after pcdev->ops->setup_pi_matrix() has succeeded.
release_pis frees only the PI array, the OF node references and the
kfifo.

For pd692x0, pd692x0_setup_pi_matrix() has already done this by the time
of the new deferral point:

pd692x0_register_managers_regulator()
  devm_regulator_bulk_get_enable(&rdev->dev, ...)    /* aux rails on */
pd692x0_req_managers_pw_budget()
  regulator_request_power_budget(supply, pw_budget)  /* claim on vmain */

pw_requested_mW is lowered only by regulator_free_power_budget().
regulator_put() and regulator_unregister() leave it alone.

pd692x0_managers_free_pw_budget() is called only from
setup_pi_matrix()'s own error labels and from pd692x0_i2c_remove(). When
devm_pse_controller_register() fails, pd692x0_i2c_probe() just returns
dev_err_probe().

If pse_pi_check_supply() returns -EPROBE_DEFER here, does the vmain
budget stay claimed by a probe instance that no longer exists?

On the retry, regulator_get_unclaimed_power_budget() would return the
reduced value, or 0. The new instance would then record
manager_pw_budget[i] = 0 and skip SET_POWER_BANK in later
pd692x0_hw_conf_init() calls, for example after a firmware upload.

Later budget requests through vmain, such as those from
pse_pi_update_pw_budget(), could fail with -ERANGE. The aux rails are
also turned on and off on every deferred retry.

The same leak already existed for rarer failures after
setup_pi_matrix(). This patch adds a failure point that can repeat on
every deferred probe.

The commit message says "Doing all the checks before the registration
loop also keeps the common deferral on the unwind path", but that
doesn't cover setup_pi_matrix() side effects.

Moving the check ahead of setup_pi_matrix() doesn't look possible,
because pd692x0 PIs normally take vpwr from manager regulators
registered inside it. Would this need a driver unwind hook, or a
devm-managed release of the budget in pd692x0?

> +
>  	/* 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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de

Re: [PATCH net-next v7 5/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 3 (1) · Low: 3

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

- [Medium] phy_try_attach_pse() drops -EPROBE_DEFER silently on the
  assumption that a later PSE_REGISTERED event will retry.
- [Medium] PHY registration now succeeds before the PSE controller
  exists, but fw_devlink still treats `pses` as a supplier (parse_pses…
- [Medium] PHY attach now runs synchronously from PSE_REGISTERED inside
  pse_controller_register(), which the PSE driver calls in the middle of…
- [Low] The commit message says the patch fixes user-visible problems: a
  deferred-probe retry storm, and PHYs or DSA switches that never…
- [Low] The comment above phy_try_attach_pse() says 'Any other error
  means a broken binding'.
- [Low] pd692x0_i2c_probe() registers the PSE controller even when
  fw_state is PD692X0_FW_BROKEN or PD692X0_FW_NEED_UPDATE.

Pre-existing issues:
- [High] struct pse_control stores a raw `attached_phydev` pointer and
  holds no device reference on it.
- [Medium] pse_control_get_internal() leaks a module reference.

--

net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

This patch moves ownership of phydev->psec from
fwnode_mdiobus_register_phy() into phylib. The handle is attached from
phy_device_register() and from the PSE_REGISTERED notifier walk. It is
released from phy_device_remove() and from the PSE_UNREGISTERED walk.

A new pse_phy_mutex in pse_core serialises phydev->psec access in place of
rtnl, and the ethtool PSE paths take it as well.

Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/

> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index ba70915182654..7bd979b59f496 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c

[ ... ]

> @@ -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;
> -

[Severity: Low]
The commit message describes a user-visible deferred probe retry storm, and
there are Reported-by and Closes tags, but no Fixes: tag. The -EPROBE_DEFER
pass-through removed here came from commit 5e82147de1cb ("net: mdiobus:
search for PSE nodes by parsing PHY nodes.").

Should a Fixes: tag be added? Leaving it out may be intentional. The earlier
patches in the series suggest it targets net-next. A Fixes: tag could also
pull this patch into stable without the patches it depends on.

[ ... ]

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa92..971d9326d2265 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1012,9 +1013,111 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>  }
>  EXPORT_SYMBOL(get_phy_device);
>  
> +/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
> + * Caller must hold pse_phy_lock(). A missing phandle (-ENOENT) or a
> + * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
> + * retries the latter at PSE_REGISTERED time. Any other error means a broken
> + * binding and is warned about, but left non-fatal so the phy still registers.

[Severity: Low]
This isn't a bug, but is "Any other error means a broken binding" accurate?

of_pse_control_get() -> pse_control_get_internal() can also fail with:

  - -ENOMEM from the allocation
  - errors from pse_pi_is_hw_enabled() -> ops->pi_get_admin_state(), such
    as i2c failures or a firmware-unavailable code
  - regulator_get_exclusive() errors such as -EBUSY or -EPERM

The commit message itself calls these transient failures.

> + *
> + * A phy with psec_detached set is skipped: it is either not registered yet
> + * or on its way out, and nothing would release a handle attached now.
> + */
> +static void phy_try_attach_pse(struct phy_device *phydev)
> +{

[ ... ]

> +	psec = of_pse_control_get(np, phydev);
> +	if (IS_ERR(psec)) {
> +		if (PTR_ERR(psec) != -EPROBE_DEFER && PTR_ERR(psec) != -ENOENT)

[Severity: Medium]
Can a controller that is already registered still return -EPROBE_DEFER
here? The commit message says:

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

pse_control_get_internal() can still defer through the PI's vpwr supply:

pse_control_get_internal()
  regulator_get_exclusive()
    _regulator_get_common()
      regulator_resolve_supply()

drivers/regulator/core.c:regulator_resolve_supply() {
    ...
	if (r->dev.parent && r->dev.parent != rdev->dev.parent) {
		if (!device_is_bound(r->dev.parent)) {
			put_device(&r->dev);
			ret = -EPROBE_DEFER;
			goto out;
		}
	}
    ...
}

The preceding patch adds the pse_pi_check_supply() pre-check. That check
only calls of_regulator_get_optional(), which does not look at
device_is_bound(). That patch's message also says "a PSE probe
interleaving with the provider's own probe can still pass the check and
resolve late".

In that case PSE_REGISTERED fires, the lookup defers, and the phy is
skipped with no message. When the supply resolves later, nothing triggers
another PSE_REGISTERED or phy registration.

Would the port then stay at "No PSE is attached" for the rest of the boot?
Before this patch, deferred probe of the MDIO bus would have retried it.

> +			phydev_warn(phydev, "failed to get PSE control: %pe\n",
> +				    psec);
> +		return;
> +	}

[Severity: Low]
This isn't a regression, because the commit message documents it and the
failure is logged. There is one case where no later retry ever comes,
though.

pd692x0_i2c_probe() registers the controller while fw_state is still
PD692X0_FW_BROKEN or PD692X0_FW_NEED_UPDATE, and firmware_upload_register()
runs after that. The PSE_REGISTERED walk gets the pd692x0_fw_unavailable()
error from pi_get_admin_state() and gives up here.

After a successful firmware upload, pd692x0_fw_poll_complete() only does:

	priv->fw_state = PD692X0_FW_OK;
	return FW_UPLOAD_ERR_NONE;

Nothing re-runs the attach, so the phys stay without PSE until the PSE or
MAC driver is rebound. Is that the intended behaviour?

> +
> +	phydev->psec = psec;
> +}

[ ... ]

> +static int phy_pse_detach_one(struct device *dev, void *data)
> +{

[ ... ]

> +	phydev->psec = NULL;
> +	pse_control_put(psec);
> +	return 0;
> +}

[Severity: Medium]
Can this power down ports when a PSE driver fails later in its own probe?

The PSE_REGISTERED walk now runs synchronously inside
pse_controller_register(), partway through the PSE driver's probe. For a
PI that the hardware already has powered, pse_control_get_internal() sets
admin_state_enabled from pse_pi_is_hw_enabled(), and
regulator_get_exclusive() sets use_count to 1.

If a later probe step fails, devm unwinding calls
pse_controller_unregister(), and this walk drops the last reference:

phy_pse_detach_one()
  pse_control_put()
    __pse_control_release()
      if (psec->pcdev->pi[psec->id].admin_state_enabled)
          regulator_disable(psec->ps);
            -> pse_pi_disable() -> ops->pi_disable()

For example, tps23881_i2c_probe() calls tps23881_setup_irq() after
devm_pse_controller_register(), and that returns -EINVAL when client->irq
is 0:

	if (!irq) {
		dev_err(&client->dev, "interrupt is missing");
		return -EINVAL;
	}

pd692x0 can fail the same way in firmware_upload_register().

On a tps23881 board with no interrupt in DT, would every boot switch off
ports that were delivering power before the probe began? Before this
patch, a failed PSE probe left those ports alone. A normal unbind now also
powers down every PI attached to a phy.

> +static int phy_pse_notifier_event(struct notifier_block *nb,
> +				  unsigned long event, void *data)
> +{
> +	switch (event) {
> +	case PSE_REGISTERED:
> +		pse_phy_lock();
> +		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
> +				 phy_pse_attach_one);
> +		pse_phy_unlock();
> +		return NOTIFY_OK;

[Severity: Medium]
This is a pre-existing issue, but does this retry make a module reference
leak in pse_control_get_internal() easier to hit?

After try_module_get(pcdev->owner) succeeds, two error paths jump past
the module_put(): the !pcdev->ops->pi_get_admin_state branch and the
pse_pi_is_hw_enabled() failure:

	ret = pse_pi_is_hw_enabled(pcdev, index);
	if (ret < 0)
		goto free_psec;

The put_module label sits above free_psec, so it is skipped.

The lookup is now best effort. It is repeated for every unattached phy on
each PSE_REGISTERED walk, and again in phy_device_register(). Each attach
that hits a failing pi_get_admin_state() (an i2c error, or a pd692x0
firmware-unavailable code) would leak one reference. The PSE driver
module could then no longer be unloaded.

Should those paths use goto put_module instead?

[ ... ]

> @@ -1034,12 +1137,25 @@ int phy_device_register(struct phy_device *phydev)

[ ... ]

> +	pse_phy_lock();
> +	phydev->psec_detached = false;
> +	phy_try_attach_pse(phydev);
> +	pse_phy_unlock();
> +
>  	return 0;

[Severity: Medium]
Could this leave the phy bound to the generic driver? Registration now
succeeds before the PSE controller exists, but fw_devlink still treats
pses as a supplier:

drivers/of/property.c:of_supplier_bindings[] {
	...
	{ .parse_prop = parse_pses, },
	...
}

With fw_devlink enforcing, device_links_check_suppliers() defers the phy's
own driver probe until the PSE device binds. If the MAC or DSA switch
connects to the phy in that window, phy_attach_direct() falls back to the
generic driver:

	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;
		phydev->is_genphy_driven = 1;
	}

device_bind_driver() -> device_links_force_bind() then binds it despite
the pending supplier link.

When the PSE module loads later, the notifier only attaches phydev->psec.
Would the phy then stay on genphy until a reboot or rebind?

Before this patch, phy registration itself deferred until the PSE
controller registered, so this window did not exist. The trigger is the
modular PSE case this patch is aimed at.

> @@ -1061,8 +1177,22 @@ EXPORT_SYMBOL(phy_device_register);

[ ... ]

> +	pse_phy_lock();
> +	psec = phydev->psec;
> +	phydev->psec = NULL;
> +	phydev->psec_detached = true;
> +	pse_control_put(psec);
> +	pse_phy_unlock();
> +
>  	device_del(&phydev->mdio.dev);

[Severity: High]
This is a pre-existing issue, but psec->attached_phydev is a raw pointer
that holds no device reference. Neither this path nor phy_pse_detach_one()
clears it.

Can pse_send_ntf_worker() still use it after the phy has been freed?

pse_send_ntf_worker()
  psec = pse_control_find_by_id(pcdev, ntf.id);
      (takes a kref, then drops pse_list_mutex)
  rtnl_lock();
      (may block)
  netdev = pse_control_get_netdev(psec);
      return psec->attached_phydev->attached_dev;

Suppose the netdev detaches the phy and the MDIO bus is torn down while
the worker waits on rtnl. This put drops the count from 2 to 1, then
device_del() and phy_device_free() free the phy_device. The worker then
reads freed memory.

There is a second route when two phys resolve the same PI id.
pse_control_get_internal() returns the existing psec with a kref_get(),
but it only sets attached_phydev when it creates a new psec. After the
first phy is removed, the surviving phy's psec still points at the freed
phy.

The worker never takes pse_phy_lock(), so the new lock does not cover
this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de