[PATCH net 1/2] net: dsa: properly keep track of conduit reference

[PATCH net 1/2] net: dsa: properly keep track of conduit reference

[Vladimir Oltean]

Problem description
-------------------

DSA has a mumbo-jumbo of reference handling of the conduit net device
and its kobject which, sadly, is just wrong and doesn't make sense.

There are two distinct problems.

1. The OF path, which uses of_find_net_device_by_node(), never releases
   the elevated refcount on the conduit's kobject. Nominally, the OF and
   non-OF paths should have identical code paths, and it is already
   suspicious that dsa_dev_to_net_device() has a put_device() call which
   is missing in dsa_port_parse_of(), but we can actually even verify
   that an issue exists. With CONFIG_DEBUG_KOBJECT_RELEASE=y, if we run
   this command "before" and "after" applying this patch:

(unbind the conduit driver for net device eno2)
echo 0000:00:00.2 > /sys/bus/pci/drivers/fsl_enetc/unbind

we see these lines in the output diff which appear only with the patch
applied:

kobject: 'eno2' (ffff002009a3a6b8): kobject_release, parent 0000000000000000 (delayed 1000)
kobject: '109' (ffff0020099d59a0): kobject_release, parent 0000000000000000 (delayed 1000)

2. After we find the conduit interface one way (OF) or another (non-OF),
   it can get unregistered at any time, and DSA remains with a long-lived,
   but in this case stale, cpu_dp->conduit pointer. Holding the net
   device's underlying kobject isn't actually of much help, it just
   prevents it from being freed (but we never need that kobject
   directly). What helps us to prevent the net device from being
   unregistered is the parallel netdev reference mechanism (dev_hold()
   and dev_put()).

