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

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

[Carlo Szelinsky]

This is v8 of the series Corey started as an RFC [3]; I took it over
from v2 [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.

v7 [10] drew an LLM review [11]. Four of its findings are fixed here,
one I disagree with (patch 5 below), and the rest I answered in that
thread. v6 [7] drew one too [8]; v7 folded the patches it was mostly
about into the phy patch, so no commit carries the rtnl detour or the
deferred release. Per Documentation/process/maintainer-netdev.rst I also
ran LLM review locally on v7 and v8 before posting.

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 before the event and before
anything is freed, 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 that
has a DT node and no handle yet, which happens on any board with two PSE
controllers. disable_irq() moves up for the same reason: pse_isr()
queues notifications and reaches pcdev->pi. On tps23881, the only
in-tree user of devm_pse_irq_helper(), devres has already run free_irq()
by then, so the move is what makes the ordering hold for a driver that
requests its irq earlier rather than leaving it to devres.

[9] makes a related reordering for net, independently of any subscriber,
so pse_controller_unregister() conflicts if [9] is applied. 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 up to just after
disable_irq(). The merged function wants this order, which already
includes [9]'s reordering:

    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);
    WARN_ON(!list_empty(&pcdev->pse_control_head));
    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. cancel_work_sync() sits below the event,
not above it, because __pse_control_release() can re-enter your budget
code. regulator_disable() on a PI that is still on runs
_pse_pi_disable(). With the static strategy that retries a pending port
on the same power domain, and if the domain is still over budget it sheds
a lower priority one through pse_disable_pi_pol() - which queues a
notification and calls schedule_work() from inside the walk. Draining
before the walk would leave that work racing kfifo_free(). Draining after
it also keeps 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.

I staged that case in QEMU (below) and confirmed it with a dump_stack()
from pse_disable_pi_pol(), so it is a path I have hit rather than only
reasoned about. The ordering rests on your design though, so please take
a look at it.

Patch 5: 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 5 checks every PI's supply before registering any PI regulator, 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.

The review of v7 found two things wrong with that. The PI-node lookup has
to be taken only once the PI's own phandle resolves, or
of_get_child_regulator() walks the whole controller subtree and answers
one PI from a sibling's supply before the controller node is ever
consulted; that is fixed. The core reaches the same sibling walk, but
only after the controller's own vpwr-supply has had its turn.

It also asked for the loop to move ahead of setup_pi_matrix(), because
pd692x0 claims a power budget there that nothing gives back when
registration fails later. I tried that and it is wrong: pd692x0's pse-pi
nodes name the manager regulators setup_pi_matrix() itself registers, so
checking earlier defers on a supply only that driver can bring up and it
never probes. I added a controller of that shape to the test setup and
confirmed it both ways. The loop stays where it is and the changelog says
what the placement does not clean up.

-EPERM is now treated as resolved, since it only means the provider is
held exclusively. The check still stops short of the core's
device_is_bound() gate, so a probe interleaving with the provider's own
can slip through, and nothing retries that PI until the next
PSE_REGISTERED. fw_devlink only prevents that when the vpwr-supply sits
on the controller node: one in a pse-pi node gives the controller just a
SYNC_STATE_ONLY proxy link, which does not hold its probe back.

Patch 3 exists because patch 5 makes -EPROBE_DEFER an ordinary return
from pse_controller_register(). That function has no unwind past
kfifo_alloc(): the kfifo leaks on every failure, and past
of_load_pse_pis(), which cleans up after itself, so do the PI array and
its OF references - once today and on each retry after patch 5 - 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. It also clears pi[].pw_d, and says
what it does not cover: a power domain shared with another controller can
still be freed under it, which is pre-existing and wants the domain out
of devm.

Patch 4 is a small si3474 change for the same reason. si3474 logged
every controller registration failure with dev_err(), and its binding
puts vpwr-supply in the pse-pi nodes, which fw_devlink does not wait
for, so with patch 5 it would log an error on every deferred probe
retry until the supply appears. It now uses dev_err_probe(), as
pd692x0 and tps23881 already do.

The v6 review [8] caught two things in the phy patch, both fixed in v7.
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. The write before device_add()
is the one place it is set without pse_phy_lock(), which is safe because
the phy is not on the klist yet and device_add()'s own locking orders
it.

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.

Patch 6 is a one-entry change in drivers/of/. fw_devlink treats "pses"
as a supplier binding, so a phy that references a PI has a device link to
the PSE controller and its driver probe waits for it. That was invisible
while the PSE lookup deferred the phy anyway, since fwnode_mdio removed
it again on every retry. Once patch 7 stops
deferring, the phy is registered while its own driver is still blocked,
and a MAC attaching in that window takes the generic driver through
phy_attach_direct() - device_bind_driver() then forces the bind past the
pending link, and nothing re-probes it later.

Marking the link FWLINK_FLAG_IGNORE drops it entirely: no device link, no
ordering, no cycle detection, and the fwnode link goes at the consumer's
device_add(). That is what post-init-providers already does.

Two things do ride on that link today, because it is managed - the fwnode
link sits on the phy's own node, so fw_devlink_create_devlink() asks for
fw_devlink_flags, which defaults to FW_DEVLINK_FLAGS_RPM and carries
neither DL_FLAG_STATELESS nor DL_FLAG_SYNC_STATE_ONLY, so
device_link_add() promotes it to DL_FLAG_MANAGED. So unbinding the PSE
controller releases the phy's driver, and DL_FLAG_AUTOPROBE_CONSUMER
probes the phy when the controller binds. Both are given up on purpose:
the notifier does the attach and detach now, without tearing the port's
phy driver down, and there is no deferral left for an autoprobe to wait
on. DL_FLAG_PM_RUNTIME is the one that really had no effect, since no PSE
driver implements PM ops, sync_state or runtime PM.

That also means patch 6 is not a no-op on its own. The MDIO path
registers the phy before it looks the PI up, so the link exists and goes
active as soon as the controller binds, which puts the cascade's removal
at patch 6 rather than at patch 7. It also stops holding the phy's driver
back during those retries, so at patch 6 alone the driver probes and is
removed again on each one until the controller binds; patch 7 removes
the retries. Its changelog says both.

Rob, Saravana: this one is yours, and it is why the phy patch is safe to
take.

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 net, phylib and drivers/of plus six new exports, and tagging it
would invite a stable backport of all that to cure a probe-retry loop.

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 PSE-originated -EPROBE_DEFER leaves it.
Patch 6 removes the other half, the fw_devlink link that deferred the
phy's driver. With both gone there is no probe-retry loop left.

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 tags I kept. Patch 1 only gained a kernel-doc
correction. 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. That board carries the notifier
mechanism from patches 1, 2 and 7; the failure paths of patches 3 and 5
are exercised in QEMU only, below, patch 6 only by booting with it
applied, and patch 4 is build-tested only.

 - 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, power domain index stable

Also exercised under QEMU, on arm64 under KASAN, PROVE_LOCKING and
kmemleak. The device tree has five PSE controllers:

 - a working one, plus 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.
 - one whose vpwr provider never appears - must defer.
 - one with its vpwr-supply on the controller node instead of the PI
   nodes, which the core resolves one stage later - must defer.
 - one whose controller node carries a vpwr-supply that never appears,
   with the first PI naming its own working supply and the next none, so
   a check that lets of_get_child_regulator() answer the second from the
   first misses the controller's supply - must defer.
 - one shaped like pd692x0, whose PI names a regulator the driver only
   registers from setup_pi_matrix() - must register, not defer.

I confirmed the last three both ways, with and without the fix each is
there for. The WARN_ON in patch 7 stays silent across six unbind cycles
and kmemleak reports nothing.

The same setup stages an over-budget static-priority domain for the
patch 2 case above: releasing a handle in the walk sheds a lower
priority port through pse_disable_pi_pol(). No lockdep splat on that
path, which re-enters the regulator core from a notifier callback under
the chain's rwsem and pse_phy_mutex.

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

