* [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
@ 2026-09-06 15:30 ` Carlo Szelinsky
2026-09-09 6:33 ` netdev-bot+sashiko
2026-09-06 15:30 ` [PATCH net-next v6 2/5] net: pse-pd: fire lifecycle events on controller register/unregister Carlo Szelinsky
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Carlo Szelinsky @ 2026-09-06 15:30 UTC (permalink / raw)
To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel, Carlo Szelinsky
From: Corey Leavitt <corey@leavitt.info>
Introduce a blocking notifier chain that allows other subsystems to be
informed when a PSE controller is registered or unregistered, and
provide pse_register_notifier() / pse_unregister_notifier() as the
subscriber interface.
Subsequent patches will use this to let the phy subsystem own the
phydev->psec lifecycle directly, decoupling PSE lookup from
fwnode_mdiobus_register_phy() and removing the probe-time
-EPROBE_DEFER coupling that currently exists between mdio, phy and
pse-pd when the PSE controller driver is modular.
A blocking chain (rather than atomic) is used because callbacks will
take rtnl_lock and call back into pse_core via of_pse_control_get().
The enum pse_controller_event is placed outside the
IS_ENABLED(CONFIG_PSE_CONTROLLER) guard so that subscribers compiled
into a kernel without PSE support can still reference the event
values in dead-code paths without breaking the build.
This patch is pure infrastructure: nothing fires events yet, and
nothing subscribes. No observable behavior change.
Signed-off-by: Corey Leavitt <corey@leavitt.info>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
Tested-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
drivers/net/pse-pd/pse_core.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/pse-pd/pse.h | 32 ++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 69dbdbde9d71..3c4d09f1d6e4 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..78fe3a2b1ea8 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,24 @@ 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 about to be removed from
+ * pse_controller_list. 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 +385,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 +438,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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events
2026-09-06 15:30 ` [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events Carlo Szelinsky
@ 2026-09-09 6:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 6:33 UTC (permalink / raw)
To: github
Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
edumazet, kuba, pabeni, corey, jelonek.jonas, horms, olek2,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] Claim: the patch adds a blocking notifier chain and two
EXPORT_SYMBOL_GPL() entry points…
--
net: pse-pd: add notifier chain for controller lifecycle events
This adds a blocking notifier chain to pse_core along with
pse_register_notifier() and pse_unregister_notifier() as the subscriber
interface, plus enum pse_controller_event describing PSE_REGISTERED and
PSE_UNREGISTERED. Nothing fires the chain and nothing subscribes to it in
this patch.
No Link: tags present in the commit header.
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 69dbdbde9d71a..3c4d09f1d6e4f 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
[ ... ]
> @@ -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);
[Severity: Low]
This isn't a bug, just a note on something that stood out while reading the
patch on its own.
At this commit, pse_register_notifier() and pse_unregister_notifier() in
drivers/net/pse-pd/pse_core.c are exported with no in-tree caller, and the
event-data contract documented for enum pse_controller_event has no producer
yet, so the documented semantics cannot be checked from this patch alone.
Following the series forward, the producer side shows up in the next commit,
with pse_controller_register() and pse_controller_unregister() doing:
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_REGISTERED, pcdev);
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
and the first subscriber lands in phy_init() in drivers/net/phy/phy_device.c:
rc = pse_register_notifier(&phy_pse_notifier);
Both call sites pass pcdev, which matches what the kerneldoc for enum
pse_controller_event promises, and the commit message states outright that
nothing fires or subscribes yet.
So nothing to fix here, this is the usual "add the API, then use it" split.
[ ... ]
> diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
> index 4e5696cfade7c..78fe3a2b1ea84 100644
> --- a/include/linux/pse-pd/pse.h
> +++ b/include/linux/pse-pd/pse.h
[ ... ]
> @@ -366,6 +385,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
[ ... ]
No other regressions found. The CONFIG_PSE_CONTROLLER=n stubs returning 0
match the existing stub style in include/linux/pse-pd/pse.h, and placing
enum pse_controller_event outside the IS_ENABLED() guard is consistent with
the stated goal of letting subscribers reference the values in dead-code
paths.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v6 2/5] net: pse-pd: fire lifecycle events on controller register/unregister
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
2026-09-06 15:30 ` [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events Carlo Szelinsky
@ 2026-09-06 15:30 ` Carlo Szelinsky
2026-09-09 6:33 ` netdev-bot+sashiko
2026-09-06 15:31 ` [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook Carlo Szelinsky
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Carlo Szelinsky @ 2026-09-06 15:30 UTC (permalink / raw)
To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel, Carlo Szelinsky
From: Corey Leavitt <corey@leavitt.info>
Hook the newly-introduced pse_controller_notifier chain so that
pse_controller_register() fires PSE_REGISTERED after the controller
has been added to pse_controller_list (i.e. is now resolvable by
of_pse_control_get()), and pse_controller_unregister() fires
PSE_UNREGISTERED before the controller is removed from the list
(while it is still valid to dereference from a subscriber's
pse_control pointer targeting it).
With no subscribers yet, this is observably a no-op. A later change
wires the phy subsystem in as the first subscriber.
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 | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 3c4d09f1d6e4..0190fac33c78 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,6 +1151,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
*/
void pse_controller_unregister(struct pse_controller_dev *pcdev)
{
+ blocking_notifier_call_chain(&pse_controller_notifier,
+ PSE_UNREGISTERED, pcdev);
+
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev);
if (pcdev->irq)
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v6 2/5] net: pse-pd: fire lifecycle events on controller register/unregister
2026-09-06 15:30 ` [PATCH net-next v6 2/5] net: pse-pd: fire lifecycle events on controller register/unregister Carlo Szelinsky
@ 2026-09-09 6:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 6:33 UTC (permalink / raw)
To: github
Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
edumazet, kuba, pabeni, corey, jelonek.jonas, horms, olek2,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1
- [High] pse_controller_unregister() fires PSE_UNREGISTERED while @pcdev
is still linked in pse_controller_list and only calls list_del() at…
- [Low] Reported concern (assessed as not a bug today): both new call
sites use blocking_notifier_call_chain() and discard the return value;…
Pre-existing issues:
- [High] pse_controller_unregister() calls pse_release_pis() (which
kfree()s pcdev->pi) at pse_core.c:1158, but only drains the…
--
net: pse-pd: fire lifecycle events on controller register/unregister
This hooks the new pse_controller_notifier chain into the PSE core, firing
PSE_REGISTERED after the controller is added to pse_controller_list and
PSE_UNREGISTERED before it is removed. A later patch in the series wires
the phy subsystem in as the first subscriber.
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 3c4d09f1d6e4f..0190fac33c78a 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);
> +
[Severity: Low]
This isn't a bug today, but both new call sites discard the return value of
blocking_notifier_call_chain(). notifier_call_chain() stops walking as soon
as a callback returns a value containing NOTIFY_STOP_MASK, so a subscriber
returning NOTIFY_STOP or NOTIFY_BAD would hide PSE_UNREGISTERED from
subscribers registered later in the chain, and their pse_control objects
would then outlive pse_release_pis().
The only in-tree subscriber added by this series, phy_pse_notifier_event(),
returns NOTIFY_OK or NOTIFY_DONE, so the chain cannot currently be
truncated.
Would it be worth checking notifier_to_errno() (or at least warning) here,
and/or warning in pse_controller_unregister() when pcdev->pse_control_head
is not empty after the broadcast?
> return 0;
> }
> EXPORT_SYMBOL_GPL(pse_controller_register);
> @@ -1148,6 +1151,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
> */
> void pse_controller_unregister(struct pse_controller_dev *pcdev)
> {
> + blocking_notifier_call_chain(&pse_controller_notifier,
> + PSE_UNREGISTERED, pcdev);
> +
[Severity: High]
Can a new pse_control still be handed out for this controller after the
event has been broadcast?
PSE_UNREGISTERED is emitted while pcdev is still linked in
pse_controller_list, and list_del() only happens at the very end of
pse_controller_unregister(), after pse_flush_pw_ds(), pse_release_pis(),
disable_irq() and kfifo_free().
of_pse_control_get() gates resolution purely on list membership plus
try_module_get(pcdev->owner):
list_for_each_entry(r, &pse_controller_list, list) {
if (!r->no_of_pse_pi) {
ret = of_pse_match_pi(r, args.np);
There is no "dying" state that of_pse_control_get() rejects, and
try_module_get() succeeds in the common case since unregister runs from the
devm release on device unbind while the provider module is still loaded.
The subscriber added later in the series does not close the window either:
phy_pse_notifier_event() takes and then releases pse_phy_mutex, while the
concurrent acquirer path is:
phy_device_register()
pse_phy_lock()
phy_try_attach_pse()
of_pse_control_get()
pse_phy_unlock()
so the lock does not cover the rest of the teardown. Two outcomes look
possible:
If the attach wins before pse_release_pis(), the phy stores a pse_control
whose psec->pcdev->pi[] array is freed moments later, and the final put
reads 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);
devm_regulator_put(psec->ps);
module_put(psec->pcdev->owner);
If the lookup runs after pse_release_pis(), of_pse_match_pi() dereferences
pcdev->pi[i].np directly, and pse_release_pis() does not clear the pointer:
drivers/net/pse-pd/pse_core.c:pse_release_pis() {
...
kfree(pcdev->pi);
}
The header text added by the series says subscribers "must not acquire new
references for it" for PSE_UNREGISTERED, but non-subscriber lookup paths can
still resolve the controller.
Would it be more robust to make the controller unresolvable first (list_del()
or a dying flag checked by of_pse_control_get()) and only then broadcast
PSE_UNREGISTERED?
> pse_flush_pw_ds(pcdev);
> pse_release_pis(pcdev);
> if (pcdev->irq)
[Severity: High]
This isn't a bug introduced by this patch, but the ordering here becomes
easier to hit with the new broadcast, so it seems worth raising.
pse_release_pis() frees pcdev->pi, while the notification worker is only
drained afterwards:
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev);
if (pcdev->irq)
disable_irq(pcdev->irq);
cancel_work_sync(&pcdev->ntf_work);
pse_send_ntf_worker() takes a real reference via pse_control_find_by_id():
if (psec->id == id) {
kref_get(&psec->refcnt);
then drops pse_list_mutex and sleeps across rtnl_lock() /
ethnl_pse_send_ntf() / rtnl_unlock() before calling pse_control_put().
With PSE_UNREGISTERED now telling the phy subscriber to drop its reference
first, can the in-flight worker end up being the last holder, so that its
pse_control_put() runs __pse_control_release() against the already freed
pcdev->pi:
if (psec->pcdev->pi[psec->id].admin_state_enabled)
regulator_disable(psec->ps);
Should disable_irq() and cancel_work_sync() run before pse_release_pis(), so
the IRQ producer and the worker are stopped and drained before controller
state is released?
> disable_irq(pcdev->irq);
> cancel_work_sync(&pcdev->ntf_work);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
2026-09-06 15:30 ` [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events Carlo Szelinsky
2026-09-06 15:30 ` [PATCH net-next v6 2/5] net: pse-pd: fire lifecycle events on controller register/unregister Carlo Szelinsky
@ 2026-09-06 15:31 ` Carlo Szelinsky
2026-09-09 6:33 ` netdev-bot+sashiko
2026-09-06 15:31 ` [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach Carlo Szelinsky
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Carlo Szelinsky @ 2026-09-06 15:31 UTC (permalink / raw)
To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel, Carlo Szelinsky
From: Corey Leavitt <corey@leavitt.info>
Transfer ownership of phydev->psec from fwnode_mdio to the phy
subsystem itself. The phy subsystem now subscribes to the pse-pd
notifier chain and manages psec attach/detach in response to PSE
controller lifecycle events, while fwnode_mdio loses its PSE awareness
entirely.
phydev->psec is attached after device_add() has made the phy visible
on mdio_bus_type, under a narrow rtnl_lock() that covers only
phy_try_attach_pse(). Ordering the attach after registration closes
the race that would otherwise leave a phy unattached: a PSE_REGISTERED
event firing during registration walks mdio_bus_type and either finds
the phy already added (and attaches it) or runs before device_add(),
in which case the post-add attach resolves it. The phydev->psec check
in phy_try_attach_pse() makes the two paths idempotent. Holding rtnl
across of_pse_control_get() is safe because pse_list_mutex is never
taken in the opposite order.
device_add() is deliberately left outside rtnl. Binding a phy that
itself provides an SFP cage reaches sfp_bus_add_upstream() through
phy_probe() -> phy_setup_ports() -> phy_sfp_probe(), and
sfp_bus_add_upstream() takes rtnl_lock(); holding rtnl across
device_add() would deadlock such phys (reported on RTL8214FC).
phy_device_register() is split into the public form, which takes the
narrow rtnl_lock() around the attach, and a phy_device_register_locked()
form for callers that already hold rtnl (the SFP module state machine
via __sfp_sm_event). This pair mirrors the register_netdevice() /
register_netdev() split convention already established in the core
networking stack. The _locked form runs device_add() under the
caller's rtnl, which is safe because a phy resident on an SFP module
does not itself provide a downstream cage, so phy_sfp_probe() is a
no-op there.
- On PSE_REGISTERED: an rtnl-guarded bus walk retries the attach 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 boot-time probe-retry storm on systems with a modular
PSE controller driver.
- On PSE_UNREGISTERED: an rtnl-guarded bus walk releases every
phydev->psec that targets the departing controller before
pse_release_pis() frees pcdev->pi. Without this, a phy still
holding a pse_control reference would cause a use-after-free in
__pse_control_release()'s pcdev->pi[psec->id] access, and the PSE
driver module could not finish unloading while any phy still held a
reference.
A bad `pses` binding -- an error from of_pse_control_get() other than
-ENOENT (no phandle) or -EPROBE_DEFER (controller not yet registered)
-- is reported with phydev_warn() rather than silently dropped,
preserving the diagnostic that the removed fwnode_mdio lookup used to
provide.
The final pse_control_put() of phydev->psec moves from
phy_device_remove() to phy_device_release(), so it runs only after
every reference on the device -- including the bus-iterator references
taken by bus_for_each_dev() in the notifier walk -- has been dropped.
Finally, delete fwnode_find_pse_control() and its call site in
fwnode_mdiobus_register_phy(), and drop the PSE header from
fwnode_mdio.c. The MDIO/DSA probe no longer sees any PSE-originated
-EPROBE_DEFER, so the probe-retry storm is gone and fwnode_mdio is
now PSE-agnostic.
Reported-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Closes: https://lore.kernel.org/netdev/e00048dd-1ed3-40c3-9912-59bccf015ad5@gmail.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>
---
drivers/net/mdio/fwnode_mdio.c | 34 -------
drivers/net/phy/phy_device.c | 168 +++++++++++++++++++++++++++++++--
drivers/net/phy/sfp.c | 2 +-
drivers/net/pse-pd/pse_core.c | 14 +++
include/linux/phy.h | 2 +
include/linux/pse-pd/pse.h | 9 ++
6 files changed, 186 insertions(+), 43 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 0615228459ef..f5febff4b00b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -223,8 +223,19 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
static void phy_device_release(struct device *dev)
{
+ struct phy_device *phydev = to_phy_device(dev);
+
+ /* bus_for_each_dev() holds get_device() across each iteration
+ * step, deferring this release callback until any in-flight PSE
+ * notifier walk has advanced past this phy. pse_control_put()
+ * takes pse_list_mutex, so this path must run in sleepable
+ * context.
+ */
+ might_sleep();
+ pse_control_put(phydev->psec);
+
fwnode_handle_put(dev->fwnode);
- kfree(to_phy_device(dev));
+ kfree(phydev);
}
static void phy_mdio_device_remove(struct mdio_device *mdiodev)
@@ -1102,11 +1113,103 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
}
EXPORT_SYMBOL(get_phy_device);
-/**
- * phy_device_register - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
+/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
+ * Caller must hold rtnl. A missing phandle (-ENOENT) or a not-yet-registered
+ * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
+ * PSE_REGISTERED time. Any other error means a broken binding and is warned
+ * about, but left non-fatal so the phy still registers.
*/
-int phy_device_register(struct phy_device *phydev)
+static void phy_try_attach_pse(struct phy_device *phydev)
+{
+ struct pse_control *psec;
+ struct device_node *np;
+
+ ASSERT_RTNL();
+
+ np = phydev->mdio.dev.of_node;
+ if (!np)
+ return;
+
+ if (phydev->psec)
+ 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 __maybe_unused)
+{
+ ASSERT_RTNL();
+
+ 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;
+
+ ASSERT_RTNL();
+
+ 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:
+ rtnl_lock();
+ bus_for_each_dev(&mdio_bus_type, NULL, NULL,
+ phy_pse_attach_one);
+ rtnl_unlock();
+ return NOTIFY_OK;
+ case PSE_UNREGISTERED:
+ rtnl_lock();
+ bus_for_each_dev(&mdio_bus_type, NULL, data,
+ phy_pse_detach_one);
+ rtnl_unlock();
+ return NOTIFY_OK;
+ default:
+ return NOTIFY_DONE;
+ }
+}
+
+static struct notifier_block phy_pse_notifier __read_mostly = {
+ .notifier_call = phy_pse_notifier_event,
+};
+
+/* Core registration: add the phy to the MDIO bus. Does not touch rtnl or
+ * PSE. phydev->psec is attached by the callers below, 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. Keeping
+ * device_add() out of rtnl also avoids deadlocking when binding a phy that
+ * itself provides an SFP cage (phy_probe() -> phy_sfp_probe() ->
+ * sfp_bus_add_upstream() takes rtnl).
+ */
+static int __phy_device_register(struct phy_device *phydev)
{
int err;
@@ -1135,10 +1238,54 @@ int phy_device_register(struct phy_device *phydev)
out:
/* Assert the reset signal */
phy_device_reset(phydev, 1);
-
mdiobus_unregister_device(&phydev->mdio);
return err;
}
+
+/**
+ * phy_device_register_locked - Register the phy device on the MDIO bus
+ * @phydev: phy_device structure to be added to the MDIO bus
+ *
+ * Same as phy_device_register() but caller must already hold rtnl_lock().
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int phy_device_register_locked(struct phy_device *phydev)
+{
+ int err;
+
+ ASSERT_RTNL();
+
+ err = __phy_device_register(phydev);
+ if (err)
+ return err;
+
+ phy_try_attach_pse(phydev);
+
+ return 0;
+}
+EXPORT_SYMBOL(phy_device_register_locked);
+
+/**
+ * phy_device_register - Register the phy device on the MDIO bus
+ * @phydev: phy_device structure to be added to the MDIO bus
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int phy_device_register(struct phy_device *phydev)
+{
+ int err;
+
+ err = __phy_device_register(phydev);
+ if (err)
+ return err;
+
+ rtnl_lock();
+ phy_try_attach_pse(phydev);
+ rtnl_unlock();
+
+ return 0;
+}
EXPORT_SYMBOL(phy_device_register);
/**
@@ -1152,8 +1299,6 @@ EXPORT_SYMBOL(phy_device_register);
void phy_device_remove(struct phy_device *phydev)
{
unregister_mii_timestamper(phydev->mii_ts);
- pse_control_put(phydev->psec);
-
device_del(&phydev->mdio.dev);
/* Assert the reset signal */
@@ -3981,8 +4126,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:
@@ -3999,6 +4150,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/phy/sfp.c b/drivers/net/phy/sfp.c
index 03bfd8640db9..18868bdd6485 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2083,7 +2083,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
/* Mark this PHY as being on a SFP module */
phy->is_on_sfp_module = true;
- err = phy_device_register(phy);
+ err = phy_device_register_locked(phy);
if (err) {
phy_device_free(phy);
dev_err(sfp->dev, "phy_device_register failed: %pe\n",
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 0190fac33c78..611540df43bb 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -2021,3 +2021,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 199a7aaa341b..865b9baddb85 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2158,6 +2158,8 @@ struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
+/* Caller must hold rtnl_lock(); see phy_device_register() for the public form. */
+int phy_device_register_locked(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
void phy_device_remove(struct phy_device *phydev);
int phy_get_c45_ids(struct phy_device *phydev);
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 78fe3a2b1ea8..d4310ca71a3e 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -385,6 +385,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);
+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);
@@ -438,6 +441,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;
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook
2026-09-06 15:31 ` [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook Carlo Szelinsky
@ 2026-09-09 6:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 6:33 UTC (permalink / raw)
To: github
Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
edumazet, kuba, pabeni, corey, jelonek.jonas, horms, olek2,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 2 · Medium: 2 · Low: 2
- [High] Deferred psec put vs. bus-walk-based detach: moving the final
pse_control_put(phydev->psec) from phy_device_remove() to…
- [High] The PSE_UNREGISTERED detach walk does not actually close the
use-after-free class the commit claims to fix: the notifier callback…
- [Medium] pse_controller_unregister() frees the PI storage before
quiescing the notification worker, and the new PSE_UNREGISTERED detach…
- [Medium] phy_try_attach_pse() treats every of_pse_control_get() error
other than -ENOENT/-EPROBE_DEFER as permanent and warn-only, but that…
- [Low] The commit message claims that without the new PSE_UNREGISTERED
detach walk "the PSE driver module could not finish unloading while…
- [Low] The patch fixes a reported, user-visible regression (boot-time
probe-retry storm / PHYs failing to register when the PSE controller…
--
net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook
This moves ownership of phydev->psec from fwnode_mdio into phylib, which
now subscribes to the pse-pd notifier chain and attaches/detaches psec on
PSE controller register/unregister events. phy_device_register() is split
into a public form that takes rtnl around the attach and a
phy_device_register_locked() form for callers already holding rtnl, and
the final pse_control_put() moves from phy_device_remove() to
phy_device_release().
Closes: https://lore.kernel.org/netdev/e00048dd-1ed3-40c3-9912-59bccf015ad5@gmail.com/
A couple of questions on the changelog first.
[Severity: Low]
> - On PSE_UNREGISTERED: an rtnl-guarded bus walk releases every
> phydev->psec that targets the departing controller before
> pse_release_pis() frees pcdev->pi. Without this, a phy still
> holding a pse_control reference would cause a use-after-free in
> __pse_control_release()'s pcdev->pi[psec->id] access, and the PSE
> driver module could not finish unloading while any phy still held a
> reference.
Is the module unload part of this rationale the right way round?
pse_control_get_internal() pins the controller driver for every psec:
if (!try_module_get(pcdev->owner)) {
ret = -ENODEV;
goto free_psec;
}
and the matching module_put() only happens in __pse_control_release().
While a phy holds a psec the module refcount is non-zero, so
delete_module() fails with -EWOULDBLOCK before the module exit path (and
therefore pse_controller_unregister(), the only emitter of
PSE_UNREGISTERED) is ever entered.
The detach walk does help for driver unbind and device removal, where
pse_controller_unregister() is reached with the module still loaded, but
it cannot make an rmmod that was already refused succeed. Could that
sentence be reworded to talk about unbind/removal instead?
[Severity: Low]
> Reported-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> Closes: https://lore.kernel.org/netdev/e00048dd-1ed3-40c3-9912-59bccf015ad5@gmail.com/
This carries Reported-by:, Closes: and Tested-by: for a user-visible
regression (PHYs failing to register / probe-retry storm with a modular
PSE controller driver), but there is no Fixes: tag and no indication of
the target tree.
git blame on the removed lookup in fwnode_mdiobus_register_phy() points at
5e82147de1cbd7 for the original PSE lookup, later touched by
fa2f0454174c2f, so the affected releases can be named. Could a Fixes: tag
be added, and could the intended tree (net vs net-next) be stated given
the size of the refactor and the new exported symbol?
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0615228459ef4..f5febff4b00b8 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -223,8 +223,19 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
>
> static void phy_device_release(struct device *dev)
> {
> + struct phy_device *phydev = to_phy_device(dev);
> +
> + /* bus_for_each_dev() holds get_device() across each iteration
> + * step, deferring this release callback until any in-flight PSE
> + * notifier walk has advanced past this phy. pse_control_put()
> + * takes pse_list_mutex, so this path must run in sleepable
> + * context.
> + */
> + might_sleep();
> + pse_control_put(phydev->psec);
> +
> fwnode_handle_put(dev->fwnode);
> - kfree(to_phy_device(dev));
> + kfree(phydev);
> }
[Severity: High]
Can a psec survive here past the point where the detach walk can find it?
Once phy_device_remove() has run, device_del() -> bus_remove_device() ->
klist_del() takes the phy off the mdio_bus_type klist, but the phy itself
can still be pinned by a get_device() from phy_attach_direct() or
of_phy_find_device(). The detach is driven only by the klist walk:
case PSE_UNREGISTERED:
rtnl_lock();
bus_for_each_dev(&mdio_bus_type, NULL, data,
phy_pse_detach_one);
so an off-bus phy keeps its psec, while pse_controller_unregister()
continues straight on:
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev); /* kfree(pcdev->pi) */
When the last device reference finally drops and this release callback
runs, __pse_control_release() does:
if (psec->pcdev->pi[psec->id].admin_state_enabled)
regulator_disable(psec->ps);
which reads the freed pi array and may act on it.
The last patch of this series ("net: phy: release phydev->psec from
phy_device_remove() again") restores the put plus phydev->psec = NULL
under pse_phy_lock() in phy_device_remove() before device_del(), which is
the ordering that avoids this. Would it be better to keep the put in
phy_device_remove() from this patch onwards so the intermediate tree is
not left with the window open?
> @@ -1102,11 +1113,103 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> }
> EXPORT_SYMBOL(get_phy_device);
>
> -/**
> - * phy_device_register - Register the phy device on the MDIO bus
> - * @phydev: phy_device structure to be added to the MDIO bus
> +/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
> + * Caller must hold rtnl. A missing phandle (-ENOENT) or a not-yet-registered
> + * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
> + * PSE_REGISTERED time. Any other error means a broken binding and is warned
> + * about, but left non-fatal so the phy still registers.
> */
> -int phy_device_register(struct phy_device *phydev)
> +static void phy_try_attach_pse(struct phy_device *phydev)
> +{
> + struct pse_control *psec;
> + struct device_node *np;
> +
> + ASSERT_RTNL();
> +
> + np = phydev->mdio.dev.of_node;
> + if (!np)
> + return;
> +
> + if (phydev->psec)
> + 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;
> +}
[Severity: Medium]
Is every error other than -ENOENT and -EPROBE_DEFER really a broken
binding? of_pse_control_get() does hardware traffic on this path:
drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
...
ret = pse_pi_is_hw_enabled(pcdev, index);
if (ret < 0)
goto free_psec;
pcdev->pi[index].admin_state_enabled = ret;
...
psec->ps = devm_regulator_get_exclusive(...);
}
pse_pi_is_hw_enabled() calls pcdev->ops->pi_get_admin_state(), which is an
I2C/SMBus read on tps23881, si3474 and pd692x0, so a transient -EIO,
-EBUSY or -ENOMEM is possible. Here that is only warned about, phydev->psec
stays NULL, and the only retry left is a fresh PSE_REGISTERED event.
That is aggravated by PSE_REGISTERED firing synchronously from the tail of
pse_controller_register(), i.e. mid-probe of the controller driver:
drivers/net/pse-pd/tps23881.c:tps23881_probe() {
...
ret = devm_pse_controller_register(dev, &priv->pcdev);
...
ret = tps23881_setup_irq(priv, client->irq);
}
Previously the same failure propagated out of fwnode_mdiobus_register_phy()
and deferred probe retried it. With this change, does a single transient
bus error permanently lose the PSE binding for that port, leaving the PI
without a consumer and unreachable from ethtool even though
admin_state_enabled may have been seeded from the chip?
> +
> +static int phy_pse_attach_one(struct device *dev, void *data __maybe_unused)
> +{
> + ASSERT_RTNL();
> +
> + 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;
> +
> + ASSERT_RTNL();
> +
> + 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;
> +}
[Severity: Medium]
Once this walk drops the phy's reference, can the notification worker end
up being the last holder of the psec?
pse_send_ntf_worker() takes its own reference and then blocks on rtnl:
psec = pse_control_find_by_id(pcdev, ntf.id);
rtnl_lock();
netdev = pse_control_get_netdev(psec);
...
rtnl_unlock();
pse_control_put(psec);
The detach walk runs under rtnl, so it can complete in that window and the
psec then survives only on the worker's reference. Meanwhile
pse_controller_unregister() frees the PI storage before quiescing the
worker:
pse_release_pis(pcdev); /* kfree(pcdev->pi) */
if (pcdev->irq)
disable_irq(pcdev->irq);
cancel_work_sync(&pcdev->ntf_work);
so the worker's pse_control_put() reaches __pse_control_release() and reads
psec->pcdev->pi[psec->id].admin_state_enabled after the array is gone. The
still-enabled IRQ can also queue more notification work before
cancel_work_sync(). Should disable_irq() and cancel_work_sync() move ahead
of pse_flush_pw_ds()/pse_release_pis()?
> +
> +static int phy_pse_notifier_event(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + switch (event) {
> + case PSE_REGISTERED:
> + rtnl_lock();
> + bus_for_each_dev(&mdio_bus_type, NULL, NULL,
> + phy_pse_attach_one);
> + rtnl_unlock();
> + return NOTIFY_OK;
> + case PSE_UNREGISTERED:
> + rtnl_lock();
> + bus_for_each_dev(&mdio_bus_type, NULL, data,
> + phy_pse_detach_one);
> + rtnl_unlock();
> + return NOTIFY_OK;
> + default:
> + return NOTIFY_DONE;
> + }
> +}
[Severity: High]
Does the detach walk actually close the use-after-free the changelog
describes, given that rtnl is dropped before the notifier returns?
pse_controller_unregister() unlinks the controller last:
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev); /* kfree(pcdev->pi) */
...
mutex_lock(&pse_list_mutex);
list_del(&pcdev->list);
In the window after the notifier returns and before that list_del(), a phy
registering on another CPU runs phy_device_register() ->
phy_try_attach_pse() -> of_pse_control_get(), still finds the departing
pcdev on pse_controller_list, and creates a fresh psec on it. That path
touches the already freed pi array: of_pse_match_pi() reads
pcdev->pi[i].np, and pse_control_get_internal() reads pcdev->pi[index].rdev
and writes pcdev->pi[index].admin_state_enabled = ret.
Since the attach decision is taken under rtnl but the rest of the teardown
is not, this looks like a check-then-attach race against controller
teardown. Would unlinking pcdev from pse_controller_list before the
notifier fires, or publishing a "dying" state under the same lock the
attach uses, be needed here?
> @@ -1152,8 +1299,6 @@ EXPORT_SYMBOL(phy_device_register);
> void phy_device_remove(struct phy_device *phydev)
> {
> unregister_mii_timestamper(phydev->mii_ts);
> - pse_control_put(phydev->psec);
> -
> device_del(&phydev->mdio.dev);
>
> /* Assert the reset signal */
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
` (2 preceding siblings ...)
2026-09-06 15:31 ` [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook Carlo Szelinsky
@ 2026-09-06 15:31 ` Carlo Szelinsky
2026-09-09 6:33 ` netdev-bot+sashiko
2026-09-06 15:31 ` [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again Carlo Szelinsky
2026-09-10 9:06 ` [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Paolo Abeni
5 siblings, 1 reply; 12+ messages in thread
From: Carlo Szelinsky @ 2026-09-06 15:31 UTC (permalink / raw)
To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel, Carlo Szelinsky
phy_device_register() took rtnl_lock() around phy_try_attach_pse() to
serialise phydev->psec against the PSE controller notifier walk. But an
MDIO bus registered from ndo_init() runs with rtnl already held:
register_netdevice() # holds rtnl
ndo_init() == ltq_etop_init()
ltq_etop_mdio_init()
mdiobus_register()
mdiobus_scan()
phy_device_register()
rtnl_lock() # deadlock
so any such driver (lantiq_etop, sni_ave, netsec) deadlocks on probe.
Replace rtnl with a dedicated mutex for the attach, for the notifier
attach/detach walks, and for the ethtool PSE paths that dereference
phydev->psec. A private lock cannot recurse against the caller's rtnl, so
the register path no longer deadlocks, while attach vs notifier and
detach vs ethtool stay mutually excluded.
The mutex lives in pse_core, not phylib, and is reached through
pse_phy_lock() / pse_phy_unlock(). net/ethtool/pse-pd.c is built into
vmlinux unconditionally while PHYLIB is tristate, so ethtool cannot call
a phylib export: with CONFIG_PHYLIB=m or =n the link fails. PSE_CONTROLLER
is bool, so pse_core is always in vmlinux (or absent, with no-op stubs
for !PSE_CONTROLLER), and both built-in and modular callers reach the
lock. It does not protect a phylib field but the phy<->controller binding
against PSE controller teardown, and it sits directly above
pse_list_mutex, which pse_core already owns.
rtnl also kept the ethtool PSE reads from racing the PSE_UNREGISTERED
detach that frees phydev->psec, so net/ethtool/pse-pd.c takes the same
lock across its phydev->psec accesses; guarding only the phy side would
reopen a use-after-free there. The lock order is
rtnl -> pse_phy_mutex -> pse_list_mutex -> pcdev->lock, and the notifier
walks enter at pse_phy_lock() and never take rtnl.
Because the attach no longer takes rtnl, phy_device_register() and the
phy_device_register_locked() variant that was added for the rtnl-holding
sfp caller became identical, so fold them back into phy_device_register().
Reported-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Closes: https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
Tested-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
drivers/net/phy/phy_device.c | 94 +++++++++++------------------------
drivers/net/phy/sfp.c | 2 +-
drivers/net/pse-pd/pse_core.c | 46 +++++++++++++++++
include/linux/phy.h | 2 -
include/linux/pse-pd/pse.h | 23 +++++++++
net/ethtool/pse-pd.c | 16 ++++--
6 files changed, 111 insertions(+), 72 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f5febff4b00b..e8d894bbfa7a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1114,17 +1114,17 @@ 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 rtnl. A missing phandle (-ENOENT) or a not-yet-registered
- * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
- * PSE_REGISTERED time. Any other error means a broken binding and is warned
- * about, but left non-fatal so the phy still registers.
+ * Caller must hold pse_phy_lock(). A missing phandle (-ENOENT) or a
+ * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
+ * retries the latter at PSE_REGISTERED time. Any other error means a broken
+ * binding and is warned about, but left non-fatal so the phy still registers.
*/
static void phy_try_attach_pse(struct phy_device *phydev)
{
struct pse_control *psec;
struct device_node *np;
- ASSERT_RTNL();
+ pse_phy_lock_assert_held();
np = phydev->mdio.dev.of_node;
if (!np)
@@ -1146,7 +1146,7 @@ static void phy_try_attach_pse(struct phy_device *phydev)
static int phy_pse_attach_one(struct device *dev, void *data __maybe_unused)
{
- ASSERT_RTNL();
+ pse_phy_lock_assert_held();
if (dev->type != &mdio_bus_phy_type)
return 0;
@@ -1161,7 +1161,7 @@ static int phy_pse_detach_one(struct device *dev, void *data)
struct phy_device *phydev;
struct pse_control *psec;
- ASSERT_RTNL();
+ pse_phy_lock_assert_held();
if (dev->type != &mdio_bus_phy_type)
return 0;
@@ -1181,16 +1181,16 @@ static int phy_pse_notifier_event(struct notifier_block *nb,
{
switch (event) {
case PSE_REGISTERED:
- rtnl_lock();
+ pse_phy_lock();
bus_for_each_dev(&mdio_bus_type, NULL, NULL,
phy_pse_attach_one);
- rtnl_unlock();
+ pse_phy_unlock();
return NOTIFY_OK;
case PSE_UNREGISTERED:
- rtnl_lock();
+ pse_phy_lock();
bus_for_each_dev(&mdio_bus_type, NULL, data,
phy_pse_detach_one);
- rtnl_unlock();
+ pse_phy_unlock();
return NOTIFY_OK;
default:
return NOTIFY_DONE;
@@ -1201,15 +1201,22 @@ static struct notifier_block phy_pse_notifier __read_mostly = {
.notifier_call = phy_pse_notifier_event,
};
-/* Core registration: add the phy to the MDIO bus. Does not touch rtnl or
- * PSE. phydev->psec is attached by the callers below, 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. Keeping
- * device_add() out of rtnl also avoids deadlocking when binding a phy that
- * itself provides an SFP cage (phy_probe() -> phy_sfp_probe() ->
- * sfp_bus_add_upstream() takes rtnl).
+/**
+ * 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.
*/
-static int __phy_device_register(struct phy_device *phydev)
+int phy_device_register(struct phy_device *phydev)
{
int err;
@@ -1233,6 +1240,10 @@ static int __phy_device_register(struct phy_device *phydev)
goto out;
}
+ pse_phy_lock();
+ phy_try_attach_pse(phydev);
+ pse_phy_unlock();
+
return 0;
out:
@@ -1241,51 +1252,6 @@ static int __phy_device_register(struct phy_device *phydev)
mdiobus_unregister_device(&phydev->mdio);
return err;
}
-
-/**
- * phy_device_register_locked - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
- *
- * Same as phy_device_register() but caller must already hold rtnl_lock().
- *
- * Return: 0 on success, negative error code on failure.
- */
-int phy_device_register_locked(struct phy_device *phydev)
-{
- int err;
-
- ASSERT_RTNL();
-
- err = __phy_device_register(phydev);
- if (err)
- return err;
-
- phy_try_attach_pse(phydev);
-
- return 0;
-}
-EXPORT_SYMBOL(phy_device_register_locked);
-
-/**
- * phy_device_register - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
- *
- * Return: 0 on success, negative error code on failure.
- */
-int phy_device_register(struct phy_device *phydev)
-{
- int err;
-
- err = __phy_device_register(phydev);
- if (err)
- return err;
-
- rtnl_lock();
- phy_try_attach_pse(phydev);
- rtnl_unlock();
-
- return 0;
-}
EXPORT_SYMBOL(phy_device_register);
/**
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 18868bdd6485..03bfd8640db9 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2083,7 +2083,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
/* Mark this PHY as being on a SFP module */
phy->is_on_sfp_module = true;
- err = phy_device_register_locked(phy);
+ err = phy_device_register(phy);
if (err) {
phy_device_free(phy);
dev_err(sfp->dev, "phy_device_register failed: %pe\n",
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 611540df43bb..b771f5a2a83f 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -24,8 +24,54 @@ 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 - hold phydev->psec stable against PSE controller teardown
+ *
+ * The PSE_UNREGISTERED notifier clears phydev->psec and drops the last
+ * reference on the pse_control before the controller frees its state. Callers
+ * that attach, detach or dereference phydev->psec must hold this lock across
+ * the whole access so the detach cannot run underneath them.
+ */
+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
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 865b9baddb85..199a7aaa341b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2158,8 +2158,6 @@ struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
-/* Caller must hold rtnl_lock(); see phy_device_register() for the public form. */
-int phy_device_register_locked(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
void phy_device_remove(struct phy_device *phydev);
int phy_get_c45_ids(struct phy_device *phydev);
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index d4310ca71a3e..8c10613d9c51 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -391,6 +391,17 @@ bool pse_control_matches_pcdev(struct pse_control *psec,
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,
@@ -457,6 +468,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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach
2026-09-06 15:31 ` [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach Carlo Szelinsky
@ 2026-09-09 6:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 6:33 UTC (permalink / raw)
To: github
Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
edumazet, kuba, pabeni, corey, jelonek.jonas, horms, olek2,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 2 · Medium: 1 · Low: 1
- [High] At this commit, phydev->psec is released asynchronously and
without the new lock: phy_device_remove()…
- [High] pse_phy_lock() only serialises the attach against the notifier
callback itself, not against the controller teardown that follows it.
- [Medium] Commit message hides that the deadlock being fixed was
introduced two patches earlier in the same series.
- [Low] The commit message wrongly lists netsec among the drivers that
deadlock ("so any such driver (lantiq_etop, sni_ave, netsec) deadlocks…
--
net: phy: use a dedicated mutex instead of rtnl for PSE control attach
This replaces the rtnl_lock() that phy_device_register() took around
phy_try_attach_pse() with a new mutex living in pse_core, reached via
pse_phy_lock() / pse_phy_unlock(), and uses it in the PSE notifier walks
and in the ethtool PSE paths. phy_device_register_locked() becomes
identical to phy_device_register() and is folded back in.
Link: https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
A few questions on the changelog first.
> phy_device_register() took rtnl_lock() around phy_try_attach_pse() to
> serialise phydev->psec against the PSE controller notifier walk.
[Severity: Medium]
Should this say where that rtnl_lock() came from? At the baseline commit
cef9d6804030, drivers/net/phy/phy_device.c has no rtnl in
phy_device_register() and no phy_device_register_locked() at all; both were
added by the earlier patch in this same series ("net: phy: own phydev->psec
via PSE notifier and remove fwnode_mdio hook").
As written the message reads as if the rtnl_lock() were pre-existing
upstream behaviour, and there is no Fixes: tag or note that the deadlock is
introduced inside this submission.
That also leaves the series non-bisectable: with only the earlier patches
applied, register_netdevice() holds rtnl and calls ltq_etop_init() ->
ltq_etop_mdio_init() -> mdiobus_register() -> mdiobus_scan() ->
phy_device_register() -> rtnl_lock(), and ave_init() -> of_mdiobus_register()
reaches the same recursion, so those boards hang on probe at the
intermediate commit.
Would it make sense to squash this patch (and the never-needed exported
phy_device_register_locked() API that it removes again) into the patch that
introduced the rtnl_lock()?
> so any such driver (lantiq_etop, sni_ave, netsec) deadlocks on probe.
[Severity: Low]
Does netsec belong in that list? netsec_netdev_init(), its .ndo_init, only
allocates the TX/RX descriptor rings and resets the hardware; the MDIO bus
is registered from netsec_probe() via netsec_register_mdio(), before
register_netdev() is called, so phy_device_register() runs without rtnl
held there. The lantiq_etop and sni_ave paths do match the described
recursion.
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f5febff4b00b8..e8d894bbfa7aa 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
[ ... ]
> @@ -1181,16 +1181,16 @@ static int phy_pse_notifier_event(struct notifier_block *nb,
> {
> switch (event) {
> case PSE_REGISTERED:
> - rtnl_lock();
> + pse_phy_lock();
> bus_for_each_dev(&mdio_bus_type, NULL, NULL,
> phy_pse_attach_one);
> - rtnl_unlock();
> + pse_phy_unlock();
> return NOTIFY_OK;
> case PSE_UNREGISTERED:
> - rtnl_lock();
> + pse_phy_lock();
> bus_for_each_dev(&mdio_bus_type, NULL, data,
> phy_pse_detach_one);
> - rtnl_unlock();
> + pse_phy_unlock();
> return NOTIFY_OK;
[Severity: High]
Can a phy that has already left mdio_bus_type keep its pse_control past the
controller teardown?
The detach walk only reaches devices still on the bus, but at this commit
phy_device_remove() does not touch phydev->psec:
drivers/net/phy/phy_device.c:phy_device_remove() {
unregister_mii_timestamper(phydev->mii_ts);
device_del(&phydev->mdio.dev);
...
}
and the put is deferred to the release callback, without the new lock:
drivers/net/phy/phy_device.c:phy_device_release() {
...
might_sleep();
pse_control_put(phydev->psec);
...
}
So a phy that has been device_del()'d but is still pinned (an attached
netdev holding a reference, or an SFP-module phy waiting for
phy_device_free()) is invisible to the PSE_UNREGISTERED walk and keeps its
psec across pse_controller_unregister(). That function then frees the pi
array:
drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev); /* kfree(pcdev->pi) */
...
}
and the later put reaches __pse_control_release(), which does
"if (psec->pcdev->pi[psec->id].admin_state_enabled) regulator_disable(psec->ps);"
on that freed memory. This also runs outside pse_phy_lock(), which the new
kernel-doc says every phydev->psec access must hold.
The last patch of the series ("net: phy: release phydev->psec from
phy_device_remove() again") does move the put into phy_device_remove()
under pse_phy_lock() and strips it from phy_device_release(), so the end
state is fine; the concern is only about this intermediate commit, which
is also the one that documents the locking contract.
> @@ -1201,15 +1201,22 @@ static struct notifier_block phy_pse_notifier __read_mostly = {
[ ... ]
> +int phy_device_register(struct phy_device *phydev)
> {
> int err;
>
> @@ -1233,6 +1240,10 @@ static int __phy_device_register(struct phy_device *phydev)
> goto out;
> }
>
> + pse_phy_lock();
> + phy_try_attach_pse(phydev);
> + pse_phy_unlock();
> +
> return 0;
>
> out:
> @@ -1241,51 +1252,6 @@ static int __phy_device_register(struct phy_device *phydev)
> mdiobus_unregister_device(&phydev->mdio);
> return err;
> }
> -
> -/**
> - * phy_device_register_locked - Register the phy device on the MDIO bus
> - * @phydev: phy_device structure to be added to the MDIO bus
> - *
> - * Same as phy_device_register() but caller must already hold rtnl_lock().
> - *
> - * Return: 0 on success, negative error code on failure.
> - */
> -int phy_device_register_locked(struct phy_device *phydev)
> -{
> - int err;
> -
> - ASSERT_RTNL();
> -
> - err = __phy_device_register(phydev);
> - if (err)
> - return err;
> -
> - phy_try_attach_pse(phydev);
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(phy_device_register_locked);
[ ... ]
> EXPORT_SYMBOL(phy_device_register);
[ ... ]
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 611540df43bb1..b771f5a2a83fc 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -24,8 +24,54 @@ 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
[ ... ]
> +static DEFINE_MUTEX(pse_phy_mutex);
> +
> static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
>
> +/**
> + * pse_phy_lock - hold phydev->psec stable against PSE controller teardown
> + *
> + * The PSE_UNREGISTERED notifier clears phydev->psec and drops the last
> + * reference on the pse_control before the controller frees its state. Callers
> + * that attach, detach or dereference phydev->psec must hold this lock across
> + * the whole access so the detach cannot run underneath them.
> + */
> +void pse_phy_lock(void)
> +{
> + mutex_lock(&pse_phy_mutex);
> +}
> +EXPORT_SYMBOL_GPL(pse_phy_lock);
[Severity: High]
Does this lock actually cover the teardown it claims to protect against?
phy_pse_notifier_event() drops pse_phy_mutex as soon as the detach walk
returns, and pse_controller_unregister() does the rest of the teardown with
the mutex no longer held:
drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev); /* kfree(pcdev->pi) */
if (pcdev->irq)
disable_irq(pcdev->irq);
cancel_work_sync(&pcdev->ntf_work);
kfifo_free(&pcdev->ntf_fifo);
mutex_lock(&pse_list_mutex);
list_del(&pcdev->list);
mutex_unlock(&pse_list_mutex);
}
The controller stays on pse_controller_list until the very end, and the
window spans cancel_work_sync(), which can block for a queued notification
work item.
Can a concurrent phy_device_register() -> pse_phy_lock() ->
phy_try_attach_pse() -> of_pse_control_get() land in that window?
of_pse_control_get() walks pse_controller_list under pse_list_mutex only,
with no unregistering-state check, so of_pse_match_pi() reads the freed
pcdev->pi[i].np and pse_control_get_internal() then writes
pcdev->pi[index].admin_state_enabled and dereferences pcdev->pi[index].rdev.
The pse_control created that way can also never be detached, because the
PSE_UNREGISTERED walk has already finished, so its eventual put re-enters
__pse_control_release() on the freed pi array.
Would moving the list_del() ahead of pse_release_pis(), or holding
pse_phy_mutex across the whole of pse_controller_unregister(), close this?
The list_del-last ordering predates the series, but the changelog states
the new mutex protects "the phy<->controller binding against PSE controller
teardown", and the series adds both new attach sites, so the case seems
worth addressing here. This is still present at the tip of the series.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
` (3 preceding siblings ...)
2026-09-06 15:31 ` [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach Carlo Szelinsky
@ 2026-09-06 15:31 ` Carlo Szelinsky
2026-09-09 6:33 ` netdev-bot+sashiko
2026-09-10 9:06 ` [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Paolo Abeni
5 siblings, 1 reply; 12+ messages in thread
From: Carlo Szelinsky @ 2026-09-06 15:31 UTC (permalink / raw)
To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel, Carlo Szelinsky
"net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio
hook" deferred the final pse_control_put() of phydev->psec from
phy_device_remove() to phy_device_release(), so it would run only after
the PSE_UNREGISTERED notifier walk had dropped its bus-iterator reference
on the phy. But bus_for_each_dev() only reaches phys still on the
mdio_bus_type klist: a phy that has been device_del()'d yet is still
pinned (e.g. by an attached netdev) is invisible to the walk, so
phy_pse_detach_one() never clears its phydev->psec. Its deferred put then
runs after pse_controller_unregister() -> pse_release_pis() has freed
pcdev->pi[], and __pse_control_release() dereferences the freed array:
use-after-free.
Put phydev->psec back in phy_device_remove(), before device_del(), so
the detach is synchronous and ordered ahead of the phy leaving the bus;
it can no longer outlive the PSE controller. "net: phy: use a dedicated
mutex instead of rtnl for PSE control attach" replaced rtnl with
pse_phy_lock() for the attach/detach, so this put can take that same lock
without the rtnl recursion that originally motivated the deferral, and it
serialises against the notifier walk: whichever runs first clears
phydev->psec, the other sees NULL.
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
drivers/net/phy/phy_device.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e8d894bbfa7a..bb120045c406 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -223,19 +223,8 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
static void phy_device_release(struct device *dev)
{
- struct phy_device *phydev = to_phy_device(dev);
-
- /* bus_for_each_dev() holds get_device() across each iteration
- * step, deferring this release callback until any in-flight PSE
- * notifier walk has advanced past this phy. pse_control_put()
- * takes pse_list_mutex, so this path must run in sleepable
- * context.
- */
- might_sleep();
- pse_control_put(phydev->psec);
-
fwnode_handle_put(dev->fwnode);
- kfree(phydev);
+ kfree(to_phy_device(dev));
}
static void phy_mdio_device_remove(struct mdio_device *mdiodev)
@@ -1265,6 +1254,16 @@ EXPORT_SYMBOL(phy_device_register);
void phy_device_remove(struct phy_device *phydev)
{
unregister_mii_timestamper(phydev->mii_ts);
+
+ /* 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.
+ */
+ pse_phy_lock();
+ pse_control_put(phydev->psec);
+ phydev->psec = NULL;
+ pse_phy_unlock();
+
device_del(&phydev->mdio.dev);
/* Assert the reset signal */
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again
2026-09-06 15:31 ` [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again Carlo Szelinsky
@ 2026-09-09 6:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 6:33 UTC (permalink / raw)
To: github
Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
edumazet, kuba, pabeni, corey, jelonek.jonas, horms, olek2,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 2 (1) · Medium: 2 · Low: 0
- [High] The commit claims that after this change phydev->psec "can no
longer outlive the PSE controller", but the same freed-array sink…
- [High] phy_device_remove() now drops phydev->psec and releases
pse_phy_lock() *before* device_del(), i.e. before the phy leaves the…
- [Medium] In-series regression fixed by a later patch of the same
series, with no attribution.
- [Medium] Memory leak of pse_control on the error path of
phy_device_register() if device_add() fails.
Pre-existing issues:
- [High] phylib clears only the forward link on detach (phydev->psec =
NULL in phy_pse_detach_one() and in the newly added block in…
--
net: phy: release phydev->psec from phy_device_remove() again
This moves the final pse_control_put() of phydev->psec out of
phy_device_release() and back into phy_device_remove(), where it now runs
under pse_phy_lock() just before device_del(). The stated reason is that a
device_del()'d but still pinned phy is invisible to the PSE_UNREGISTERED
bus walk, so a deferred put could outlive the PSE controller.
Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/
A few questions below.
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e8d894bbfa7aa..bb120045c406e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -223,19 +223,8 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
>
> static void phy_device_release(struct device *dev)
> {
> - struct phy_device *phydev = to_phy_device(dev);
> -
> - /* bus_for_each_dev() holds get_device() across each iteration
> - * step, deferring this release callback until any in-flight PSE
> - * notifier walk has advanced past this phy. pse_control_put()
> - * takes pse_list_mutex, so this path must run in sleepable
> - * context.
> - */
> - might_sleep();
> - pse_control_put(phydev->psec);
> -
> fwnode_handle_put(dev->fwnode);
> - kfree(phydev);
> + kfree(to_phy_device(dev));
> }
[Severity: Medium]
With the put gone from the release callback, phy_device_remove() is
the only place phylib ever drops phydev->psec. Do the error paths of
phy_device_register() still reach it?
As far as I can tell they do not: if device_add() fails,
phy_device_register() logs "failed to add", jumps to out:, asserts the
reset and calls mdiobus_unregister_device(), then returns the error to
the caller, which typically just phy_device_free()s the phy.
phy_device_remove() is never invoked, and phy_device_release() is now
only fwnode_handle_put() plus kfree().
The window matters because device_add() puts the device on the
mdio_bus_type klist in bus_add_device() before dpm_sysfs_add() and the
remaining steps, any of which can still fail and roll back. While the
phy sits on that klist a concurrent PSE_REGISTERED walk
(bus_for_each_dev(&mdio_bus_type, ..., phy_pse_attach_one) under
pse_phy_lock()) can see it, and phy_try_attach_pse() will set
phydev->psec via of_pse_control_get() because psec is still NULL. On
the failure return nothing puts that reference: the struct pse_control
stays on pcdev->pse_control_head with attached_phydev pointing at
memory that is about to be kfree()d, and the try_module_get(pcdev->owner)
taken in pse_control_get_internal() is never dropped, pinning the PSE
driver module for the lifetime of the system. Keeping the put in
phy_device_release() happened to cover this case for free.
This is a cold path, but could the same locked put be factored into a
small helper (say phy_pse_detach(phydev)) and called from both
phy_device_remove() and the out: label of phy_device_register()? Or
do you consider a put in the release callback still warranted as a
backstop for phys that never reach phy_device_remove() at all?
Related question: if phy_device_register() itself attaches the psec
before any of its remaining failure points, the leak would not even
need a concurrent notifier walk -- can you confirm the ordering there?
[Severity: Medium]
Is the use-after-free described in the changelog reachable in any released
tree? In the baseline this series applies to, phy_device_remove() already
contained:
unregister_mii_timestamper(phydev->mii_ts);
pse_control_put(phydev->psec);
and phy_device_release() had no PSE put at all. The deferral being undone
here was added earlier in this same submission by "net: phy: own
phydev->psec via PSE notifier and remove fwnode_mdio hook", whose own
changelog says the put "moves from phy_device_remove() to
phy_device_release()".
Should this hunk simply be folded into that patch instead of appearing as a
separate commit? As posted, the two intermediate commits of the series
carry the live use-after-free on pcdev->pi[] that this commit removes, so
bisecting an unrelated phy or PSE problem into that window hits it.
The two sibling patches are referenced by quoted title only, with no SHA,
no Fixes: tag and no statement that they belong to the same series. Could
that be made explicit either way, so a maintainer or stable backporter can
tell whether a Fixes:/backport is needed here? If the commits do stay
separate, the introducing patch's changelog is left permanently documenting
a deferral rationale that no longer matches the tree.
> @@ -1265,6 +1254,16 @@ EXPORT_SYMBOL(phy_device_register);
> void phy_device_remove(struct phy_device *phydev)
> {
> unregister_mii_timestamper(phydev->mii_ts);
> +
> + /* 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.
> + */
> + pse_phy_lock();
> + pse_control_put(phydev->psec);
> + phydev->psec = NULL;
> + pse_phy_unlock();
> +
> device_del(&phydev->mdio.dev);
[Severity: High]
Can a concurrent PSE_REGISTERED walk re-attach a psec to this phy after the
put has already run?
The lock is dropped here, and the phy only leaves the mdio_bus_type klist
much later, inside device_del() -> bus_remove_device():
drivers/base/bus.c:bus_remove_device() {
...
if (klist_node_attached(&dev->p->knode_bus))
klist_del(&dev->p->knode_bus);
...
}
bus_for_each_dev() does not skip devices whose device_del() is in progress,
so throughout kill_device(), BUS_NOTIFY_DEL_DEVICE, dpm_sysfs_remove() and
the sysfs teardown the phy is still reachable from:
phy_pse_notifier_event() {
case PSE_REGISTERED:
pse_phy_lock();
bus_for_each_dev(&mdio_bus_type, NULL, NULL,
phy_pse_attach_one);
pse_phy_unlock();
}
phy_pse_attach_one() -> phy_try_attach_pse() then sees dev->type ==
&mdio_bus_phy_type, of_node still set, and psec == NULL because this hunk
just cleared it:
if (phydev->psec)
return;
psec = of_pse_control_get(np, phydev);
...
phydev->psec = psec;
Which path releases that new reference? phy_pse_detach_one() can no longer
see the phy once it is off the klist, the put in phy_device_remove() has
already run, and phy_device_release() is now just fwnode_handle_put() plus
kfree(). That looks like a leaked struct pse_control left on
pcdev->pse_control_head with attached_phydev pointing at freed memory, plus
the try_module_get(pcdev->owner) reference taken in
pse_control_get_internal(), which would pin the PSE driver module for good.
The changelog says "whichever runs first clears phydev->psec, the other sees
NULL". Does that hold for PSE_REGISTERED, which creates a reference rather
than clearing one?
Would placing the locked put after device_del(), still synchronously in
phy_device_remove(), close this? Then either the put wins and the walk
cannot see the off-klist phy, or the walk wins and the put releases whatever
it attached.
[Severity: High]
The changelog states the put "can no longer outlive the PSE controller". Is
that accurate given the attach side?
pse_controller_unregister() fires the notifier, frees pcdev->pi[], and only
then removes the controller from the list:
drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
blocking_notifier_call_chain(&pse_controller_notifier,
PSE_UNREGISTERED, pcdev);
pse_flush_pw_ds(pcdev);
pse_release_pis(pcdev);
...
mutex_lock(&pse_list_mutex);
list_del(&pcdev->list);
mutex_unlock(&pse_list_mutex);
}
pse_release_pis() does kfree(pcdev->pi) without clearing pcdev->pi or
pcdev->nr_lines. pse_phy_lock() is held only for the duration of the
notifier walk inside phy_pse_notifier_event(), which ends before
pse_release_pis() runs, so between the end of the walk and that list_del the
controller is still resolvable:
phy_device_register() -> phy_try_attach_pse() -> of_pse_control_get()
of_pse_match_pi() then reads pcdev->pi[i].np out of the freed array, and
pse_control_get_internal() reads and writes it:
pcdev->pi[index].admin_state_enabled = ret;
psec->ps = devm_regulator_get_exclusive(pcdev->dev,
rdev_get_name(pcdev->pi[index].rdev));
The resulting psec lands in phydev->psec, and its eventual put reaches the
same freed-array dereference the changelog says is eliminated:
drivers/net/pse-pd/pse_core.c:__pse_control_release() {
if (psec->pcdev->pi[psec->id].admin_state_enabled)
regulator_disable(psec->ps);
}
The documented contract in include/linux/pse-pd/pse.h says subscribers "must
not acquire new references for it" after PSE_UNREGISTERED, but nothing on
the subscriber side can enforce that while the controller stays on
pse_controller_list.
Would making the controller unresolvable before pse_release_pis() (list_del,
or an unregistering flag tested under pse_list_mutex), and having
pse_release_pis() NULL out pcdev->pi and zero nr_lines, be needed for the
claim to hold?
[Severity: High]
This isn't a bug introduced by this patch, but only the forward link is
cleared here; nothing in phylib ever invalidates the reverse link
psec->attached_phydev. pse_core sets it once, and only when it allocates a
fresh psec:
drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
psec->attached_phydev = phydev;
}
An existing psec at the same PI index is merely kref_get()'d and the new
phydev pointer discarded, so the back-pointer stays bound to whichever phy
attached first and survives that phy's detach. It is dereferenced here:
drivers/net/pse-pd/pse_core.c:pse_control_get_netdev() {
ASSERT_RTNL();
if (!psec || !psec->attached_phydev)
return NULL;
return psec->attached_phydev->attached_dev;
}
The NULL check cannot detect a freed phy. pse_send_ntf_worker() takes a
transient reference and then blocks:
psec = pse_control_find_by_id(pcdev, ntf.id);
rtnl_lock();
netdev = pse_control_get_netdev(psec);
If the phy removal completes in that window (this put drops phylib's
reference while the psec stays alive on the worker's kref, then
phy_mdio_device_free() -> put_device() -> phy_device_release() -> kfree()),
does the worker dereference the freed phy_device and hand a garbage
net_device pointer to ethnl_pse_send_ntf()?
Same window existed with the put in phy_device_release() and in the
baseline, so this is not a new problem, but should phylib also clear
psec->attached_phydev on detach?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe
2026-09-06 15:30 [PATCH net-next v6 0/5] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
` (4 preceding siblings ...)
2026-09-06 15:31 ` [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again Carlo Szelinsky
@ 2026-09-10 9:06 ` Paolo Abeni
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2026-09-10 9:06 UTC (permalink / raw)
To: Carlo Szelinsky, Oleksij Rempel, Kory Maincent, Andrew Lunn,
Heiner Kallweit, Russell King, David S . Miller, Eric Dumazet,
Jakub Kicinski
Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
Aleksander Jan Bajkowski, netdev, linux-kernel
On 9/6/26 5:30 PM, Carlo Szelinsky wrote:
> This is v6 of Corey's series [1]. It takes the PSE controller lookup out
> of the MDIO probe path, so a modular PSE driver no longer makes the
> PHY/DSA probe spin on -EPROBE_DEFER until the PSE module loads.
>
> Patches 1-3 are the same three notifier patches as v4 [4], unchanged,
> with Jonas's Tested-by. Patches 4 and 5 fix two problems the v4 review
> surfaced; v6 additionally fixes a build regression in v5 [7]'s patch 4.
>
> Patch 4: Aleksander reported [5] that v4 deadlocks on probe for an MDIO
> bus registered from ndo_init (lantiq_etop, sni_ave, netsec): those
> already hold rtnl via register_netdevice(), and v4's phy attach took rtnl
> again underneath. Patch 4 swaps that rtnl for a dedicated mutex, so the
> register path no longer recurses. The ethtool PSE paths take the same
> mutex, so the use-after-free that rtnl used to close stays closed. The
> mutex lives in pse_core rather than phylib: net/ethtool is always built
> into vmlinux but PHYLIB is tristate, so with CONFIG_PHYLIB=m or =n a
> phylib export is unresolved (v5 failed to link there [8]); PSE_CONTROLLER
> is bool, so pse_core is always reachable.
>
> Patch 5: Paolo's review [6] pointed out that patch 3 defers the
> pse_control_put() to phy_device_release(). A phy that is device_del()'d
> but still pinned (an attached netdev) is off the mdio_bus_type klist, so
> the PSE_UNREGISTERED notifier walk never clears its phydev->psec, and the
> deferred put later touches a pcdev->pi[] the controller has already
> freed. Patch 5 puts phydev->psec back in phy_device_remove(), which the
> mutex from patch 4 now makes safe (the rtnl recursion that motivated the
> deferral is gone), so the detach is synchronous and cannot outlive the
> controller.
>
> How it works: pse_core gets a notifier chain (REGISTERED / UNREGISTERED).
> The phy layer subscribes, owns phydev->psec, and attaches the PSE 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.
>
> Tested on a Realtek rtl93xx PoE switch with two HS104 PSE controllers on
> i2c:
>
> - clean boot, no probe-retry loop, no watchdog reset
> - 10G SFP+ port: module hotplug works, no deadlock
> - ethtool --set-pse enable/disable cuts and restores power to a PD
> - i2c unbind -> rmmod -> modprobe: PSE detaches on unbind and re-attaches
> on reload with power restored, no reboot. No lockdep splats.
>
> Jonas confirmed the RTL8214FC deadlock he reported is gone. Aleksander
> confirmed the lantiq_etop probe deadlock is gone at boot.
>
> Tested-by: Carlo Szelinsky <github@szelinsky.de>
A bunch of 'high prio' sashiko reported issues are actually fixed by
patch 5/5, so IMHO not very relevant. Still I think there are a few
points to act upon.
@Carlo: please have a look at commit
c82ff94592fb68f529afe63ca7f5ddb7dae4ba83: you are supposed to reply to
LLM's comments.
/P
^ permalink raw reply [flat|nested] 12+ messages in thread