Actually we actually use that netdev tracker mechanism implicitly on
user ports since commit 2f1e8ea726e9 ("net: dsa: link interfaces with
the DSA master to get rid of lockdep warnings"), via netdev_upper_dev_link().
But time still passes at DSA switch probe time between the initial
of_find_net_device_by_node() code and the user port creation time, time
during which the conduit could unregister itself and DSA wouldn't know
about it.

So we have to run of_find_net_device_by_node() under rtnl_lock() to
prevent that from happening, and release the lock only with the netdev
tracker having acquired the reference.

Do we need to keep the reference until dsa_unregister_switch() /
dsa_switch_shutdown()?
1: Maybe yes. A switch device will still be registered even if all user
   ports failed to probe, see commit 86f8b1c01a0a ("net: dsa: Do not
   make user port errors fatal"), and the cpu_dp->conduit pointers
   remain valid.  I haven't audited all call paths to see whether they
   will actually use the conduit in lack of any user port, but if they
   do, it seems safer to not rely on user ports for that reference.
2. Definitely yes. We support changing the conduit which a user port is
   associated to, and we can get into a situation where we've moved all
   user ports away from a conduit, thus no longer hold any reference to
   it via the net device tracker. But we shouldn't let it go nonetheless
   - see the next change in relation to dsa_tree_find_first_conduit()
   and LAG conduits which disappear.
   We have to be prepared to return to the physical conduit, so the CPU
   port must explicitly keep another reference to it. This is also to
   say: the user ports and their CPU ports may not always keep a
   reference to the same conduit net device, and both are needed.

As for the conduit's kobject for the /sys/class/net/ entry, we don't
care about it, we can release it as soon as we hold the net device
object itself.

History and blame attribution
-----------------------------

The code has been refactored so many times, it is very difficult to
follow and properly attribute a blame, but I'll try to make a short
history which I hope to be correct.

We have two distinct probing paths:
- one for OF, introduced in 2016 in commit 83c0afaec7b7 ("net: dsa: Add
  new binding implementation")
- one for non-OF, introduced in 2017 in commit 71e0bbde0d88 ("net: dsa:
  Add support for platform data")

These are both complete rewrites of the original probing paths (which
used struct dsa_switch_driver and other weird stuff, instead of regular
devices on their respective buses for register access, like MDIO, SPI,
I2C etc):
- one for OF, introduced in 2013 in commit 5e95329b701c ("dsa: add
  device tree bindings to register DSA switches")
- one for non-OF, introduced in 2015 in commit 91da11f870f0 ("net:
  Distributed Switch Architecture protocol support")

except for tiny bits and pieces like dsa_dev_to_net_device() which were
seemingly carried over since the original commit, and used to this day.

The point is that the original probing paths received a fix in 2015 in
the form of commit 679fb46c5785 ("net: dsa: Add missing master netdev
dev_put() calls"), but the fix never made it into the "new" (dsa2)
probing paths that can still be traced to today, and the fixed probing
path was later deleted in 2019 in commit 93e86b3bc842 ("net: dsa: Remove
legacy probing support").

That is to say, the new probing paths were never quite correct in this
area.

The existence of the legacy probing support which was deleted in 2019
explains why dsa_dev_to_net_device() returns a conduit with elevated
refcount (because it was supposed to be released during
dsa_remove_dst()). After the removal of the legacy code, the only user
of dsa_dev_to_net_device() calls dev_put(conduit) immediately after this
function returns. This pattern makes no sense today, and can only be
interpreted historically to understand why dev_hold() was there in the
first place.

Change details
--------------

Today we have a better netdev tracking infrastructure which we should
use. It belongs in common code (dsa_port_parse_cpu()), which shows that
the OF and non-OF code paths aren't actually so different.

When dsa_port_parse_cpu() or any subsequent function during setup fails,
dsa_switch_release_ports() will be called. However, dsa_port_parse_cpu()
may fail prior to us assigning cpu_dp->conduit and bumping the reference.
So we have to test for the conduit being NULL prior to calling
netdev_put().

There have still been so many transformations to the code since the
blamed commits (rename master -> conduit, commit 0650bf52b31f ("net:
dsa: be compatible with masters which unregister on shutdown")), that it
only makes sense to fix the code using the best methods available today
and see how it can be backported to stable later. I suspect the fix
cannot even be backported to kernels which lack dsa_switch_shutdown(),
and I suspect this is also maybe why the long-lived conduit reference
didn't make it into the new DSA probing paths at the time (problems
during shutdown).

Because dsa_dev_to_net_device() has a single call site and has to be
changed anyway, the logic was just absorbed into the non-OF
dsa_port_parse().

Tested on the ocelot/felix switch and on dsa_loop, both on the NXP
LS1028A with CONFIG_DEBUG_KOBJECT_RELEASE=y.

Reported-by: Ma Ke <make24@iscas.ac.cn>
Closes: https://lore.kernel.org/netdev/20251214131204.4684-1-make24@iscas.ac.cn/
Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
Fixes: 71e0bbde0d88 ("net: dsa: Add support for platform data")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h |  1 +
 net/dsa/dsa.c     | 53 +++++++++++++++++++++++++----------------------
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index cced1a866757..6b2b5ed64ea4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -302,6 +302,7 @@ struct dsa_port {
 	struct devlink_port	devlink_port;
 	struct phylink		*pl;
 	struct phylink_config	pl_config;
+	netdevice_tracker	conduit_tracker;
 	struct dsa_lag		*lag;
 	struct net_device	*hsr_dev;
 
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index a20efabe778f..ac7900113d2b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1221,6 +1221,7 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *conduit,
 		dst->tag_ops = tag_ops;
 	}
 
+	netdev_hold(conduit, &dp->conduit_tracker, GFP_KERNEL);
 	dp->conduit = conduit;
 	dp->type = DSA_PORT_TYPE_CPU;
 	dsa_port_set_tag_protocol(dp, dst->tag_ops);
@@ -1253,14 +1254,21 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
 	if (ethernet) {
 		struct net_device *conduit;
 		const char *user_protocol;
+		int err;
 
+		rtnl_lock();
 		conduit = of_find_net_device_by_node(ethernet);
 		of_node_put(ethernet);
-		if (!conduit)
+		if (!conduit) {
+			rtnl_unlock();
 			return -EPROBE_DEFER;
+		}
 
 		user_protocol = of_get_property(dn, "dsa-tag-protocol", NULL);
-		return dsa_port_parse_cpu(dp, conduit, user_protocol);
+		err = dsa_port_parse_cpu(dp, conduit, user_protocol);
+		put_device(&conduit->dev);
+		rtnl_unlock();
+		return err;
 	}
 
 	if (link)
@@ -1393,37 +1401,27 @@ static struct device *dev_find_class(struct device *parent, char *class)
 	return device_find_child(parent, class, dev_is_class);
 }
 
-static struct net_device *dsa_dev_to_net_device(struct device *dev)
-{
-	struct device *d;
-
-	d = dev_find_class(dev, "net");
-	if (d != NULL) {
-		struct net_device *nd;
-
-		nd = to_net_dev(d);
-		dev_hold(nd);
-		put_device(d);
-
-		return nd;
-	}
-
-	return NULL;
-}
-
 static int dsa_port_parse(struct dsa_port *dp, const char *name,
 			  struct device *dev)
 {
 	if (!strcmp(name, "cpu")) {
 		struct net_device *conduit;
+		struct device *d;
+		int err;
 
-		conduit = dsa_dev_to_net_device(dev);
-		if (!conduit)
+		rtnl_lock();
+		d = dev_find_class(dev, "net");
+		if (!d) {
+			rtnl_unlock();
 			return -EPROBE_DEFER;
+		}
 
-		dev_put(conduit);
+		conduit = to_net_dev(d);
 
-		return dsa_port_parse_cpu(dp, conduit, NULL);
+		err = dsa_port_parse_cpu(dp, conduit, NULL);
+		put_device(d);
+		rtnl_unlock();
+		return err;
 	}
 
 	if (!strcmp(name, "dsa"))
@@ -1491,6 +1489,9 @@ static void dsa_switch_release_ports(struct dsa_switch *ds)
 	struct dsa_vlan *v, *n;
 
 	dsa_switch_for_each_port_safe(dp, next, ds) {
+		if (dsa_port_is_cpu(dp) && dp->conduit)
+			netdev_put(dp->conduit, &dp->conduit_tracker);
+
 		/* These are either entries that upper layers lost track of
 		 * (probably due to bugs), or installed through interfaces
 		 * where one does not necessarily have to remove them, like
@@ -1635,8 +1636,10 @@ void dsa_switch_shutdown(struct dsa_switch *ds)
 	/* Disconnect from further netdevice notifiers on the conduit,
 	 * since netdev_uses_dsa() will now return false.
 	 */
-	dsa_switch_for_each_cpu_port(dp, ds)
+	dsa_switch_for_each_cpu_port(dp, ds) {
+		netdev_put(dp->conduit, &dp->conduit_tracker);
 		dp->conduit->dsa_ptr = NULL;
+	}
 
 	rtnl_unlock();
 out:
-- 
2.43.0

[PATCH net 2/2] net: dsa: fix missing put_device() in dsa_tree_find_first_conduit()

[Vladimir Oltean]

of_find_net_device_by_node() searches net devices by their /sys/class/net/,
entry. It is documented in its kernel-doc that:

 * If successful, returns a pointer to the net_device with the embedded
 * struct device refcount incremented by one, or NULL on failure. The
 * refcount must be dropped when done with the net_device.

We are missing a put_device(&conduit->dev) which we could place at the
end of dsa_tree_find_first_conduit(). But to explain why calling
put_device() right away is safe is the same as to explain why the chosen
solution is different.

The code is very poorly split: dsa_tree_find_first_conduit() was first
introduced in commit 95f510d0b792 ("net: dsa: allow the DSA master to be
seen and changed through rtnetlink") but was first used several commits
later, in commit acc43b7bf52a ("net: dsa: allow masters to join a LAG").

Assume there is a switch with 2 CPU ports and 2 conduits, eno2 and eno3.
When we create a LAG (bonding or team device) and place eno2 and eno3
beneath it, we create a 3rd conduit (the LAG device itself), but this is
slightly different than the first two.

Namely, the cpu_dp->conduit pointer of the CPU ports does not change,
and remains pointing towards the physical Ethernet controllers which are
now LAG ports. Only 2 things change:
- the LAG device has a dev->dsa_ptr which marks it as a DSA conduit
- dsa_port_to_conduit(user port) finds the LAG and not the physical
  conduit, because of the dp->cpu_port_in_lag bit being set.

When the LAG device is destroyed, dsa_tree_migrate_ports_from_lag_conduit()
is called and this is where dsa_tree_find_first_conduit() kicks in.

This is the logical mistake and the reason why introducing code in one
patch and using it from another is bad practice. I didn't realize that I
don't have to call of_find_net_device_by_node() again; the cpu_dp->conduit
association was never undone, and is still available for direct (re)use.
There's only one concern - maybe the conduit disappeared in the
meantime, but the netdev_hold() call we made during dsa_port_parse_cpu()
(see previous change) ensures that this was not the case.

Therefore, fixing the code means reimplementing it in the simplest way.

I am blaming the time of use, since this is what "git blame" would show
if we were to monitor for the conduit's kobject's refcount remaining
elevated instead of being freed.

Tested on the NXP LS1028A, using the steps from
Documentation/networking/dsa/configuration.rst section "Affinity of user
ports to CPU ports", followed by (extra prints added by me):

$ ip link del bond0
mscc_felix 0000:00:00.5 swp3: Link is Down
bond0 (unregistering): (slave eno2): Releasing backup interface
fsl_enetc 0000:00:00.2 eno2: Link is Down
mscc_felix 0000:00:00.5 swp0: bond0 disappeared, migrating to eno2
mscc_felix 0000:00:00.5 swp1: bond0 disappeared, migrating to eno2
mscc_felix 0000:00:00.5 swp2: bond0 disappeared, migrating to eno2
mscc_felix 0000:00:00.5 swp3: bond0 disappeared, migrating to eno2

Fixes: acc43b7bf52a ("net: dsa: allow masters to join a LAG")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ac7900113d2b..e3b233060242 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -367,16 +367,10 @@ static struct dsa_port *dsa_tree_find_first_cpu(struct dsa_switch_tree *dst)
 
 struct net_device *dsa_tree_find_first_conduit(struct dsa_switch_tree *dst)
 {
-	struct device_node *ethernet;
-	struct net_device *conduit;
 	struct dsa_port *cpu_dp;
 
 	cpu_dp = dsa_tree_find_first_cpu(dst);
-	ethernet = of_parse_phandle(cpu_dp->dn, "ethernet", 0);
-	conduit = of_find_net_device_by_node(ethernet);
-	of_node_put(ethernet);
-
-	return conduit;
+	return cpu_dp->conduit;
 }
 
 /* Assign the default CPU port (the first one in the tree) to all ports of the
-- 
2.43.0

Re: [PATCH net 1/2] net: dsa: properly keep track of conduit reference

[Jonas Gorski]

Hi,

On Sun, Dec 14, 2025 at 7:25 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Problem description
> -------------------
>
> DSA has a mumbo-jumbo of reference handling of the conduit net device
> and its kobject which, sadly, is just wrong and doesn't make sense.
>
> There are two distinct problems.
>
> 1. The OF path, which uses of_find_net_device_by_node(), never releases
>    the elevated refcount on the conduit's kobject. Nominally, the OF and
>    non-OF paths should have identical code paths, and it is already
>    suspicious that dsa_dev_to_net_device() has a put_device() call which
>    is missing in dsa_port_parse_of(), but we can actually even verify
>    that an issue exists. With CONFIG_DEBUG_KOBJECT_RELEASE=y, if we run
>    this command "before" and "after" applying this patch:
>
> (unbind the conduit driver for net device eno2)
> echo 0000:00:00.2 > /sys/bus/pci/drivers/fsl_enetc/unbind
>
> we see these lines in the output diff which appear only with the patch
> applied:
>
> kobject: 'eno2' (ffff002009a3a6b8): kobject_release, parent 0000000000000000 (delayed 1000)
> kobject: '109' (ffff0020099d59a0): kobject_release, parent 0000000000000000 (delayed 1000)
>
> 2. After we find the conduit interface one way (OF) or another (non-OF),
>    it can get unregistered at any time, and DSA remains with a long-lived,
>    but in this case stale, cpu_dp->conduit pointer. Holding the net
>    device's underlying kobject isn't actually of much help, it just
>    prevents it from being freed (but we never need that kobject
>    directly). What helps us to prevent the net device from being
>    unregistered is the parallel netdev reference mechanism (dev_hold()
>    and dev_put()).
>
> Actually we actually use that netdev tracker mechanism implicitly on
> user ports since commit 2f1e8ea726e9 ("net: dsa: link interfaces with
> the DSA master to get rid of lockdep warnings"), via netdev_upper_dev_link().
> But time still passes at DSA switch probe time between the initial
> of_find_net_device_by_node() code and the user port creation time, time
> during which the conduit could unregister itself and DSA wouldn't know
> about it.
>
> So we have to run of_find_net_device_by_node() under rtnl_lock() to
> prevent that from happening, and release the lock only with the netdev
> tracker having acquired the reference.
>
> Do we need to keep the reference until dsa_unregister_switch() /
> dsa_switch_shutdown()?
> 1: Maybe yes. A switch device will still be registered even if all user
>    ports failed to probe, see commit 86f8b1c01a0a ("net: dsa: Do not
>    make user port errors fatal"), and the cpu_dp->conduit pointers
>    remain valid.  I haven't audited all call paths to see whether they
>    will actually use the conduit in lack of any user port, but if they
>    do, it seems safer to not rely on user ports for that reference.
> 2. Definitely yes. We support changing the conduit which a user port is
>    associated to, and we can get into a situation where we've moved all
>    user ports away from a conduit, thus no longer hold any reference to
>    it via the net device tracker. But we shouldn't let it go nonetheless
>    - see the next change in relation to dsa_tree_find_first_conduit()
>    and LAG conduits which disappear.
>    We have to be prepared to return to the physical conduit, so the CPU
>    port must explicitly keep another reference to it. This is also to
>    say: the user ports and their CPU ports may not always keep a
>    reference to the same conduit net device, and both are needed.
>
> As for the conduit's kobject for the /sys/class/net/ entry, we don't
> care about it, we can release it as soon as we hold the net device
> object itself.
>
> History and blame attribution
> -----------------------------
>
> The code has been refactored so many times, it is very difficult to
> follow and properly attribute a blame, but I'll try to make a short
> history which I hope to be correct.
>
> We have two distinct probing paths:
> - one for OF, introduced in 2016 in commit 83c0afaec7b7 ("net: dsa: Add
>   new binding implementation")
> - one for non-OF, introduced in 2017 in commit 71e0bbde0d88 ("net: dsa:
>   Add support for platform data")
>
> These are both complete rewrites of the original probing paths (which
> used struct dsa_switch_driver and other weird stuff, instead of regular
> devices on their respective buses for register access, like MDIO, SPI,
> I2C etc):
> - one for OF, introduced in 2013 in commit 5e95329b701c ("dsa: add
>   device tree bindings to register DSA switches")
> - one for non-OF, introduced in 2015 in commit 91da11f870f0 ("net:
>   Distributed Switch Architecture protocol support")
>
> except for tiny bits and pieces like dsa_dev_to_net_device() which were
> seemingly carried over since the original commit, and used to this day.
>
> The point is that the original probing paths received a fix in 2015 in
> the form of commit 679fb46c5785 ("net: dsa: Add missing master netdev
> dev_put() calls"), but the fix never made it into the "new" (dsa2)
> probing paths that can still be traced to today, and the fixed probing
> path was later deleted in 2019 in commit 93e86b3bc842 ("net: dsa: Remove
> legacy probing support").
>
> That is to say, the new probing paths were never quite correct in this
> area.
>
> The existence of the legacy probing support which was deleted in 2019
> explains why dsa_dev_to_net_device() returns a conduit with elevated
> refcount (because it was supposed to be released during
> dsa_remove_dst()). After the removal of the legacy code, the only user
> of dsa_dev_to_net_device() calls dev_put(conduit) immediately after this
> function returns. This pattern makes no sense today, and can only be
> interpreted historically to understand why dev_hold() was there in the
> first place.
>
> Change details
> --------------
>
> Today we have a better netdev tracking infrastructure which we should
> use. It belongs in common code (dsa_port_parse_cpu()), which shows that
> the OF and non-OF code paths aren't actually so different.
>
> When dsa_port_parse_cpu() or any subsequent function during setup fails,
> dsa_switch_release_ports() will be called. However, dsa_port_parse_cpu()
> may fail prior to us assigning cpu_dp->conduit and bumping the reference.
> So we have to test for the conduit being NULL prior to calling
> netdev_put().
>
> There have still been so many transformations to the code since the
> blamed commits (rename master -> conduit, commit 0650bf52b31f ("net:
> dsa: be compatible with masters which unregister on shutdown")), that it
> only makes sense to fix the code using the best methods available today
> and see how it can be backported to stable later. I suspect the fix
> cannot even be backported to kernels which lack dsa_switch_shutdown(),
> and I suspect this is also maybe why the long-lived conduit reference
> didn't make it into the new DSA probing paths at the time (problems
> during shutdown).
>
> Because dsa_dev_to_net_device() has a single call site and has to be
> changed anyway, the logic was just absorbed into the non-OF
> dsa_port_parse().

Largely matches my observations.

>
> Tested on the ocelot/felix switch and on dsa_loop, both on the NXP
> LS1028A with CONFIG_DEBUG_KOBJECT_RELEASE=y.

Also tested on b53, but since it neither has multiple conduits nor LAG
support (yet), the testing was very limited.

Two minor nits/comments though ...

>
> Reported-by: Ma Ke <make24@iscas.ac.cn>
> Closes: https://lore.kernel.org/netdev/20251214131204.4684-1-make24@iscas.ac.cn/
> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
> Fixes: 71e0bbde0d88 ("net: dsa: Add support for platform data")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  include/net/dsa.h |  1 +
>  net/dsa/dsa.c     | 53 +++++++++++++++++++++++++----------------------
>  2 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index cced1a866757..6b2b5ed64ea4 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -302,6 +302,7 @@ struct dsa_port {
>         struct devlink_port     devlink_port;
>         struct phylink          *pl;
>         struct phylink_config   pl_config;
> +       netdevice_tracker       conduit_tracker;
>         struct dsa_lag          *lag;
>         struct net_device       *hsr_dev;
>
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index a20efabe778f..ac7900113d2b 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -1221,6 +1221,7 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *conduit,
>                 dst->tag_ops = tag_ops;
>         }
>
> +       netdev_hold(conduit, &dp->conduit_tracker, GFP_KERNEL);
>         dp->conduit = conduit;
>         dp->type = DSA_PORT_TYPE_CPU;
>         dsa_port_set_tag_protocol(dp, dst->tag_ops);
> @@ -1253,14 +1254,21 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
>         if (ethernet) {
>                 struct net_device *conduit;
>                 const char *user_protocol;
> +               int err;
>
> +               rtnl_lock();
>                 conduit = of_find_net_device_by_node(ethernet);
>                 of_node_put(ethernet);
> -               if (!conduit)
> +               if (!conduit) {
> +                       rtnl_unlock();
>                         return -EPROBE_DEFER;
> +               }
>
>                 user_protocol = of_get_property(dn, "dsa-tag-protocol", NULL);

Maybe move this to before the rtnl_lock()? Not sure if this makes a
difference (avoiding a lookup for netdev not yet there vs avoiding a
lookup under rtnl lock).

> -               return dsa_port_parse_cpu(dp, conduit, user_protocol);
> +               err = dsa_port_parse_cpu(dp, conduit, user_protocol);
> +               put_device(&conduit->dev);
> +               rtnl_unlock();
> +               return err;
>         }
>
>         if (link)
> @@ -1393,37 +1401,27 @@ static struct device *dev_find_class(struct device *parent, char *class)
>         return device_find_child(parent, class, dev_is_class);
>  }
>
> -static struct net_device *dsa_dev_to_net_device(struct device *dev)
> -{
> -       struct device *d;
> -
> -       d = dev_find_class(dev, "net");
> -       if (d != NULL) {
> -               struct net_device *nd;
> -
> -               nd = to_net_dev(d);
> -               dev_hold(nd);
> -               put_device(d);
> -
> -               return nd;
> -       }
> -
> -       return NULL;
> -}
> -
>  static int dsa_port_parse(struct dsa_port *dp, const char *name,
>                           struct device *dev)
>  {
>         if (!strcmp(name, "cpu")) {
>                 struct net_device *conduit;
> +               struct device *d;
> +               int err;
>
> -               conduit = dsa_dev_to_net_device(dev);
> -               if (!conduit)
> +               rtnl_lock();
> +               d = dev_find_class(dev, "net");
> +               if (!d) {
> +                       rtnl_unlock();
>                         return -EPROBE_DEFER;
> +               }
>
> -               dev_put(conduit);
> +               conduit = to_net_dev(d);
>
> -               return dsa_port_parse_cpu(dp, conduit, NULL);
> +               err = dsa_port_parse_cpu(dp, conduit, NULL);
> +               put_device(d);
> +               rtnl_unlock();
> +               return err;
>         }
>
>         if (!strcmp(name, "dsa"))
> @@ -1491,6 +1489,9 @@ static void dsa_switch_release_ports(struct dsa_switch *ds)
>         struct dsa_vlan *v, *n;
>
>         dsa_switch_for_each_port_safe(dp, next, ds) {
> +               if (dsa_port_is_cpu(dp) && dp->conduit)
> +                       netdev_put(dp->conduit, &dp->conduit_tracker);
> +
>                 /* These are either entries that upper layers lost track of
>                  * (probably due to bugs), or installed through interfaces
>                  * where one does not necessarily have to remove them, like
> @@ -1635,8 +1636,10 @@ void dsa_switch_shutdown(struct dsa_switch *ds)
>         /* Disconnect from further netdevice notifiers on the conduit,
>          * since netdev_uses_dsa() will now return false.
>          */
> -       dsa_switch_for_each_cpu_port(dp, ds)
> +       dsa_switch_for_each_cpu_port(dp, ds) {
> +               netdev_put(dp->conduit, &dp->conduit_tracker);
>                 dp->conduit->dsa_ptr = NULL;

We should probably call netdev_put() only after clearing
dp->conduit->ds_ptr, not before.

With that addressed,

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

> +       }
>
>         rtnl_unlock();
>  out:
> --
> 2.43.0
>

Re: [PATCH net 1/2] net: dsa: properly keep track of conduit reference

[Vladimir Oltean]

On Mon, Dec 15, 2025 at 11:13:58AM +0100, Jonas Gorski wrote:
> Hi,
> 
> On Sun, Dec 14, 2025 at 7:25 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > Problem description
> > -------------------
> >
> > DSA has a mumbo-jumbo of reference handling of the conduit net device
> > and its kobject which, sadly, is just wrong and doesn't make sense.
> >
> > There are two distinct problems.
> >
> > 1. The OF path, which uses of_find_net_device_by_node(), never releases
> >    the elevated refcount on the conduit's kobject. Nominally, the OF and
> >    non-OF paths should have identical code paths, and it is already
> >    suspicious that dsa_dev_to_net_device() has a put_device() call which
> >    is missing in dsa_port_parse_of(), but we can actually even verify
> >    that an issue exists. With CONFIG_DEBUG_KOBJECT_RELEASE=y, if we run
> >    this command "before" and "after" applying this patch:
> >
> > (unbind the conduit driver for net device eno2)
> > echo 0000:00:00.2 > /sys/bus/pci/drivers/fsl_enetc/unbind
> >
> > we see these lines in the output diff which appear only with the patch
> > applied:
> >
> > kobject: 'eno2' (ffff002009a3a6b8): kobject_release, parent 0000000000000000 (delayed 1000)
> > kobject: '109' (ffff0020099d59a0): kobject_release, parent 0000000000000000 (delayed 1000)
> >
> > 2. After we find the conduit interface one way (OF) or another (non-OF),
> >    it can get unregistered at any time, and DSA remains with a long-lived,
> >    but in this case stale, cpu_dp->conduit pointer. Holding the net
> >    device's underlying kobject isn't actually of much help, it just
> >    prevents it from being freed (but we never need that kobject
> >    directly). What helps us to prevent the net device from being
> >    unregistered is the parallel netdev reference mechanism (dev_hold()
> >    and dev_put()).
> >
> > Actually we actually use that netdev tracker mechanism implicitly on
> > user ports since commit 2f1e8ea726e9 ("net: dsa: link interfaces with
> > the DSA master to get rid of lockdep warnings"), via netdev_upper_dev_link().
> > But time still passes at DSA switch probe time between the initial
> > of_find_net_device_by_node() code and the user port creation time, time
> > during which the conduit could unregister itself and DSA wouldn't know
> > about it.
> >
> > So we have to run of_find_net_device_by_node() under rtnl_lock() to
> > prevent that from happening, and release the lock only with the netdev
> > tracker having acquired the reference.
> >
> > Do we need to keep the reference until dsa_unregister_switch() /
> > dsa_switch_shutdown()?
> > 1: Maybe yes. A switch device will still be registered even if all user
> >    ports failed to probe, see commit 86f8b1c01a0a ("net: dsa: Do not
> >    make user port errors fatal"), and the cpu_dp->conduit pointers
> >    remain valid.  I haven't audited all call paths to see whether they
> >    will actually use the conduit in lack of any user port, but if they
> >    do, it seems safer to not rely on user ports for that reference.
> > 2. Definitely yes. We support changing the conduit which a user port is
> >    associated to, and we can get into a situation where we've moved all
> >    user ports away from a conduit, thus no longer hold any reference to
> >    it via the net device tracker. But we shouldn't let it go nonetheless
> >    - see the next change in relation to dsa_tree_find_first_conduit()
> >    and LAG conduits which disappear.
> >    We have to be prepared to return to the physical conduit, so the CPU
> >    port must explicitly keep another reference to it. This is also to
> >    say: the user ports and their CPU ports may not always keep a
> >    reference to the same conduit net device, and both are needed.
> >
> > As for the conduit's kobject for the /sys/class/net/ entry, we don't
> > care about it, we can release it as soon as we hold the net device
> > object itself.
> >
> > History and blame attribution
> > -----------------------------
> >
> > The code has been refactored so many times, it is very difficult to
> > follow and properly attribute a blame, but I'll try to make a short
> > history which I hope to be correct.
> >
> > We have two distinct probing paths:
> > - one for OF, introduced in 2016 in commit 83c0afaec7b7 ("net: dsa: Add
> >   new binding implementation")
> > - one for non-OF, introduced in 2017 in commit 71e0bbde0d88 ("net: dsa:
> >   Add support for platform data")
> >
> > These are both complete rewrites of the original probing paths (which
> > used struct dsa_switch_driver and other weird stuff, instead of regular
> > devices on their respective buses for register access, like MDIO, SPI,
> > I2C etc):
> > - one for OF, introduced in 2013 in commit 5e95329b701c ("dsa: add
> >   device tree bindings to register DSA switches")
> > - one for non-OF, introduced in 2015 in commit 91da11f870f0 ("net:
> >   Distributed Switch Architecture protocol support")
> >
> > except for tiny bits and pieces like dsa_dev_to_net_device() which were
> > seemingly carried over since the original commit, and used to this day.
> >
> > The point is that the original probing paths received a fix in 2015 in
> > the form of commit 679fb46c5785 ("net: dsa: Add missing master netdev
> > dev_put() calls"), but the fix never made it into the "new" (dsa2)
> > probing paths that can still be traced to today, and the fixed probing
> > path was later deleted in 2019 in commit 93e86b3bc842 ("net: dsa: Remove
> > legacy probing support").
> >
> > That is to say, the new probing paths were never quite correct in this
> > area.
> >
> > The existence of the legacy probing support which was deleted in 2019
> > explains why dsa_dev_to_net_device() returns a conduit with elevated
> > refcount (because it was supposed to be released during
> > dsa_remove_dst()). After the removal of the legacy code, the only user
> > of dsa_dev_to_net_device() calls dev_put(conduit) immediately after this
> > function returns. This pattern makes no sense today, and can only be
> > interpreted historically to understand why dev_hold() was there in the
> > first place.
> >
> > Change details
> > --------------
> >
> > Today we have a better netdev tracking infrastructure which we should
> > use. It belongs in common code (dsa_port_parse_cpu()), which shows that
> > the OF and non-OF code paths aren't actually so different.
> >
> > When dsa_port_parse_cpu() or any subsequent function during setup fails,
> > dsa_switch_release_ports() will be called. However, dsa_port_parse_cpu()
> > may fail prior to us assigning cpu_dp->conduit and bumping the reference.
> > So we have to test for the conduit being NULL prior to calling
> > netdev_put().
> >
> > There have still been so many transformations to the code since the
> > blamed commits (rename master -> conduit, commit 0650bf52b31f ("net:
> > dsa: be compatible with masters which unregister on shutdown")), that it
> > only makes sense to fix the code using the best methods available today
> > and see how it can be backported to stable later. I suspect the fix
> > cannot even be backported to kernels which lack dsa_switch_shutdown(),
> > and I suspect this is also maybe why the long-lived conduit reference
> > didn't make it into the new DSA probing paths at the time (problems
> > during shutdown).
> >
> > Because dsa_dev_to_net_device() has a single call site and has to be
> > changed anyway, the logic was just absorbed into the non-OF
> > dsa_port_parse().
> 
> Largely matches my observations.
> 
> >
> > Tested on the ocelot/felix switch and on dsa_loop, both on the NXP
> > LS1028A with CONFIG_DEBUG_KOBJECT_RELEASE=y.
> 
> Also tested on b53, but since it neither has multiple conduits nor LAG
> support (yet), the testing was very limited.
> 
> Two minor nits/comments though ...
> >
> > Reported-by: Ma Ke <make24@iscas.ac.cn>
> > Closes: https://lore.kernel.org/netdev/20251214131204.4684-1-make24@iscas.ac.cn/
> > Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
> > Fixes: 71e0bbde0d88 ("net: dsa: Add support for platform data")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  include/net/dsa.h |  1 +
> >  net/dsa/dsa.c     | 53 +++++++++++++++++++++++++----------------------
> >  2 files changed, 29 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index cced1a866757..6b2b5ed64ea4 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -302,6 +302,7 @@ struct dsa_port {
> >         struct devlink_port     devlink_port;
> >         struct phylink          *pl;
> >         struct phylink_config   pl_config;
> > +       netdevice_tracker       conduit_tracker;
> >         struct dsa_lag          *lag;
> >         struct net_device       *hsr_dev;
> >
> > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> > index a20efabe778f..ac7900113d2b 100644
> > --- a/net/dsa/dsa.c
> > +++ b/net/dsa/dsa.c
> > @@ -1221,6 +1221,7 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *conduit,
> >                 dst->tag_ops = tag_ops;
> >         }
> >
> > +       netdev_hold(conduit, &dp->conduit_tracker, GFP_KERNEL);
> >         dp->conduit = conduit;
> >         dp->type = DSA_PORT_TYPE_CPU;
> >         dsa_port_set_tag_protocol(dp, dst->tag_ops);
> > @@ -1253,14 +1254,21 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
> >         if (ethernet) {
> >                 struct net_device *conduit;
> >                 const char *user_protocol;
> > +               int err;
> >
> > +               rtnl_lock();
> >                 conduit = of_find_net_device_by_node(ethernet);
> >                 of_node_put(ethernet);
> > -               if (!conduit)
> > +               if (!conduit) {
> > +                       rtnl_unlock();
> >                         return -EPROBE_DEFER;
> > +               }
> >
> >                 user_protocol = of_get_property(dn, "dsa-tag-protocol", NULL);
> 
> Maybe move this to before the rtnl_lock()? Not sure if this makes a
> difference (avoiding a lookup for netdev not yet there vs avoiding a
> lookup under rtnl lock).

If keeping the rtnl mutex acquired for too long is a concern (dsa_port_parse_cpu()
also calls request_module(), so there's that), I guess I could also reconsider
moving the netdev_hold() call right after the successful of_find_net_device_by_node()
call, and release the rtnl mutex right away. It's a tradeoff and I initially
considered that, but the error handling would become slightly more complex
and there would be some duplicated logic between the OF and non-OF paths.

> > -               return dsa_port_parse_cpu(dp, conduit, user_protocol);
> > +               err = dsa_port_parse_cpu(dp, conduit, user_protocol);
> > +               put_device(&conduit->dev);
> > +               rtnl_unlock();
> > +               return err;
> >         }
> >
> >         if (link)
> > @@ -1393,37 +1401,27 @@ static struct device *dev_find_class(struct device *parent, char *class)
> >         return device_find_child(parent, class, dev_is_class);
> >  }
> >
> > -static struct net_device *dsa_dev_to_net_device(struct device *dev)
> > -{
> > -       struct device *d;
> > -
> > -       d = dev_find_class(dev, "net");
> > -       if (d != NULL) {
> > -               struct net_device *nd;
> > -
> > -               nd = to_net_dev(d);
> > -               dev_hold(nd);
> > -               put_device(d);
> > -
> > -               return nd;
> > -       }
> > -
> > -       return NULL;
> > -}
> > -
> >  static int dsa_port_parse(struct dsa_port *dp, const char *name,
> >                           struct device *dev)
> >  {
> >         if (!strcmp(name, "cpu")) {
> >                 struct net_device *conduit;
> > +               struct device *d;
> > +               int err;
> >
> > -               conduit = dsa_dev_to_net_device(dev);
> > -               if (!conduit)
> > +               rtnl_lock();
> > +               d = dev_find_class(dev, "net");
> > +               if (!d) {
> > +                       rtnl_unlock();
> >                         return -EPROBE_DEFER;
> > +               }
> >
> > -               dev_put(conduit);
> > +               conduit = to_net_dev(d);
> >
> > -               return dsa_port_parse_cpu(dp, conduit, NULL);
> > +               err = dsa_port_parse_cpu(dp, conduit, NULL);
> > +               put_device(d);
> > +               rtnl_unlock();
> > +               return err;
> >         }
> >
> >         if (!strcmp(name, "dsa"))
> > @@ -1491,6 +1489,9 @@ static void dsa_switch_release_ports(struct dsa_switch *ds)
> >         struct dsa_vlan *v, *n;
> >
> >         dsa_switch_for_each_port_safe(dp, next, ds) {
> > +               if (dsa_port_is_cpu(dp) && dp->conduit)
> > +                       netdev_put(dp->conduit, &dp->conduit_tracker);
> > +
> >                 /* These are either entries that upper layers lost track of
> >                  * (probably due to bugs), or installed through interfaces
> >                  * where one does not necessarily have to remove them, like
> > @@ -1635,8 +1636,10 @@ void dsa_switch_shutdown(struct dsa_switch *ds)
> >         /* Disconnect from further netdevice notifiers on the conduit,
> >          * since netdev_uses_dsa() will now return false.
> >          */
> > -       dsa_switch_for_each_cpu_port(dp, ds)
> > +       dsa_switch_for_each_cpu_port(dp, ds) {
> > +               netdev_put(dp->conduit, &dp->conduit_tracker);
> >                 dp->conduit->dsa_ptr = NULL;
> 
> We should probably call netdev_put() only after clearing
> dp->conduit->ds_ptr, not before.

Ok.

> With that addressed,
> 
> Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

Thanks for the feedback.

> 
> > +       }
> >
> >         rtnl_unlock();
> >  out:
> > --
> > 2.43.0
> >