Changes in v8:
 - Rewrite the changelogs: drop references to other patches by position
   and to the pending net series (its conflict note is now below the
   --- of patch 2), reflow, and cut review history that belongs here.
 - Patch 2: add Co-developed-by, since the reordering is mine.
 - New patch 4: switch si3474 to dev_err_probe() for the controller
   registration error, so the supply check's -EPROBE_DEFER is not
   logged as an error on every retry.
 - Patch 7: say that any phy registered with a DT node now gets its PI,
   including ones found by mdiobus_scan(), and that a persistent lookup
   error is reported again on each controller registration.
 - Patch 3: clear pi[].pw_d in pse_flush_pw_ds(). The domain is devm
   memory of whoever created it, and pse_pi_is_enabled() still reaches
   that pointer from the regulator "state" attribute.
 - Patch 5: take the PI-node supply lookup only once that PI's own
   vpwr-supply phandle resolves. of_regulator_get_optional() falls back
   to of_get_child_regulator() on the device node, so a PI naming no
   supply of its own was answered from a sibling PI's before the
   controller node was consulted, masking an unresolved vpwr-supply
   there.
 - Patch 5: treat -EPERM from the supply get as resolved. It only means
   another consumer holds the provider exclusively, which the core's own
   resolution never asks about.
 - Patch 5: say why the checks cannot move ahead of setup_pi_matrix(),
   which the review asked for - a pd692x0 PI names a regulator that
   function registers itself.
 - New patch 6: stop "pses" gating probe in fw_devlink, so the phy is not
   left bound to the generic driver once patch 7 removes the deferral.
   Its changelog spells out that the link is managed, so dropping it also
   drops the supplier-unbind cascade and DL_FLAG_AUTOPROBE_CONSUMER, that
   this happens at patch 6 rather than at patch 7, and that on its own it
   lets the phy driver probe and unbind on every MDIO retry.
 - Patch 5: ask the controller-device stage of the supply lookup once for
   the whole controller instead of once per PI.
 - Patch 3: drop the flush_pw_ds label, which had to jump over
   release_pis, in favour of flushing inline at the one failure that
   needs it.
 - Document what is not fixed: the worker drain is only final once the
   phy patch lands, devres already quiesces the irq for tps23881, the OF
   references leak with the PI array on the three paths that keep it, a
   PSE probe failing after registration powers the PI down, and a shared
   power domain can still be freed under a second controller. The last
   is pre-existing and wants the domain out of devm, so not fixed here.
 - Fix changelog claims the review caught: pse_release_pis() does run
   from of_load_pse_pis() too, and nothing retries a PI that resolves
   after the pre-check (v7 said it could "resolve late").
 - Fix the phy_try_attach_pse() comment: other errors are not
   necessarily a broken binding, and -EPROBE_DEFER from a registered
   controller whose vpwr provider is not yet bound is not retried.
 - Patch 3: the shared power domain problem is not a refcount race, as
   my reply to the v7 review put it - pse_register_pw_ds() takes its
   reference under pse_pw_d_mutex and kref_put_mutex() takes that mutex
   for the final put. It is only that the domain is devm memory of the
   controller that created it. Changelog corrected.
 - Correct changelog and comment claims found in a final audit: patch 5's
   comments no longer say nothing retries the consumer at that commit
   (fwnode_mdio still does) or describe a CONFIG_OF=n path that cannot
   be reached; patch 7 no longer says a hard lookup error used to be
   retried by deferred probe - it failed the whole MDIO bus registration.

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 with the phy patch instead
   (patch 7 here), 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 (since v4 [4]):
 - Replace rtnl with a dedicated mutex in the PSE attach path; with rtnl
   it deadlocked lantiq_etop [5].
 - Put phydev->psec back in phy_device_remove(), closing the
   use-after-free Paolo forwarded [6].

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/
[10] https://lore.kernel.org/netdev/20260927191850.1370515-1-github@szelinsky.de/
[11] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de

Carlo Szelinsky (4):
  net: pse-pd: unwind allocations when controller registration fails
  net: pse-pd: si3474: use dev_err_probe() for controller registration
  net: pse-pd: check the PI vpwr supply before registering the
    controller
  of: property: do not let "pses" block a consumer's probe

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   | 144 ++++++++++++++-
 drivers/net/pse-pd/pse_core.c  | 321 +++++++++++++++++++++++++++++++--
 drivers/net/pse-pd/si3474.c    |   7 +-
 drivers/of/property.c          |   5 +-
 include/linux/phy.h            |   7 +
 include/linux/pse-pd/pse.h     |  65 +++++++
 net/ethtool/pse-pd.c           |  16 +-
 8 files changed, 540 insertions(+), 59 deletions(-)


base-commit: e3bfd25626b44b6fa61a13c17178922171d519ce
-- 
2.43.0

[PATCH net-next v8 1/7] 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.

A follow-up change uses 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 takes a mutex, and of_pse_control_get() queries the
controller hardware, over i2c or a UART on most drivers. 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 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 v8 2/7] net: pse-pd: fire lifecycle events on controller register/unregister

[Carlo Szelinsky]

From: Corey Leavitt <corey@leavitt.info>

Hook the 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. The
reordering of pse_controller_unregister() around it is not a no-op,
though: it closes a teardown race that is reachable today, with no
subscriber involved.

The frees move to the bottom. pse_flush_pw_ds() and pse_release_pis()
are currently the first two statements, ahead of disable_irq() and
cancel_work_sync(), so a concurrent of_pse_control_get() and the
notification worker can reach pcdev->pi[] after pse_release_pis() has
freed it, and so can pse_isr() for a driver that requests its irq
before registering. They also have to stay below the event, because the
release path a subscriber runs reads pcdev->pi[] and pi->pw_d->supply.

The controller is unlinked before the event and before anything is
freed. of_pse_control_get() walks pse_controller_list and dereferences
pcdev->pi[] through of_pse_match_pi(). 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.

disable_irq() moves up for the same reason: pse_isr() queues
notifications and reaches pcdev->pi, and nothing after it re-enables
the interrupt. On tps23881, the only in-tree user of
devm_pse_irq_helper(), the irq is requested after
devm_pse_controller_register(), so devres has already run free_irq()
by the time this runs. The move is what makes the ordering hold for a
driver that requests its irq earlier.

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

That retry can also call ops->pi_enable() through
_pse_pi_delivery_power_sw_pw_ctrl(), so a port on the controller being
torn down can be energised from inside the event. The driver is still
attached at that point - the event runs before pse_release_pis() and
before devres unwinds the PI regulators - so the call is legal.

The drain is not yet final on its own. A pse_control holder that does
not subscribe - every phy today, since fwnode_mdio hands out handles
that only phy_device_remove() releases - can still reach
pse_disable_pi_pol() from the ethtool path and queue work after it.
That is how the tree behaves today and this does not widen it; it is
closed once phylib releases its handles in the event under a common
lock.

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

Notes:
    This conflicts in pse_controller_unregister() with the net series at
    https://lore.kernel.org/netdev/20260813200653.980170-1-github@szelinsky.de/
    which reorders the same function without an event to place. The
    resolution should take the order here, which already includes that
    reordering; the cover letter has the merged function.

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

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 84c734ed4553..dc261beb6170 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,41 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
-	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
+	/* Raise the interrupt's disable depth before anything is freed.
+	 * pse_isr() queues notifications and reaches pcdev->pi, and nothing
+	 * below re-enables it. For a driver that requests its irq after
+	 * devm_pse_controller_register(), devres has already run free_irq()
+	 * by the time we get here and this only bumps the depth - the
+	 * ordering does not rely on that, so a driver requesting the irq
+	 * earlier is covered too.
+	 */
 	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 v8 3/7] 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 OF references on each described PI
node and its pairset nodes. Every failure after that point simply
returns and none of it is freed. of_load_pse_pis() cleans up after
itself, but past that point 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.

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 pse_register_pw_ds(), reading pw_d->supply for
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 them. So:

 - failures before of_load_pse_pis() succeeds free only the kfifo;
 - failures after it and before the PI regulator loop (today
   setup_pi_matrix()) also release the PI array and its OF references;
 - failures from the loop onwards free the kfifo, and the
   pse_register_pw_ds() one also flushes the power domains, but leave
   the PI array and its OF references leaked exactly as today rather
   than hand live regulators a dangling pointer.

A driver's setup_pi_matrix() may have registered devm regulators of its
own by the time it fails - pd692x0 registers its managers there - but
those do not index pcdev->pi[]. Freeing the array safely past the loop
would need the regulator ops to tolerate a NULL pcdev->pi, which they do
not today.

pcdev->pi is cleared at the release_pis label and deliberately not
inside pse_release_pis(): pse_controller_unregister() calls that helper
with every PI regulator still registered, and pse_pi_is_enabled()
indexes pcdev->pi[] unguarded behind the "state" attribute, so clearing
it there would turn that pre-existing read of freed memory into a NULL
dereference.

pse_flush_pw_ds() now also clears pi[].pw_d. The domain is devm memory
of whichever controller created it, so it can go as soon as that probe
unwinds, and pse_pi_is_enabled() still reaches pi[].pw_d from the
"state" attribute until the PI regulators are unregistered.

This does not fix a shared power domain. The reference counting is
sound - pse_register_pw_ds() takes its kref_get() under pse_pw_d_mutex
and kref_put_mutex() takes that mutex for the final put - but if
another controller on the same supply holds a reference, the flush only
goes 2->1 and devres frees the creator's pw_d underneath it anyway.
Unregistering a controller whose domain is shared has the same problem
today. Fixing it means moving the domain out of devm memory so that
only the last put frees it.

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

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index dc261beb6170..eeefbf25e671 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -937,11 +937,21 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
 			continue;
 
 		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
-		if (!pw_d)
+		if (!pw_d) {
+			pcdev->pi[i].pw_d = NULL;
 			continue;
+		}
 
 		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
 			       &pse_pw_d_mutex);
+		/* The pw_d is devm memory of whichever controller created
+		 * it, so it can go away as soon as that probe unwinds.
+		 * Nothing may be left pointing at it: pse_pi_is_enabled()
+		 * reaches pi[].pw_d from the regulator "state" attribute,
+		 * which stays readable until the PI regulators are
+		 * unregistered after us.
+		 */
+		pcdev->pi[i].pw_d = NULL;
 	}
 }
 
