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

[Paolo Abeni <pabeni@redhat.com>]

On 9/27/26 21:18, Carlo Szelinsky wrote:
> This is v7 of Corey's series [1]. It takes the PSE controller lookup out
> of the MDIO probe path, so a modular PSE controller driver no longer makes
> the PHY (and any DSA switch behind it) spin on -EPROBE_DEFER until the PSE
> module loads.
> 
> v6 [7] drew a large AI review [8], which I have now answered point by point
> in that thread. Paolo's reading was right: most of the [High] findings
> described the state of the tree between the old patches 3 and 5, which the
> old patch 5 then fixed. Rather than argue that in five changelogs, v7 folds
> those three patches into one, so the rtnl detour and the deferred release
> never exist at any commit. That also removes a real bisect hazard: the old
> patch 3 on its own hung lantiq_etop and sni_ave on probe, and the old patch
> 4 was what repaired it.
> 
> Per Documentation/process/maintainer-netdev.rst I ran LLM review over v7
> before posting, more than once. Patches 3 and 4 exist because of what it
> found, and it changed patch 2 and the phy patch as well:
> 
> Patch 2 reorders pse_controller_unregister() around the new event, because
> a subscriber runs arbitrary teardown inside it. The controller is unlinked
> from pse_controller_list first, so a lookup racing the teardown resolves
> nothing rather than a controller whose pcdev->pi[] pse_release_pis() is
> about to free. v6 disclosed that as pre-existing and reachable only from
> phy registration; it is reachable from more than that here, because after
> the last patch a second controller registering runs a PSE_REGISTERED walk
> that calls of_pse_control_get() for every phy on mdio_bus_type, which is
> exactly the two-controller board I test on. disable_irq() moves up for the
> same reason: pse_isr() queues notifications and reaches pcdev->pi.
> 
> cancel_work_sync() moves the other way, to after the event rather than
> before it. A subscriber dropping the last pse_control reference reaches
> __pse_control_release(), which calls regulator_disable() if the PI is
> still on. With the static budget strategy that retries any port on the
> same power domain waiting for power, and if the domain is still over
> budget it sheds a lower priority port through pse_disable_pi_pol(),
> which queues a notification and calls schedule_work(). Draining the
> worker before the walk would leave work queued behind it, racing the
> kfifo_free() below. Draining after it also stops the worker's own
> transient reference, taken by pse_control_find_by_id(), from becoming
> the last one once pse_release_pis() has freed the array.
> 
> [9] makes a related reordering for net, independently of any
> subscriber, so pse_controller_unregister() will conflict when [9]
> back-merges. The order is not identical: [9] leaves the unlink below
> cancel_work_sync() and pse_flush_pw_ds(), which it can, having no event
> to place. Here the event has to sit after the unlink and before the
> frees, and cancel_work_sync() after the event, so the unlink moves to
> the top. The merged function wants this order, which contains [9]'s fix:
> 
>      if (pcdev->irq)
>              disable_irq(pcdev->irq);
>      mutex_lock(&pse_list_mutex);
>      list_del(&pcdev->list);
>      mutex_unlock(&pse_list_mutex);
>      blocking_notifier_call_chain(&pse_controller_notifier,
>                                   PSE_UNREGISTERED, pcdev);
>      cancel_work_sync(&pcdev->ntf_work);
>      pse_flush_pw_ds(pcdev);
>      pse_release_pis(pcdev);
>      kfifo_free(&pcdev->ntf_fifo);
> 
> I am happy to send that as a follow-up on top of the merge if that is
> easier than carrying it in the conflict.
> 
> Kory, a specific ask on patch 2. The reason cancel_work_sync() sits below
> the event and not above it is that __pse_control_release() can re-enter
> your budget code: regulator_disable() on a PI that is still on runs
> _pse_pi_disable(), and with the static strategy that retries a pending
> port on the same power domain and, if the domain is still over budget,
> sheds a lower priority one through pse_disable_pi_pol() - which queues a
> notification and calls schedule_work() from inside the walk. I have
> tested that path rather than only reasoned about it, but the ordering
> rests on your design, so I would rather you looked at it than have it
> ride in unremarked.
> 
> Patch 4: each PSE PI regulator is registered with a "vpwr" supply. The
> regulator core deliberately treats an unresolved supply at registration as
> non-fatal, so pse_controller_register() completes and PSE_REGISTERED fires
> for a controller whose PIs cannot be handed out yet: regulator_get_exclusive()
> in pse_control_get_internal() resolves the supply itself and keeps
> returning -EPROBE_DEFER until the vpwr provider appears. Before this series
> the MDIO layer propagated that and deferred probe retried it. After it,
> phylib has no event left to retry on, and the port would silently lose PSE
> for good. Patch 4 checks every PI's supply before registering anything, so
> the PSE driver's own probe defers and deferred probe handles the ordering.
> It checks exactly the PIs the registration loop creates a regulator for,
> including a controller with no pse-pis node, and it follows both stages
> the core uses - the PI node, then the controller device - because a
> vpwr-supply written once on the controller node is invisible from the PI
> node but resolves at stage two. It stops short of the core's
> device_is_bound() gate, so a probe interleaving with the provider's own
> can still resolve late; the changelog says so.
> 
> Patch 3: that makes -EPROBE_DEFER an ordinary return from
> pse_controller_register(), which has no error unwind at all. The kfifo and
> the PI array plus its OF references are leaked on every failure, once today
> and on each retry after patch 4, and a partial pse_register_pw_ds() leaves
> devm-allocated power domains in the global xarray for the next registration
> to trip over. Patch 3 adds the unwind, at two depths: pse_pi_ops index
> pcdev->pi[], and the PI regulators are devm-registered, so once one exists
> the array cannot be freed here at all and stays leaked as it is today. It
> is released on the failures that happen while it exists and before the
> first PI regulator does - setup_pi_matrix() and the supply check - which is
> where the ordinary deferral now lands.
> 
> The same reviews caught two things in the phy patch. The error paths of
> phy_device_register() could leak a handle: device_add() puts the phy on
> the klist before its own later failure points, so a PSE_REGISTERED walk
> can attach one that nothing releases. A put at the out: label does not
> work, because by then device_add() has unwound the phy off the bus and
> the PSE_UNREGISTERED walk would miss it too. phydev->psec_detached now
> covers registration as well as removal, so no handle is attached in that
> window. And that flag is a plain bool rather than another bit in the
> flags word, since it is written under pse_phy_lock() while its
> neighbours are written under phydev->lock and rtnl.
> 
> No Fixes: tag. 5e82147de1cb ("net: mdiobus: search for PSE nodes by
> parsing PHY nodes") is the commit to blame, but this is a refactor
> across two subsystems plus a new export, and tagging it would invite a
> stable backport of all that to cure a probe-retry loop.
> 
> Two changelog errors from v6 are also fixed: netsec does not deadlock (its
> MDIO bus comes up in probe, not from ndo_init), and the module-unload
> rationale was backwards (try_module_get() pins the provider, so rmmod is
> refused before the unregister path ever runs).
> 
> How it works: pse_core gets a notifier chain (REGISTERED / UNREGISTERED).
> phylib subscribes, owns phydev->psec, and attaches the handle when the
> controller shows up instead of during probe. fwnode_mdio loses its PSE
> awareness, so no -EPROBE_DEFER leaves it and the probe-retry loop is gone.
> 
> On the tags: Jonas tested the v4 shape and Aleksander tested the v6
> locking, which is unchanged here. Neither tested the fixes above. On the
> folded phy patch the code they exercised is intact, so I have kept their
> tags there. Jonas's tag also rides on patch 2, and that one did change in
> v7 - pse_controller_unregister() is reordered around the event - so it is
> the weakest of the three. Patch 1 only gained a kernel-doc correction. I
> would rather say so here than let it pass silently; happy to drop any of
> them if either would prefer.
> 
> Tested on a Realtek rtl9303 PoE switch with an HS104 PSE controller on
> i2c, with a PD drawing power on one port:
> 
>   - clean boot, no probe-retry loop, the controller registers once
>   - rmmod is refused while a phy holds a handle
>   - i2c unbind: the notifier walk drops the handle and the port powers
>     down, and ethtool reports no PSE attached
>   - i2c bind: the handle comes back and the PD is powered again
>   - six unbind/bind cycles, no warning, power domain index stable
> 
> Also exercised under QEMU, on arm64 under KASAN, PROVE_LOCKING and
> kmemleak. The device tree has a PSE controller, a second one whose vpwr
> provider never appears, and an MDIO bus with two phys, only one of which
> references a PI. Unbinding the controller detaches that phy's handle and
> rebinding re-attaches it; the other phy is never touched; unbinding the
> MDIO bus releases a live handle; and the controller with the missing
> supply defers instead of registering. A third controller puts its
> vpwr-supply on the controller node rather than the PI nodes, which the
> core resolves one stage later - that one has to defer too, and on the
> code before patch 4's second stage it registers instead. The new
> WARN_ON in patch 5 stays silent across six unbind cycles and kmemleak
> reports nothing.
> 
> The same test setup stages an over-budget static-priority domain, so that
> dropping the last reference really does reach pse_disable_pi_pol() and
> schedule_work() from inside the walk - the case patch 2's
> cancel_work_sync() placement exists for. I checked that with a
> dump_stack() rather than by reasoning about it: releasing phy1's handle
> in the walk lands in _pse_pi_disable(), the retry picks a pending port on
> the same domain, the domain is short, and a lower priority port is shed.
> No lockdep splat, which is the result I wanted most: that path re-enters
> the regulator core from a notifier callback, under the chain's rwsem and
> pse_phy_mutex.
> 
> Build matrix, all linking a real vmlinux: PHYLIB=y, PHYLIB=m (the config
> that failed to link in v5), PHYLIB=n, PSE_CONTROLLER=n, and CONFIG_OF=n.
> 
> Tested-by: Carlo Szelinsky <github@szelinsky.de>
More feedback from clashiko. Low prio remarks, items addressed later
in the series and ask for fixes tag could be ignored, but still a few
things that look real there.

/P