@@ -1092,17 +1102,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 +1121,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 +1135,27 @@ 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;
+	if (ret) {
+		/* Deliberately not release_pis: the PI regulators registered
+		 * above index pcdev->pi[] and outlive this function.
+		 */
+		pse_flush_pw_ds(pcdev);
+		goto free_kfifo;
+	}
 
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);
@@ -1142,6 +1165,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 				     PSE_REGISTERED, pcdev);
 
 	return 0;
+
+	/* Only for failures above the PI regulator loop, where no regulator
+	 * indexes pcdev->pi[] yet. Anything below it has to go straight to
+	 * free_kfifo instead.
+	 */
+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 v8 4/7] net: pse-pd: si3474: use dev_err_probe() for controller registration

[Carlo Szelinsky]

si3474_i2c_probe() reports a failed devm_pse_controller_register() with
dev_err() and returns the error. That is fine as long as registration
only fails for real, but a follow-up change makes it return
-EPROBE_DEFER while a PI's vpwr supply provider has not registered yet.

skyworks,si3474.yaml puts vpwr-supply in the pse-pi nodes. A supply
named from a child node only gives the controller a SYNC_STATE_ONLY
proxy link in fw_devlink, which does not hold its probe back, so si3474
can probe before that regulator and would then log an error on every
deferred probe retry until it appears.

Use dev_err_probe(), which stays quiet for -EPROBE_DEFER and records the
reason for devices_deferred instead, and prints the error symbolically
rather than as a raw hex value.

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

diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
index 1845b9c51cf7..91800f39971f 100644
--- a/drivers/net/pse-pd/si3474.c
+++ b/drivers/net/pse-pd/si3474.c
@@ -541,10 +541,9 @@ static int si3474_i2c_probe(struct i2c_client *client)
 	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
 
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
-	if (ret) {
-		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register PSE controller\n");
 
 	return 0;
 }
-- 
2.43.0

[PATCH net-next v8 5/7] 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 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. A
consumer that only retries when a controller registers - which is what
phylib becomes once it attaches from the PSE notifier - 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 - every
PI has a NULL np but still gets a regulator - is covered too, through
the controller device.

The check follows both stages regulator_resolve_supply() uses: the PI's
own node, then the controller device. A vpwr-supply written once on the
controller node is invisible from the PI node, while the core finds it
at stage two. pse_pi_check_supply() answers from the PI node and
returns 1 when it cannot; pse_controller_check_supply() is then asked
at the first PI that needs it and not again, since its answer is the
same for all of them. Asking it inside the loop keeps the order the
core resolves in, so the first PI to fail is the one reported, and a
later PI's hard error cannot replace an earlier PI's -EPROBE_DEFER.

The PI node is consulted only once its own vpwr-supply phandle
resolves. of_regulator_get_optional() falls back to
of_get_child_regulator() on the device node, which walks every
descendant of the controller, so a PI naming no supply of its own would
otherwise be answered from a sibling PI's before the controller node
was consulted. The core reaches that sibling walk only at its second
stage, after the controller's own vpwr-supply.

-EPERM is treated as resolved: it only means another consumer holds the
provider exclusively, which regulator_resolve_supply() never asks about.

It is not a full replica. The core also defers while the supply
provider's parent device 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, and nothing retries that PI until the next
PSE_REGISTERED or a phy re-registration. fw_devlink only prevents this
when vpwr-supply is on the controller node; one in a pse-pi node only
gives the controller a SYNC_STATE_ONLY proxy link, which does not hold
its probe back. Closing that would mean asking the core about
rdev->supply after registration, when the PI array can no longer be
released.

The checks run after setup_pi_matrix() and before the registration
loop. They cannot run earlier: a PI's vpwr-supply may name a regulator
the controller registers in setup_pi_matrix() - microchip,pd692x0.yaml
points its pse-pi nodes at the manager regulators
pd692x0_register_managers_regulator() creates - so an earlier check
would defer forever on a supply only this driver can provide. They
should not run inside the loop either: 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 deferring after the first PI regulator exists
would leak the PI array on every retry.

What this placement does not recover is anything setup_pi_matrix() has
already claimed. pd692x0 requests a power budget on its manager
supplies there and frees it only from that function's own error labels
or from remove(). That is true of every existing failure after
setup_pi_matrix() too, and the new deferral does not hit it with the
documented binding, since a pd692x0 PI's supplies are the managers it
has just registered. A board pointing a pd692x0 PI at an external
provider would; that cleanup belongs in the driver.

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 whole
controller defers until every provider has appeared. 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.

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

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index eeefbf25e671..e560833ad034 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,98 @@ 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. A consumer that is not itself retried by deferred probe -
+ * phylib, once it attaches from the PSE notifier - would never get it.
+ * Check the supply up front so the PSE driver's own probe defers instead.
+ *
+ * This is the first of the two stages the core uses, the PI's own node.
+ * pse_controller_check_supply() is the second, the controller device.
+ * Neither get falls back to the dummy regulator, so a PI that names no supply
+ * anywhere still reports -ENODEV and keeps its existing behaviour - the
+ * core resolves it to the dummy when the PI regulator is registered.
+ *
+ * The PI node is consulted only once its own phandle is known to resolve.
+ * of_regulator_get_optional() falls back to of_get_child_regulator() on
+ * the *device* node, which walks every descendant of the controller, so a
+ * PI that does not name a supply itself would otherwise be answered from
+ * a sibling PI's and pass before the controller node is ever consulted.
+ * The core reaches the same sibling walk, but only at its second stage
+ * and only after the controller's own vpwr-supply has had its turn - so
+ * the gate is what keeps a controller-level supply from being masked.
+ *
+ * Return: 0 if this PI needs nothing more, 1 if the controller device has
+ *	   to answer for it, or a negative error.
+ */
+static int pse_pi_check_supply(struct pse_controller_dev *pcdev, int id)
+{
+	struct device_node *np;
+	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;
+
+	np = pcdev->pi[id].np ?
+	     of_parse_phandle(pcdev->pi[id].np, "vpwr-supply", 0) : NULL;
+	if (!np)
+		return 1;
+
+	of_node_put(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);
+	/* -EPERM only says someone holds this provider exclusively, which
+	 * the core's own resolution never asks about. The provider is
+	 * there, so this PI is fine.
+	 */
+	if (ret == -EPERM)
+		return 0;
+	/* 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 (ret == -ENODEV)
+		return 1;
+
+	return dev_err_probe(pcdev->dev, ret,
+			     "PI %d: failed to get vpwr supply\n", id);
+}
+
+/* Second stage of pse_pi_check_supply(). A vpwr-supply on the controller
+ * node covers every PI, and is where a PI without a node of its own
+ * resolves too, so the answer is the same for all of them - ask it once,
+ * and only if some PI needs it.
+ */
+static int pse_controller_check_supply(struct pse_controller_dev *pcdev)
+{
+	struct regulator *supply;
+	int ret;
+
+	supply = regulator_get_optional(pcdev->dev, "vpwr");
+	if (!IS_ERR(supply)) {
+		regulator_put(supply);
+		return 0;
+	}
+
+	ret = PTR_ERR(supply);
+	if (ret == -ENODEV || ret == -EPERM)
+		return 0;
+
+	return dev_err_probe(pcdev->dev, ret,
+			     "failed to get vpwr supply\n");
+}
+
 static int
 devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
 			       char *name, int id)
@@ -1082,6 +1175,7 @@ static void pse_send_ntf_worker(struct work_struct *work)
  */
 int pse_controller_register(struct pse_controller_dev *pcdev)
 {
+	bool ctrl_supply_ok = false;
 	size_t reg_name_len;
 	int ret, i;
 
@@ -1116,6 +1210,38 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 			goto release_pis;
 	}
 
+	/* Check every PI supply before any regulator of ours is registered:
+	 * a provider that has not probed yet is the ordinary -EPROBE_DEFER
+	 * case, and unwinding it must still be able to release the PI array,
+	 * which it cannot once a PI regulator exists.
+	 *
+	 * This has to stay after setup_pi_matrix(). A PI's vpwr-supply can
+	 * name a regulator the controller itself creates there - pd692x0's
+	 * pse-pi nodes point at its manager regulators - so checking any
+	 * earlier would defer on a supply that only this driver can bring
+	 * up, and it would never probe.
+	 */
+	for (i = 0; i < pcdev->nr_lines; i++) {
+		ret = pse_pi_check_supply(pcdev, i);
+		if (ret < 0)
+			goto release_pis;
+
+		/* The controller device answers for every PI that its own
+		 * node cannot, so it is asked at the first such PI and not
+		 * again. Asking it here rather than after the loop keeps
+		 * the order the core resolves in, so the first PI to fail
+		 * is still the one reported.
+		 */
+		if (!ret || ctrl_supply_ok)
+			continue;
+
+		ret = pse_controller_check_supply(pcdev);
+		if (ret)
+			goto release_pis;
+
+		ctrl_supply_ok = true;
+	}
+
 	/* Each regulator name len is pcdev dev name + 7 char +
 	 * int max digit number (10) + 1
 	 */
-- 
2.43.0

[PATCH net-next v8 6/7] of: property: do not let "pses" block a consumer's probe

[Carlo Szelinsky]

fw_devlink treats "pses" as a supplier binding, so a PHY that
references a PSE PI has its driver probe held in
device_links_check_suppliers() until the PSE controller binds: by a
fwnode link to the PI node until then, and by a device link to the
controller once it has bound.

That is harmless while the PSE lookup itself defers the PHY:
fwnode_mdio registers it, fails the lookup and removes it again on
every retry, so it never stays registered long enough to be bound
early. Once phylib stops deferring on PSE and picks the handle up from
a notifier instead, the PHY is registered while its own driver is
still blocked, and a MAC or DSA switch attaching in that window binds
the generic driver:

  phy_attach_direct()
    if (!d->driver)
            d->driver = &genphy_driver.mdiodrv.driver;
    device_bind_driver()
      device_links_force_bind()

device_bind_driver() does not wait for suppliers:
device_links_force_bind() drops any managed supplier link that is not
available yet, and driver_bound() purges the PHY's remaining fwnode
supplier links. Nothing re-probes the PHY once the PSE controller shows
up, so the port keeps running on genphy until a rebind or a reboot.

The link is not needed for correctness. PSE is not a resource the
consumer must have before it probes: phylib looks a PI up when its
controller becomes available and releases it when the controller goes
away, and of_pse_control_get() is the only reader of the property.

Mark it FWLINK_FLAG_IGNORE, as post-init-providers already is. That
drops the dependency entirely: fw_devlink_create_devlink() returns
early, so no device link is made, the fwnode link is deleted at the
consumer's device_add() like any other that has been handled, and
fw_devlink neither gates probe on it nor uses it for cycle detection.

Two things ride on the link today, and both go with it, because it is
managed. of_link_property() passes no get_con_dev for parse_pses, so
the fwnode link sits on the PHY's own node, which is the PHY device's
fwnode by the time it is registered, and fw_devlink_create_devlink()
takes its first branch:

	if (con->fwnode == link->consumer)
		flags = fw_devlink_get_flags(link->flags);
	else
		flags = FW_DEVLINK_FLAGS_PERMISSIVE;

With link->flags clear that returns fw_devlink_flags, by default
FW_DEVLINK_FLAGS_RPM, which carries neither DL_FLAG_STATELESS nor
DL_FLAG_SYNC_STATE_ONLY, so device_link_add() makes it DL_FLAG_MANAGED.
Unbinding the PSE controller therefore releases the PHY's driver today,
and DL_FLAG_AUTOPROBE_CONSUMER probes the PHY once the controller
binds. Both are given up on purpose: phylib's notifier takes the PI
away on unbind and hands it back on bind without tearing the PHY driver
down, and with no deferral left there is nothing for an autoprobe to
wait for. DL_FLAG_PM_RUNTIME and the dpm_list reordering go without
consequence, since no PSE driver implements PM ops, .shutdown,
sync_state or runtime PM.

This change is not a no-op while fwnode_mdio still does the lookup.
fwnode_mdiobus_register_phy() registers the PHY before it looks the PI
up, so the device link is created regardless and becomes active as
soon as the controller is bound; the unbind cascade above goes away
from here on. The link also held the PHY's driver back on each of those
retries while the controller was unbound. Without it the driver probes
inside device_add() - phy drivers are PROBE_FORCE_SYNCHRONOUS - and is
removed again when the lookup defers, once per retry of the MDIO bus
owner, until the PSE controller binds or fwnode_mdio stops doing the
lookup.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/of/property.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 72cf12907de0..5ec0f05b87ac 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1564,7 +1564,10 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_backlight, },
 	{ .parse_prop = parse_panel, },
 	{ .parse_prop = parse_msi_parent, },
-	{ .parse_prop = parse_pses, },
+	{
+		.parse_prop = parse_pses,
+		.fwlink_flags = FWLINK_FLAG_IGNORE,
+	},
 	{ .parse_prop = parse_power_supplies, },
 	{ .parse_prop = parse_mmc_pwrseq, },
 	{ .parse_prop = parse_gpio_compat, },
-- 
2.43.0

[PATCH net-next v8 7/7] 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 MDIO bus owner - a MAC or a DSA switch - keeps bouncing
off deferred probe, registering and removing the PHY each time. On some
boards that shows up as a boot-time probe-retry storm and PHYs that
never stay registered.

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 now covers every phy registered with a DT node, not only
those registered through fwnode_mdiobus_register_phy(). A phy found by
mdiobus_scan() on a bus registered with plain mdiobus_register() gets
its node from of_mdiobus_link_mdiodev(), and a `pses` property there is
now honoured too.

The attach happens after device_add() has made the phy visible on
mdio_bus_type. Neither device_add() nor the attach that follows it may
take rtnl, for two reasons:

  - 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 the bus
    registration reaches phy_device_register() from there
    (mdiobus_scan() for lantiq_etop, of_mdiobus_register_phy() for
    sni_ave).

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 notifier
walks change phydev->psec without rtnl, so the ethtool PSE paths take
the same mutex to keep a released handle from being dereferenced. 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. bus_for_each_dev() still reaches
a phy until bus_remove_device() takes it off the klist, 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 during registration: 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. The flag is therefore held until
registration has succeeded, and set again from phy_device_remove(). 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.

A lookup error no longer fails anything. of_pse_control_get() reaches
the hardware - pse_pi_is_hw_enabled() calls pi_get_admin_state(), a bus
transaction on tps23881, si3474, pd692x0 and realtek-pse-mcu - and that
error used to propagate out of fwnode_mdiobus_register_phy() and fail
the whole MDIO bus registration. Now it leaves the port without PSE
until a controller registers again or the phy is re-registered, and is
reported with phydev_warn(). Because every PSE_REGISTERED walk retries
each phy that has no handle yet, a persistent error such as a
`#pse-cells` mismatch - which also trips the WARN_ON() in
of_pse_control_get() - is now reported again on each controller
registration rather than once.

-EPROBE_DEFER stays silent because the notifier retries it at
PSE_REGISTERED time, and the controller's vpwr supply check removes the
case where a registered controller keeps returning it for a provider
that has not appeared. It does not remove every source: the regulator
core still defers a supply whose provider is registered but not yet
bound. Such a PI is not retried until the next PSE_REGISTERED or a phy
re-registration.

The REGISTERED walk runs inside pse_controller_register(), so it
attaches handles part-way through the PSE driver's own probe. For a PI
the hardware already has powered, pse_control_get_internal() records
admin_state_enabled and takes the exclusive regulator. If that probe
then fails after registration - tps23881 requests its irq after
devm_pse_controller_register() - devres unwinds into the UNREGISTERED
walk, the last reference goes, and __pse_control_release() disables
the PI. A port that was up before the driver loaded is then powered
down when that driver fails to finish probing, the same as on a clean
unbind.

The UNREGISTERED walk does not help rmmod: 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.

The walk relies on the teardown order pse_controller_unregister()
already has: the controller is 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, both because
that release path can queue work and because the worker's 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. It could not be added earlier, while the
fwnode_mdio hook gave each matching phy a handle that nothing released
on controller unbind.

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   | 144 ++++++++++++++++++++++++++++++++-
 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, 261 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..497cf179398b 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,116 @@ 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) is silent,
+ * and so is -EPROBE_DEFER. That mostly means the controller is not
+ * registered yet, which the notifier retries at PSE_REGISTERED time, but
+ * a registered controller returns it too while its vpwr provider is
+ * registered and not yet bound, and that is not retried until the next
+ * PSE_REGISTERED. Anything else is warned about and left non-fatal, so
+ * the phy still registers - it is not necessarily a broken binding, it
+ * can equally be -ENOMEM or the PI already being held, and nothing
+ * retries it until the next PSE_REGISTERED.
+ *
+ * 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 +1142,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 +1182,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 +4111,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 +4135,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 e560833ad034..bfa025e2194b 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
@@ -1345,6 +1392,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);
@@ -2206,3 +2260,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 v8 2/7] 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 (2) · Medium: 1 (2) · Low: 1

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] Work can be re-queued after cancel_work_sync() by pse_control
  holders that do not subscribe (baseline decision: discarded, fixed…
- [Medium] A port could be powered on inside PSE_UNREGISTERED while the
  irq is masked (baseline decision: discarded).
- [Low] Fixes: tag missing for the teardown race fixed by the reorder
  (baseline decision: discarded).

Pre-existing issues:
- [High] Shared PSE power domain is freed by devres while other
  controllers still use it (pre-existing).
- [High] Stale pse_control handles reach freed pcdev->pi and pcdev after
  unbind (pre-existing, baseline decision: discarded, fixed later in the…
- [Medium] disable_irq() without a matching enable on a line the PSE core
  may not own (pre-existing, baseline decision: discarded).
- [Medium] pse_controller_register() leaks memory on its error paths
  (pre-existing, baseline decision: discarded).

--

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

This patch hooks up the pse_controller_notifier chain.
pse_controller_register() now fires PSE_REGISTERED once the controller is
on pse_controller_list, and pse_controller_unregister() fires
PSE_UNREGISTERED after the controller has been unlinked. It also reorders
pse_controller_unregister(): the irq is disabled and the controller is
unlinked before the event, and cancel_work_sync() and the frees move
after it.

> No subscriber exists yet, so the event itself does nothing. The
> reordering of pse_controller_unregister() around it is not a no-op,
> though: it closes a teardown race that is reachable today, with no
> subscriber involved.

[Severity: Low]
The race described here is real. In the baseline, pse_release_pis() frees
pcdev->pi while the controller is still on pse_controller_list. That lets
of_pse_control_get()->of_pse_match_pi() read the freed pcdev->pi[i].np.

The notes say a separate net series reorders the same function as a
standalone fix. That looks like the right place for a Fixes: tag, because
this commit depends on the notifier from "net: pse-pd: add notifier chain
for controller lifecycle events".

Once the net fix lands, will this paragraph be out of date? Could the
message refer to the net fix instead of saying that this commit closes
the race?

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 84c734ed4553e..dc261beb61702 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;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the error paths in
pse_controller_register() leak memory. Every failure after this call
returns without kfifo_free():

	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);

Failures after of_load_pse_pis() also leak pcdev->pi and its of_node
references. A partial pse_register_pw_ds() leaks pw_d krefs and supplies.

The next patch in the series, "net: pse-pd: unwind allocations when
controller registration fails", adds the free_kfifo and release_pis
labels. It also calls pse_flush_pw_ds() when pse_register_pw_ds() fails.
It still leaks the PI array on purpose once PI regulators exist.

kfifo_alloc() is sized from pcdev->nr_lines before the nr_lines = 1
default is applied. No in-tree driver registers with nr_lines below 2.
Should the kfifo be sized after that default is set?

> @@ -1148,15 +1151,41 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>   */
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
> -	pse_flush_pw_ds(pcdev);
> -	pse_release_pis(pcdev);
> +	/* Raise the interrupt's disable depth before anything is freed.
> +	 * pse_isr() queues notifications and reaches pcdev->pi, and nothing
> +	 * below re-enables it. For a driver that requests its irq after
> +	 * devm_pse_controller_register(), devres has already run free_irq()
> +	 * by the time we get here and this only bumps the depth - the
> +	 * ordering does not rely on that, so a driver requesting the irq
> +	 * earlier is covered too.
> +	 */
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);

[Severity: Medium]
This is a pre-existing issue, but can this disable_irq() mask an
interrupt line that the PSE core never owned?

devm_pse_irq_helper() records the irq even when the request fails:

drivers/net/pse-pd/pse_core.c:devm_pse_irq_helper() {
    ...
	ret = devm_request_threaded_irq(dev, irq, NULL, pse_isr,
					IRQF_ONESHOT | irq_flags,
					irq_name, h);
	if (ret)
		dev_err(pcdev->dev, "Failed to request IRQ %d\n", irq);

	pcdev->irq = irq;
	return ret;
}

Suppose tps23881's request fails with -EBUSY because another device holds
the line exclusively. Probe then unwinds through
devm_pse_controller_release(), and this disable_irq() masks the other
device's interrupt. Nothing re-enables it afterwards.

The new comment is correct for the in-tree success case. tps23881 passes
irq_flags = 0, devres has already run free_irq(), and the next request
goes through irq_startup(), which resets the depth to 0.

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

[Severity: Medium]
With disable_irq() now running before the event, can a port be powered on
from inside the PSE_UNREGISTERED walk?

A subscriber's final put can follow this path for a pending PI on the
same power domain:

__pse_control_release()
  regulator_disable()
    _pse_pi_disable()
      pse_pw_d_retry_power_delivery()
        ops->pi_enable()

That decision uses an isr_pd_detected value that can no longer change,
because the irq is already masked.

The commit message says this is intended ("so the call is legal"). In
the final series the power-on also looks brief. A PI can only be pending
after pse_pi_enable() on the exclusive psec->ps consumer, so it always
has a holder. The same walk releases every PHY holder through
__pse_control_release()->regulator_disable()->pse_pi_disable(), and that
clears admin_state_enabled.

Leaving a port powered on for good would need a pending PI with no
holder. Only an earlier failed ops->pi_disable() could produce that. Is
that case worth handling, or is the brief power-on acceptable here?

> +
> +	/* 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]
At this point in the series, can ntf_work be queued again after this
drain?

Nothing here blocks pse_control holders that do not subscribe. A PHY
handle from fwnode_mdio can still take this path after
cancel_work_sync():

pse_ethtool_set_config() or pse_ethtool_set_prio()
  regulator op
    _pse_pi_disable()
      pse_disable_pi_pol()
        kfifo_in_spinlocked(&pcdev->ntf_fifo, &ntf, 1, &pcdev->ntf_fifo_lock);
        schedule_work(&pcdev->ntf_work);

The fifo and the work are freed just below. A requeued worker's
pse_control_put()->__pse_control_release() would also read the freed
pcdev->pi[psec->id].

The commit message mentions this ("The drain is not yet final on its
own"). The baseline freed pcdev->pi before cancel_work_sync(), so this
ordering is no worse.

The last patch in the series, "net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook", closes the window:

- The PSE_UNREGISTERED walk clears and puts every phydev->psec under
  pse_phy_lock().
- net/ethtool/pse-pd.c takes pse_phy_lock() around
  pse_get_pse_attributes() and the whole of ethnl_set_pse().
- A WARN_ON(!list_empty(&pcdev->pse_control_head)) follows the drain.

> +
> +	pse_flush_pw_ds(pcdev);

[Severity: High]
This isn't a bug introduced by this patch, but can a shared power domain
be freed here while another controller still uses it?

pse_register_pw_ds() allocates a new domain with
devm_pse_alloc_pw_d(pcdev->dev). That is a devm_kzalloc() on the device
of whichever controller (A) registered first. A second controller (B)
whose PI regulators share the same parent supply only takes a reference:

drivers/net/pse-pd/pse_core.c:pse_register_pw_ds() {
    ...
			if (regulator_is_equal(pw_d->supply, rdev->supply)) {
				present = true;
				pcdev->pi[i].pw_d = pw_d;
				break;
			}
		}
		if (present) {
			kref_get(&pw_d->refcnt);
			continue;
		}
    ...
}

When A unbinds, pse_flush_pw_ds() only drops A's own references, for
example from 2 to 1. __pse_pw_d_release() does not run, so the pw_d stays
in pse_pw_d_map and in B->pi[i].pw_d.

devm_pse_controller_register() calls devres_add() for the unregister
cookie only after pse_controller_register() returns. Devres therefore
frees the pw_d allocation right after pse_controller_unregister(A)
returns.

B and later controllers then use the freed pw_d:

- pse_pw_d_is_sw_pw_control() reads pw_d->budget_eval_strategy.
- pse_pi_allocate_pw_budget() and pse_pi_deallocate_pw_budget() use
  pw_d->supply.
- B's own pse_flush_pw_ds() does xa_load(&pse_pw_d_map,
  pcdev->pi[i].pw_d->id), then kref_put_mutex() on the freed kref.
- A third controller's pse_register_pw_ds() walks pse_pw_d_map and reads
  pw_d->supply.

The later patch "net: pse-pd: unwind allocations when controller
registration fails" says it does not fix shared power domains ("devres
frees the creator's pw_d underneath it anyway"). At the end of the series
pse_flush_pw_ds() only clears the departing controller's own pi[].pw_d.
B's pointer and the xarray entry still point at freed memory.

> +	pse_release_pis(pcdev);

[Severity: High]
This is a pre-existing issue, but at this commit, can PHY-held
pse_control handles reach the freed pcdev->pi and pcdev after unbind?

PHYs keep the handle they got from fwnode_mdio until phy_device_remove(),
and nothing subscribes to PSE_UNREGISTERED yet. Once this frees pcdev->pi
and devres frees pcdev, psec->pcdev points at freed memory.

Unprivileged users can reach this through the ETHTOOL_MSG_PSE_GET path.
pse_ethtool_get_status() dereferences pcdev->ops and pcdev->pi[psec->id].

The final pse_control_put() from phy_device_remove() also runs this on
freed memory:

drivers/net/pse-pd/pse_core.c:__pse_control_release() {
    ...
	if (psec->pcdev->pi[psec->id].admin_state_enabled)
		regulator_disable(psec->ps);
	regulator_put(psec->ps);

	module_put(psec->pcdev->owner);
    ...
}

The commit message says this stays open until phylib releases its
handles. The last patch in the series, "net: phy: own phydev->psec via
PSE notifier and remove fwnode_mdio hook", fixes it:

- fwnode_find_pse_control() is removed.
- During PSE_UNREGISTERED, phy_pse_detach_one() clears and puts each
  matching phydev->psec before cancel_work_sync() and the frees.
- pse_prepare_data(), ethnl_set_pse() and phy_device_remove() hold
  pse_phy_lock().

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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] The use-after-free the patch says it closes is still reachable
  when two controllers register concurrently.
- [Medium] The subject says the patch unwinds allocations when
  registration fails.
- [Medium] pse_flush_pw_ds() now stores NULL to pcdev->pi[i].pw_d (lines
  941 and 954) so that nothing is left pointing at the devm-allocated…
- [Low] The patch fixes a real memory-safety bug but carries no Fixes:
  tag and no stable Cc.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the podl-pse-
  regulator driver can never register a PSE controller.

--

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

This patch adds error unwinding to pse_controller_register(). The
notification kfifo is now freed on failure, and early failures also free
the PI array and its OF references. Power domains left by a partial
pse_register_pw_ds() are flushed, pse_flush_pw_ds() now clears pi[].pw_d,
and failures from the PI regulator loop onward deliberately leave the PI
array allocated.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index dc261beb61702..eeefbf25e6716 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -937,11 +937,21 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
>  			continue;
>  
>  		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
> -		if (!pw_d)
> +		if (!pw_d) {
> +			pcdev->pi[i].pw_d = NULL;
>  			continue;
> +		}
>  
>  		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
>  			       &pse_pw_d_mutex);
> +		/* The pw_d is devm memory of whichever controller created
> +		 * it, so it can go away as soon as that probe unwinds.
> +		 * Nothing may be left pointing at it: pse_pi_is_enabled()
> +		 * reaches pi[].pw_d from the regulator "state" attribute,
> +		 * which stays readable until the PI regulators are
> +		 * unregistered after us.
> +		 */
> +		pcdev->pi[i].pw_d = NULL;

[Severity: Medium]
Is this store enough to keep pse_pi_is_enabled() away from a freed pw_d?
The store is done without pcdev->lock, but the reader holds that lock:

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

On the new pse_register_pw_ds() failure path the PI regulators are still
live. That means the world-readable state attribute can reach this at any
time:

state_show()->_regulator_is_enabled()->pse_pi_is_enabled()

Say a reader has loaded pcdev->pi[id].pw_d and is preempted before
pse_pw_d_is_sw_pw_control() reads pw_d->budget_eval_strategy. In that gap,
pse_flush_pw_ds() can clear the field and return, and the probe fails.
devres then frees the pw_d before it unregisters the PI regulators,
because the pw_d was allocated after them. The reader then reads freed
memory.

Would it close this window to take pcdev->lock around the pointer clear
and do the kref_put_mutex() calls afterwards, outside pcdev->lock?

There is a second race in the intermediate state of the series.
pse_ethtool_get_status() checks pi->pw_d, calls the sleeping
pi_get_admin_state(), and then does:

	switch (pi->pw_d->budget_eval_strategy) {

If that races with pse_controller_unregister(), the new unlocked NULL
store turns this into a NULL dereference. Later in the series, "net: phy:
own phydev->psec via PSE notifier and remove fwnode_mdio hook" closes
that part: the ethtool PSE paths take pse_phy_lock(), and psec is
detached before the flush. The pse_pi_is_enabled() window is still there
at the end of the series.

[ ... ]

> @@ -1092,17 +1102,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 is a pre-existing issue and was not introduced by this patch, but
this patch reworks the kfifo handling here. Can the kfifo ever be
allocated for podl-pse-regulator? Earlier in pse_controller_register():

	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
	if (ret) {
		dev_err(pcdev->dev, "failed to allocate kfifo notifications\n");
		return ret;
	}
	INIT_WORK(&pcdev->ntf_work, pse_send_ntf_worker);

	if (!pcdev->nr_lines)
		pcdev->nr_lines = 1;

The kfifo size comes from nr_lines before the default is applied.
__kfifo_alloc_node() rounds the size up to a power of two and returns
-EINVAL if the result is below 2, and both 0 and 1 end up there.

pse_reg_probe() in drivers/net/pse-pd/pse_regulator.c never sets
nr_lines before it calls devm_pse_controller_register(). That probe
therefore seems to always fail with "failed to allocate kfifo
notifications". A controller with nr_lines == 1 fails the same way.

Would it be better to size the kfifo from the defaulted nr_lines with a
minimum of 2, for example max(nr_lines, 2)? This came in with
ffef61d6d273 ("net: pse-pd: Add support for budget evaluation
strategies"), and it is still the same at the end of the series.

[ ... ]

> @@ -1110,7 +1121,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 +1135,27 @@ 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]
Doesn't this still leak pcdev->pi and the OF references on the PI and
pairset nodes? That happens on every failure from here on: the reg_name
allocation, this one, and the pse_register_pw_ds() failure below. It even
happens on the first loop iteration, before any PI regulator exists.

On registration failure, devm_pse_controller_register() frees only its
devres cookie, so nothing else releases the array.

The commit message says:

    Freeing the array safely past the loop
    would need the regulator ops to tolerate a NULL pcdev->pi, which they do
    not today.

Is that really the only way to do it? devm_pse_pi_regulator_register()
registers the PI regulators on pcdev->dev with devm_regulator_register().
A devm action on pcdev->dev that releases the PIs could be added right
after of_load_pse_pis() succeeds. Devres releases in LIFO order, so that
action would run after every PI regulator has been unregistered. No
regulator op would ever see a freed or NULL pcdev->pi.

That would also cover pse_controller_unregister(), which the commit
message notes calls pse_release_pis() while the PI regulators are still
registered.

The later patch "net: pse-pd: check the PI vpwr supply before registering
the controller" moves the common -EPROBE_DEFER case ahead of the loop.
The leaks from the loop and from pse_register_pw_ds() are still there at
the end of the series.

>  
>  	ret = pse_register_pw_ds(pcdev);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		/* Deliberately not release_pis: the PI regulators registered
> +		 * above index pcdev->pi[] and outlive this function.
> +		 */
> +		pse_flush_pw_ds(pcdev);
> +		goto free_kfifo;
> +	}

[Severity: High]
Can a controller probing at the same time still hit the use-after-free
the commit message describes? On its error path, pse_register_pw_ds()
drops pse_pw_d_mutex while the domains it just created are still in
pse_pw_d_map:

out:
	mutex_unlock(&pse_pw_d_mutex);
	return ret;

pse_flush_pw_ds() runs only after that, without the mutex. Here is one
possible sequence, where controller B shares the same vpwr supply:

controller A
pse_register_pw_ds()
  devm_pse_alloc_pw_d()       /* pw_d published in pse_pw_d_map */
  regulator_get() fails on a later PI
  mutex_unlock(&pse_pw_d_mutex)

controller B
pse_register_pw_ds()
  regulator_is_equal(pw_d->supply, rdev->supply) matches
  pcdev->pi[i].pw_d = pw_d
  kref_get(&pw_d->refcnt)     /* 1 -> 2 */

controller A
pse_flush_pw_ds()
  kref_put_mutex()            /* 2 -> 1, no xa_erase() */
devres frees pw_d

Now pse_pw_d_map and B's pi[].pw_d both point at freed memory. The next
pse_register_pw_ds() reads pw_d->supply, and B's own flush later does
kref_put on the freed object.

Every domain the failing call created stays private to it until
pse_pw_d_mutex is dropped. Could pse_register_pw_ds() undo those domains
itself before the out: label, doing kref_put/xa_erase/regulator_put while
it still holds pse_pw_d_mutex? That would close this window without
moving pw_d out of devm memory.

[Severity: Low]
Should this patch carry Fixes: tags and a stable Cc? A partial
pse_register_pw_ds() leaves freed power domains in pse_pw_d_map, so this
fixes a real use-after-free. The commit message has only a Signed-off-by.

According to blame, the bare returns this patch replaces came from these
commits:

- after pse_register_pw_ds(): 50f8b341d268 ("net: pse-pd: Add support
  for PSE power domains")
- kfifo_alloc(): ffef61d6d273 ("net: pse-pd: Add support for budget
  evaluation strategies")
- after of_load_pse_pis(): 9be9567a7c59 ("net: pse-pd: Add support for
  PSE PIs")
- after setup_pi_matrix(): 29e28d1d7a16 ("net: pse-pd: Add support for
  setup_pi_matrix callback")

Could this also go to net on its own, rather than sit inside the
notifier chain / phylib ownership series?

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] The supply pre-check can still pass when the regulator core
  will defer the same PI.
- [Medium] This patch adds an -EPROBE_DEFER exit in
  pse_controller_register() (pse_core.c:1226-1227 and 1238-1240) that…
- [Low] The commit message leaves out a present-day behaviour change.
- [Low] The concern says the commit message relies on an unreachable
  premise: no in-tree consumer retries only on PSE_REGISTERED, and…

--

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

pse_controller_register() now checks each PI's vpwr supply after
setup_pi_matrix() and before any PI regulator is registered. A missing
provider therefore makes the PSE driver's own probe defer. Like
regulator_resolve_supply(), the check looks at the PI node first and then
at the controller device.

> 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. A
> consumer that only retries when a controller registers - which is what
> phylib becomes once it attaches from the PSE notifier - then never gets
> its PI.

[Severity: Low]
This isn't a bug, but the commit message only describes the future
phylib notifier case. It leaves out a change that applies today.

Before this patch, a PI whose vpwr provider was not registered yet when
pse_controller_register() ran got a PI regulator with
rdev->supply == NULL. pse_register_pw_ds() is called only once, and it
skips that PI:

	if (!rdev || !rdev->supply)
		continue;

So pcdev->pi[i].pw_d stayed NULL for good. That disabled budget
evaluation, power allocation and priority for that PI:
pse_pw_d_is_sw_pw_control() returns false, and pse_ethtool_set_prio()
returns -EOPNOTSUPP with "no power domain attached". This happened even
with the current fwnode_mdio consumer, which does get the PI once the
provider appears.

With the new check the controller defers until the supply is there, so
these PIs now get a power domain.

Could the commit message mention this? It matters when deciding on a
Fixes: tag or a stable backport.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index eeefbf25e6716..e560833ad0340 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -860,6 +861,98 @@ static const struct regulator_ops pse_pi_ops = {

[ ... ]

> +static int pse_pi_check_supply(struct pse_controller_dev *pcdev, int id)
> +{

[ ... ]

> +	of_node_put(np);
> +	supply = of_regulator_get_optional(pcdev->dev, pcdev->pi[id].np,
> +					   "vpwr");
> +	if (!IS_ERR(supply)) {
> +		regulator_put(supply);
> +		return 0;
> +	}

[Severity: Medium]
Can this check pass while the PI regulator registration still defers its
supply?

of_regulator_get_optional() here, and regulator_get_optional() in
pse_controller_check_supply(), both go through _regulator_get_common().
That path never checks whether the provider's parent device is bound.
The resolution done when the PI regulator is registered does check it:

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

Consider a PSE probe that overlaps the probe of an external vpwr
provider. The provider has called regulator_register() but
driver_bound() has not run yet, or the provider later fails its probe
and unregisters. This check then returns 0, and the PI regulators
register with rdev->supply == NULL. PSE_REGISTERED then fires, and
of_pse_control_get()->regulator_get_exclusive() returns -EPROBE_DEFER
for that PI.

Later in the series, "net: phy: own phydev->psec via PSE notifier and
remove fwnode_mdio hook" makes phylib retry only on PSE_REGISTERED.
Wouldn't phylib then miss this PI until the next PSE_REGISTERED or a PHY
re-registration? Later patches in the series do not change these check
functions.

The commit message mentions this window, but not a second effect that
does not go away on its own. pse_register_pw_ds() is called only once,
from pse_controller_register(), and skips a PI whose supply is not
resolved:

	if (!rdev || !rdev->supply)
		continue;

Does pcdev->pi[i].pw_d then stay NULL for that PI even after the
regulator core resolves the supply later? Budget, power allocation and
priority handling would stay off until the controller is re-bound.

The tps23881 and si3474 bindings put vpwr-supply in the pse-pi nodes.
As the commit message notes, that only creates a SYNC_STATE_ONLY
fw_devlink link, which does not hold back the controller's probe. Async
probing or module loads on different CPUs could reach this.

[ ... ]

> @@ -1116,6 +1210,38 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  			goto release_pis;
>  	}
>  
> +	/* Check every PI supply before any regulator of ours is registered:

[ ... ]

> +	for (i = 0; i < pcdev->nr_lines; i++) {
> +		ret = pse_pi_check_supply(pcdev, i);
> +		if (ret < 0)
> +			goto release_pis;

[Severity: Medium]
Does this new -EPROBE_DEFER exit leak the pd692x0 power budget claimed on
the vmain regulator?

This exit, and the one after pse_controller_check_supply(), run after
pcdev->ops->setup_pi_matrix(). On pd692x0, setup_pi_matrix() has already
done two things that release_pis does not undo:

pd692x0_setup_pi_matrix()
  pd692x0_req_managers_pw_budget()
    regulator_request_power_budget(supply, pw_budget)

Also, when !priv->cfg_saved, it programs the MCU power bank and port
matrix and saves the user byte.

The budget is released only from setup_pi_matrix()'s own error labels or
from pd692x0_i2c_remove(). pd692x0_i2c_probe() just returns the error:

	ret = devm_pse_controller_register(dev, &priv->pcdev);
	if (ret)
		return dev_err_probe(dev, ret,

Regulator put and unregister do not lower rdev->pw_requested_mW; only
regulator_free_power_budget() does. So each deferred retry adds another
claim on vmain.

Suppose vmain has regulator-power-budget-milliwatt set. On retry,
regulator_get_unclaimed_power_budget() returns 0, so manager_pw_budget[i]
stays 0. cfg_saved is now true, so the MCU is not reprogrammed and keeps
the first attempt's budget, and the driver and hardware disagree. With
PSE_BUDGET_EVAL_STRAT_DYNAMIC, could enabling any PI whose supply chains
through a manager to vmain then fail with -ERANGE in
regulator_request_power_budget()?

The documented binding points each pse-pi vpwr-supply at the driver's
own manager regulators, so it does not hit this. The schema does not
restrict the phandle target, though. A pd692x0 board with one PI on an
external provider that has not probed yet would hit this on every retry.

The commit message acknowledges this and leaves the cleanup to the
driver. This patch adds a deferral at this point that is retried by
design. Should the pd692x0 budget cleanup come with it in this series?

[ ... ]

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

Re: [PATCH net-next v8 6/7] of: property: do not let "pses" block a consumer's probe

[netdev-bot+sashiko]

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

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

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] At this commit only, the full PHY driver probe and remove now
  run on every deferral of the MDIO bus owner while the PSE controller…
- [Medium] At this commit only, deferred probing can loop without end
  while the PSE controller is absent.
- [Low] At this commit the commit message describes phylib behaviour that
  does not exist yet.

--

of: property: do not let "pses" block a consumer's probe

This marks the "pses" supplier binding in of_supplier_bindings[] with
FWLINK_FLAG_IGNORE, so fw_devlink no longer creates a device link from a
PHY to its PSE controller. The goal is to stop a MAC or DSA switch from
binding genphy while the PHY's own driver is held back by that link,
once phylib gets PSE handles from a notifier.

> The link is not needed for correctness. PSE is not a resource the
> consumer must have before it probes: phylib looks a PI up when its
> controller becomes available and releases it when the controller goes
> away, and of_pse_control_get() is the only reader of the property.

[Severity: Low]
Is this description accurate at this point in the series?

At this commit, pse_register_notifier() in drivers/net/pse-pd/pse_core.c
has no callers. The only PSE consumer is still
fwnode_mdiobus_register_phy()->fwnode_find_pse_control(). It takes the
handle once, when the PHY is registered, and the handle is only released
in phy_device_remove():

drivers/net/phy/phy_device.c:phy_device_remove() {
    ...
	pse_control_put(phydev->psec);
    ...
}

The same applies to this later paragraph:

> binds. Both are given up on purpose: phylib's notifier takes the PI
> away on unbind and hands it back on bind without tearing the PHY driver
> down, and with no deferral left there is nothing for an autoprobe to
> wait for.

With only this patch applied, the managed link that passed a PSE
controller unbind on to the PHY driver is gone, and nothing replaces it.

The replacement comes in the final patch of the series, "net: phy: own
phydev->psec via PSE notifier and remove fwnode_mdio hook". That patch
adds phy_pse_notifier and removes fwnode_find_pse_control().

Could the message say that a later patch adds the notifier behaviour, so
it reads correctly for anyone who bisects or backports to this commit?

> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 72cf12907de03..5ec0f05b87ac3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1564,7 +1564,10 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_backlight, },
>  	{ .parse_prop = parse_panel, },
>  	{ .parse_prop = parse_msi_parent, },
> -	{ .parse_prop = parse_pses, },
> +	{
> +		.parse_prop = parse_pses,
> +		.fwlink_flags = FWLINK_FLAG_IGNORE,
> +	},

[Severity: Medium]
fwnode_mdio still does the PSE lookup at this point. Does that mean every
deferral of the MDIO bus owner now runs a full PHY driver probe and
remove, including the hardware setup done in probe?

With the link ignored, fw_devlink_create_devlink() returns early:

drivers/base/core.c:fw_devlink_create_devlink() {
    ...
	if (link->flags & FWLINK_FLAG_IGNORE)
		return 0;
    ...
}

So device_links_check_suppliers() no longer holds the PHY's probe back.

fwnode_mdiobus_register_phy() still registers the PHY before the lookup,
and PHY drivers are PROBE_FORCE_SYNCHRONOUS. While the PSE controller is
unbound, each retry therefore goes through:

fwnode_mdiobus_register_phy()
  fwnode_mdiobus_phy_device_register()
    phy_device_register()
      device_add()
        phy_probe()            <- deasserts reset, calls drv->probe
  fwnode_find_pse_control()
    of_pse_control_get()       <- returns -EPROBE_DEFER
  phy_device_remove()
    phy_remove()
      phy_device_reset(phydev, 1)

Some probe routines do real work here. For example, aqr107_probe() loads
firmware through aqr_firmware_load(), and that now happens again on every
retry until the PSE controller binds.

The commit message mentions the probe/remove cycle, but not the repeated
reset toggling or firmware loading.

The cycle goes away later in the series with "net: phy: own phydev->psec
via PSE notifier and remove fwnode_mdio hook". That patch removes
fwnode_find_pse_control() from fwnode_mdiobus_register_phy(). A bisect or
partial backport that stops at this commit would still hit the cycle.

[Severity: Medium]
Can this also make deferred probing loop forever while the PSE controller
is absent?

The PHY driver now binds inside device_add() during the MAC or DSA probe,
so driver_bound() fires a deferred probe trigger:

drivers/base/dd.c:driver_bound() {
    ...
	driver_deferred_probe_del(dev);
	driver_deferred_probe_trigger();
    ...
}

Next, the PSE lookup returns -EPROBE_DEFER, the PHY is removed, and the
MAC/DSA probe returns -EPROBE_DEFER. driver_probe_device() then sees that
a trigger happened while it was probing:

drivers/base/dd.c:driver_probe_device() {
    ...
		if (trigger_count != atomic_read(&deferred_trigger_count) &&
		    !defer_all_probes)
			driver_deferred_probe_trigger();
    ...
}

That puts the MAC straight back on the active list, and
deferred_probe_work_func() picks it up again.

The PSE controller may never register, for example if its driver is not
built, its probe fails, or one of its own suppliers is missing. In that
case of_pse_control_get() keeps returning -EPROBE_DEFER and the loop never
ends.

Before this patch, device_links_check_suppliers() deferred the PHY's
probe. driver_bound() therefore did not run, and the trigger count did
not change on these retries. That holds at least on buses where every
PHY with a driver has a pses link.

The final patch of the series, "net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook", removes the PSE lookup from
fwnode_mdiobus_register_phy(), and the loop goes with it.

Would it be better to apply this change after that patch, so that no
intermediate commit behaves like this?

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

Re: [PATCH net-next v8 7/7] 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 4 potential
issue(s) to consider.

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

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] pse_control_get_internal() leaks a reference on the PSE
  controller module whenever pse_pi_is_hw_enabled() fails.
- [Medium] phy_try_attach_pse() drops -EPROBE_DEFER silently and relies
  on a later PSE_REGISTERED event to retry.
- [Medium] The PSE_REGISTERED walk now attaches PHY handles from inside
  pse_controller_register(), partway through the PSE driver's probe.
- [Low] The commit fixes two reported, user-visible problems, both
  recorded with Reported-by/Closes: the deferred-probe storm in which…

Pre-existing issues:
- [High] Use-after-free of phy_device in the PSE notification worker.
- [High] This problem wasn't introduced by this patch, but
  pse_controller_unregister() still frees pcdev->pi (pse_release_pis()…

--

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

This patch takes the pses phandle lookup out of
fwnode_mdiobus_register_phy() and gives ownership of phydev->psec to
phylib. Handles are now attached and released from the PSE controller
lifecycle notifier and from phy_device_register()/phy_device_remove(),
serialised by a new pse_phy_lock() mutex in pse_core. The goal is to stop
the -EPROBE_DEFER retry storm seen with modular PSE controller drivers.

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

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

[Severity: Low]
Should this have a Fixes: tag? Both Closes: reports describe the deferred
probe storm. That storm comes from the register-then-remove-on-defer code
in fwnode_mdiobus_register_phy(), which was added by commit fa2f0454174c
("net: pse-pd: Introduce attached_phydev to pse control"). The
phy->psec = psec assignment itself goes back to 5e82147de1cb.

This fix also depends on earlier patches in the series:

  net: pse-pd: add notifier chain for controller lifecycle events
  net: pse-pd: fire lifecycle events on controller register/unregister
  net: pse-pd: check the PI vpwr supply before registering the controller
  of: property: do not let "pses" block a consumer's probe

None of them has a Fixes or stable marker either, so backporters have no
sign that the series must be taken together. If this is meant for
net-next only, could the commit message say so?

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

[ ... ]

> @@ -1012,9 +1013,116 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)

[ ... ]

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

[Severity: Medium]
Can this retry leak a reference on the PSE controller module? When
pse_pi_is_hw_enabled() fails, pse_control_get_internal() jumps past
module_put():

drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
	...
	if (!try_module_get(pcdev->owner)) {
		ret = -ENODEV;
		goto free_psec;
	}

	if (!pcdev->ops->pi_get_admin_state) {
		ret = -EOPNOTSUPP;
		goto free_psec;
	}
	...
	ret = pse_pi_is_hw_enabled(pcdev, index);
	if (ret < 0)
		goto free_psec;
	...
put_module:
	module_put(pcdev->owner);
free_psec:
	kfree(psec);
	...
}

The missing module_put() is older than this patch. What changes here is
that the error only produces a warning and phydev->psec stays NULL. The
same phy is then tried again on every phy_device_register() and, through
phy_pse_attach_one(), on every PSE_REGISTERED walk, including walks for
unrelated controllers. Each attempt leaks another module reference.

pd692x0 hits this every time. It registers the controller even when its
firmware is broken or needs an update, and pd692x0_pi_get_admin_state()
then returns an error from pd692x0_fw_unavailable(). Bus errors on
tps23881, si3474 or realtek-pse-mcu would have the same effect.

Should the pi_get_admin_state check and the pse_pi_is_hw_enabled() error
path jump to put_module instead?

[Severity: Medium]
Does a later PSE_REGISTERED event really retry every -EPROBE_DEFER? A
controller that is already registered and matched can still return it:

pse_control_get_internal()
  regulator_get_exclusive()
    _regulator_get_common()
      regulator_resolve_supply()
        if (!device_is_bound(r->dev.parent)) {
            ...
            ret = -EPROBE_DEFER;

This happens when the vpwr provider has registered its regulator but has
not finished binding. pse_pi_check_supply() uses
of_regulator_get_optional(), which does not check binding, so the
controller registers anyway.

When the provider finishes binding, nothing calls phy_try_attach_pse()
again. The port then reports "No PSE is attached" until another PSE
controller registers or the phy is re-registered.

Before this patch, the same -EPROBE_DEFER came back out of
fwnode_mdiobus_register_phy(), and the MAC/DSA probe recovered once the
provider was bound. The commit message mentions this case. Is there a way
to retry it, for example when the supply becomes available?

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

[ ... ]

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

[Severity: Medium]
Can a failed PSE driver probe now power down a port that was already
live? The PSE_REGISTERED walk runs synchronously inside
pse_controller_register(), so it attaches handles before the PSE driver's
probe has finished. For a PI the hardware already reports as powered,
pse_control_get_internal() sets pcdev->pi[index].admin_state_enabled.

The probe can still fail after registration. One example is
tps23881_i2c_probe():

	ret = devm_pse_controller_register(dev, &priv->pcdev);
	...
	return tps23881_setup_irq(priv, client->irq);

Another is firmware_upload_register() in pd692x0. In that case devres runs
pse_controller_unregister(), the PSE_UNREGISTERED walk drops the last
reference, and __pse_control_release() does:

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

That reaches ops->pi_disable() and cuts PoE power on the port. Before this
patch, a failed PSE probe did not power down a port this way.

The commit message documents this behaviour. Could handles be attached
only after the PSE driver's probe has completed?

> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}

[ ... ]

> @@ -1061,8 +1182,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);
> +

[ ... ]

> +	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 isn't a bug introduced by this patch, but the new pse_phy_lock()
scheme does not cover psec->attached_phydev. Can pse_send_ntf_worker()
read a freed phy_device here?

The worker holds only a transient kref and rtnl:

pse_send_ntf_worker()
  psec = pse_control_find_by_id(pcdev, ntf.id);
  rtnl_lock();
  netdev = pse_control_get_netdev(psec);
    return psec->attached_phydev->attached_dev;

phy_device_remove() can run at the same time outside rtnl, for example
from mdiobus_unregister() in a MAC's remove after unregister_netdev().
The pse_control_put() above is then not the final put, because the worker
still holds a reference. psec survives with attached_phydev still pointing
at this phy, and device_del() followed by phy_device_free() frees the
phydev while the worker can still dereference it.

Nothing clears psec->attached_phydev when the owning phy drops its
reference. Also, when two phys share a PI, pse_control_get_internal()
returns the existing psec through kref_get() and leaves attached_phydev
unchanged.

[ ... ]

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index e560833ad0340..bfa025e2194bb 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -1345,6 +1392,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);

[Severity: High]
This is a pre-existing issue, but pse_release_pis() still frees pcdev->pi
here while the PI regulators that index it are registered. Can a read of
a PI regulator's state attribute touch the freed array?

devm_pse_pi_regulator_register() calls devm_regulator_register(pcdev->dev,
...) from inside pse_controller_register(). devm_pse_controller_register()
adds its own devres after that. Because devres releases in reverse order,
devm_pse_controller_release() runs and frees pcdev->pi before the
regulators are unregistered.

During that window, reading /sys/class/regulator/*/state for one of these
PIs reaches:

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

That reads pw_d and admin_state_enabled from freed memory. The earlier
series patch "net: pse-pd: unwind allocations when controller
registration fails" mentions this read of freed memory but does not fix
it. The new PSE_UNREGISTERED walk and WARN_ON clean up the control
handles, but the PI array still does not outlive the regulators.

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