[PATCH net-next 0/2] dpaa2-eth: devlink port number from the DPMAC

[PATCH net-next 0/2] dpaa2-eth: devlink port number from the DPMAC

[Vincent Jardin via B4 Relay]

dpaa2-eth registers every DPNI as a physical devlink port without a
port number, so all the DPAA2 interfaces report the same
phys_port_name "p0", including the DPNIs that have no DPMAC behind
them.

Patch 1 uses the DPMAC id as the physical port number. The attributes
of a registered devlink port cannot change, so when the DPNI endpoint
changes at runtime a warning says the number is stale until the DPNI
is rebound.

Patch 2 registers the DPNIs without a DPMAC as virtual ports. They
have no phys_port_name, so "p<dpmac id>" is unique.

With both patches, udev can name an interface using its DPMAC, for
instance:

  SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", \
    ATTR{phys_port_name}=="p3", NAME="dpmac3"

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
Vincent Jardin (2):
      dpaa2-eth: use the DPMAC id as the devlink physical port number
      dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

 .../ethernet/freescale/dpaa2/dpaa2-eth-devlink.c   | 35 +++++++++++++++++++++-
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c   |  2 ++
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h   |  1 +
 3 files changed, 37 insertions(+), 1 deletion(-)
---
base-commit: 944ae66642b726bd6b25ae71b1e9ff88a0e0bdb0
change-id: 20260923-for-upstream-dpaa2_ports-04087b33bb8f

Best regards,
-- 
Vincent Jardin <vjardin@free.fr>

[PATCH net-next 1/2] dpaa2-eth: use the DPMAC id as the devlink physical port number

[Vincent Jardin via B4 Relay]

From: Vincent Jardin <vjardin@free.fr>

dpaa2-eth registers every DPNI as a devlink port of flavour physical
without setting a port number. All DPAA2 interfaces therefore report
the same phys_port_name, p0 (see devlink port show).

The DPNI endpoint can change at runtime while the attributes of a
registered devlink port cannot, so log a warning when it happens.

Another benefit: udev rules can match ATTR{phys_port_name} to name an
interface based on its DPMAC.

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
 .../ethernet/freescale/dpaa2/dpaa2-eth-devlink.c   | 26 ++++++++++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c   |  2 ++
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h   |  1 +
 3 files changed, 29 insertions(+)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
index 8775c931106b..8b2addd3f83a 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
@@ -213,16 +213,42 @@ void dpaa2_eth_dl_unregister(struct dpaa2_eth_priv *priv)
 	devlink_unregister(priv->devlink);
 }
 
+static u32 dpaa2_eth_dl_port_number(struct dpaa2_eth_priv *priv)
+{
+	u32 port_number = 0;
+
+	mutex_lock(&priv->mac_lock);
+	if (priv->mac)
+		port_number = priv->mac->mc_dev->obj_desc.id;
+	mutex_unlock(&priv->mac_lock);
+
+	return port_number;
+}
+
 int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
 {
 	struct devlink_port *devlink_port = &priv->devlink_port;
 	struct devlink_port_attrs attrs = {};
 
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
 	devlink_port_attrs_set(devlink_port, &attrs);
 	return devlink_port_register(priv->devlink, devlink_port, 0);
 }
 
+void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
+{
+	u32 registered = priv->devlink_port.attrs.phys.port_number;
+	u32 current_number = dpaa2_eth_dl_port_number(priv);
+
+	if (registered == current_number)
+		return;
+
+	netdev_warn(priv->net_dev,
+		    "devlink port number %u is stale, the DPMAC endpoint is now %u (0: none); rebind the DPNI to update it\n",
+		    registered, current_number);
+}
+
 void dpaa2_eth_dl_port_del(struct dpaa2_eth_priv *priv)
 {
 	struct devlink_port *devlink_port = &priv->devlink_port;
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 6f1046c9cc51..def2d5c0e7f7 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -4771,6 +4771,8 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg)
 			dpaa2_eth_disconnect_mac(priv);
 		else
 			dpaa2_eth_connect_mac(priv);
+
+		dpaa2_eth_dl_port_check(priv);
 	}
 
 	return IRQ_HANDLED;
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index 834cba8c3a41..071a3d23cf1e 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -802,6 +802,7 @@ void dpaa2_eth_dl_unregister(struct dpaa2_eth_priv *priv);
 
 int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv);
 void dpaa2_eth_dl_port_del(struct dpaa2_eth_priv *priv);
+void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv);
 
 int dpaa2_eth_dl_traps_register(struct dpaa2_eth_priv *priv);
 void dpaa2_eth_dl_traps_unregister(struct dpaa2_eth_priv *priv);

-- 
2.43.0

Re: [PATCH net-next 1/2] dpaa2-eth: use the DPMAC id as the devlink physical port number

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] dpaa2_eth_dl_port_check() gets its 'current' value from
  dpaa2_eth_dl_port_number(), which reads only the driver's cached…
- [Low] In dpaa2_eth_probe(), dpaa2_eth_setup_irqs() (dpaa2-eth.c:4997)
  enables the ENDPOINT_CHANGED IRQ before dpaa2_eth_dl_port_add()…

Pre-existing issues:
- [High] This problem was not introduced by this patch, but the new
  dpaa2_eth_dl_port_check() adds another priv access on the same path.

--

dpaa2-eth: use the DPMAC id as the devlink physical port number

This commit sets each DPNI's devlink physical port number to the id of
its connected DPMAC, so interfaces no longer all report p0 as
phys_port_name. Devlink port attributes cannot change after
registration. For that reason, the endpoint-changed IRQ handler logs a
warning when the DPMAC endpoint no longer matches the registered number.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> index 8775c931106b4..8b2addd3f83ae 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> @@ -213,16 +213,42 @@ void dpaa2_eth_dl_unregister(struct dpaa2_eth_priv *priv)

[ ... ]

>  int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
>  {
>  	struct devlink_port *devlink_port = &priv->devlink_port;
>  	struct devlink_port_attrs attrs = {};
>  
>  	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
>  	devlink_port_attrs_set(devlink_port, &attrs);
>  	return devlink_port_register(priv->devlink, devlink_port, 0);
>  }
>  
> +void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
> +{
> +	u32 registered = priv->devlink_port.attrs.phys.port_number;
> +	u32 current_number = dpaa2_eth_dl_port_number(priv);

[Severity: Low]
Can this read of priv->devlink_port.attrs race with probe and remove?

In dpaa2_eth_probe(), the endpoint-changed IRQ is enabled before the
devlink port attributes are filled in:

dpaa2_eth_probe() {
    ...
	err = dpaa2_eth_setup_irqs(dpni_dev);
    ...
	err = dpaa2_eth_dl_port_add(priv);
    ...
}

dpaa2_eth_dl_port_check() reads attrs.phys.port_number without a lock
and without checking that the port is registered. The mac_lock only
covers the current_number side.

dpaa2_eth_dl_port_add() reads the DPMAC id under mac_lock. It only
publishes that id after dropping the lock, through the plain struct copy
in devlink_port_attrs_set().

Consider this interleaving:

probe:
dpaa2_eth_dl_port_add()
    dpaa2_eth_dl_port_number()   <- reads DPMAC id N, drops mac_lock

IRQ thread, ENDPOINT_CHANGED with had_mac == true:
dpaa2_eth_disconnect_mac()       <- priv->mac = NULL
dpaa2_eth_dl_port_check()
    registered = 0               <- attrs still zeroed
    current_number = 0           <- returns without a warning

probe:
    devlink_port_attrs_set()     <- publishes port_number = N
    devlink_port_register()

Wouldn't this register pN for a DPNI that no longer has a DPMAC, and
never print the warning? The attrs are zeroed by alloc_etherdev(), and
DEVLINK_PORT_FLAVOUR_PHYSICAL is 0, so registered reads as 0 here.

Now take the opposite order: DPMAC M is connected before
dpaa2_eth_dl_port_add() runs. That case seems to print a spurious
"0 is stale, now M" warning, and then M is registered correctly.

The unlocked read also races with the struct copy in
devlink_port_attrs_set(), which KCSAN would likely flag.

On the remove side, dpaa2_eth_remove() calls dpaa2_eth_dl_port_del()
before fsl_mc_free_irqs(). The check can therefore also run against a
port that is already unregistered.

This still seems to be present after the follow-up patch in this series.
That patch adds a read of attrs.flavour on the same unlocked path.

> +
> +	if (registered == current_number)
> +		return;
> +
> +	netdev_warn(priv->net_dev,
> +		    "devlink port number %u is stale, the DPMAC endpoint is now %u (0: none); rebind the DPNI to update it\n",
> +		    registered, current_number);
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 6f1046c9cc515..def2d5c0e7f7e 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -4771,6 +4771,8 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg)
>  			dpaa2_eth_disconnect_mac(priv);
>  		else
>  			dpaa2_eth_connect_mac(priv);
> +
> +		dpaa2_eth_dl_port_check(priv);

[Severity: Low]
Does the "current" value used by dpaa2_eth_dl_port_check() really
reflect the firmware endpoint?

dpaa2_eth_dl_port_number() only looks at the driver's cached priv->mac.
The warning text, however, says "the DPMAC endpoint is now %u (0: none)".

The return value of dpaa2_eth_connect_mac() is ignored here. Suppose
fsl_mc_get_endpoint() finds a real DPMAC but the connect then fails, for
example on a kzalloc failure, a dpaa2_mac_open() error, or
dpaa2_mac_connect() returning -EPROBE_DEFER or another error. In that
case priv->mac stays NULL and current_number is 0.

If the registered number is N, wouldn't the warning then say the
endpoint is 0 (none) while firmware actually has a DPMAC attached?

If the registered value is 0, registered == current_number and nothing
is printed. That happens even though the DPNI is now connected to a
DPMAC in firmware and its devlink attributes are stale. The
-EPROBE_DEFER case is especially quiet, because dpaa2_eth_connect_mac()
only reports it with netdev_dbg().

Also, when had_mac is true the handler only disconnects. Take a normal
disconnect followed by a reconnect to the same dpmac.N. The disconnect
prints the "stale ... rebind the DPNI" warning, but the reconnect, which
makes the attributes correct again, prints nothing. Could this push
users into a rebind that isn't needed?

This also seems to remain after the follow-up patch in the series, which
only changes how registered is computed.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
the new dpaa2_eth_dl_port_check() call adds another priv access on this
path. Can dpni_irq0_handler_thread() run after priv has been freed?

dpaa2_eth_setup_irqs() installs the handler with
devm_request_threaded_irq(). Neither dpaa2_eth_remove() nor the probe
error unwind calls devm_free_irq() or synchronize_irq(). Both only call
fsl_mc_free_irqs(), which just returns the IRQ resources to the pool:

drivers/bus/fsl-mc/fsl-mc-allocator.c:fsl_mc_free_irqs() {
    ...
	for (i = 0; i < irq_count; i++) {
		irqs[i]->mc_dev = NULL;
		fsl_mc_resource_free(&irqs[i]->resource);
	}

	mc_dev->irqs = NULL;
}

Both paths then free the portal and the netdev:

dpaa2_eth_remove() {
    ...
	fsl_mc_free_irqs(ls_dev);
    ...
	fsl_mc_portal_free(priv->mc_io);
    ...
	free_netdev(net_dev);
}

devres only releases the managed IRQ after the remove or probe callback
returns. Suppose a LINK_CHANGED or ENDPOINT_CHANGED thread is already
running in that window, for example blocked on the shared mc_io portal.
Wouldn't it use the freed portal through dpni_dev->mc_io (set to
priv->mc_io in dpaa2_eth_setup_dpni())? It would also touch priv->mac,
priv->mac_lock and now priv->devlink_port.attrs, all of which live in
the freed netdev.

A link flap from the remote partner seems enough to hit this. The flap
would have to arrive during unbind, or during a late probe failure after
dpaa2_eth_setup_irqs() succeeded.

>  	}
>  
>  	return IRQ_HANDLED;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-for-upstream-dpaa2_ports-v1-0-b59c865edd00%40free.fr

Re: [PATCH net-next 1/2] dpaa2-eth: use the DPMAC id as the devlink physical port number

[Vincent Jardin]

Hi M. Bot,

See below

> > +void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
> > +{
> > +	u32 registered = priv->devlink_port.attrs.phys.port_number;
> > +	u32 current_number = dpaa2_eth_dl_port_number(priv);
> 
> [Severity: Low]
> Can this read of priv->devlink_port.attrs race with probe and remove?

Yes, see v2. I did add the port number in priv under mac_lock.

> [Severity: Low]
> Does the "current" value used by dpaa2_eth_dl_port_check() really
> reflect the firmware endpoint?

Yes, it does. The 0: non was missleading.

The prints is updated following the reiews and Iona's comments, see v2.

> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch, but
> the new dpaa2_eth_dl_port_check() call adds another priv access on this
> path. Can dpni_irq0_handler_thread() run after priv has been freed?

I could be, it was there before, so let's avoid unfocusing this serie.

Best regards,
  Vincent

Re: [PATCH net-next 1/2] dpaa2-eth: use the DPMAC id as the devlink physical port number

[Ioana Ciornei]

On Wed, Sep 23, 2026 at 02:58:00PM +0200, Vincent Jardin via B4 Relay wrote:
> From: Vincent Jardin <vjardin@free.fr>
> 
> dpaa2-eth registers every DPNI as a devlink port of flavour physical
> without setting a port number. All DPAA2 interfaces therefore report
> the same phys_port_name, p0 (see devlink port show).
> 
> The DPNI endpoint can change at runtime while the attributes of a
> registered devlink port cannot, so log a warning when it happens.
> 
> Another benefit: udev rules can match ATTR{phys_port_name} to name an
> interface based on its DPMAC.
> 
> Signed-off-by: Vincent Jardin <vjardin@free.fr>
> ---
>  .../ethernet/freescale/dpaa2/dpaa2-eth-devlink.c   | 26 ++++++++++++++++++++++
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c   |  2 ++
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h   |  1 +
>  3 files changed, 29 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> index 8775c931106b..8b2addd3f83a 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> @@ -213,16 +213,42 @@ void dpaa2_eth_dl_unregister(struct dpaa2_eth_priv *priv)
>  	devlink_unregister(priv->devlink);
>  }
>  
> +static u32 dpaa2_eth_dl_port_number(struct dpaa2_eth_priv *priv)
> +{
> +	u32 port_number = 0;
> +
> +	mutex_lock(&priv->mac_lock);
> +	if (priv->mac)
> +		port_number = priv->mac->mc_dev->obj_desc.id;
> +	mutex_unlock(&priv->mac_lock);
> +
> +	return port_number;
> +}
> +
>  int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
>  {
>  	struct devlink_port *devlink_port = &priv->devlink_port;
>  	struct devlink_port_attrs attrs = {};
>  
>  	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
>  	devlink_port_attrs_set(devlink_port, &attrs);
>  	return devlink_port_register(priv->devlink, devlink_port, 0);
>  }
>  
> +void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
> +{
> +	u32 registered = priv->devlink_port.attrs.phys.port_number;
> +	u32 current_number = dpaa2_eth_dl_port_number(priv);
> +
> +	if (registered == current_number)
> +		return;
> +
> +	netdev_warn(priv->net_dev,
> +		    "devlink port number %u is stale, the DPMAC endpoint is now %u (0: none); rebind the DPNI to update it\n",
> +		    registered, current_number);

I agree with sashiko's feedback on this warning message. I would prefer
a simple "devlink port number %u is stable, rebind to update".

Ioana

Re: [PATCH net-next 1/2] dpaa2-eth: use the DPMAC id as the devlink physical port number

[Vincent Jardin]

Hi Ioana,

> I agree with sashiko's feedback on this warning message. I would prefer
> a simple "devlink port number %u is stable, rebind to update".

Done, see v2.

Thanks for your reviews of the patch and Sashiko's comments,
  Vincent

[PATCH net-next 2/2] dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

[Vincent Jardin via B4 Relay]

From: Vincent Jardin <vjardin@free.fr>

A DPNI whose endpoint is not a DPMAC has no physical connector: it
may have no endpoint at all, or be connected to another DPNI or to a
DPSW port. It is still registered as a physical devlink port, number 0,
so userspace sees a physical port that does not exist.

A virtual port has no phys_port_name, so only the DPNIs that serve a
DPMAC report one, and "p<dpmac id>" never collides.

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
index 8b2addd3f83a..30e6cb2190db 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
@@ -230,16 +230,23 @@ int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
 	struct devlink_port *devlink_port = &priv->devlink_port;
 	struct devlink_port_attrs attrs = {};
 
-	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
 	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
+	if (attrs.phys.port_number)
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+	else
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL;
 	devlink_port_attrs_set(devlink_port, &attrs);
 	return devlink_port_register(priv->devlink, devlink_port, 0);
 }
 
 void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
 {
-	u32 registered = priv->devlink_port.attrs.phys.port_number;
+	const struct devlink_port_attrs *attrs = &priv->devlink_port.attrs;
 	u32 current_number = dpaa2_eth_dl_port_number(priv);
+	u32 registered = 0;
+
+	if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PHYSICAL)
+		registered = attrs->phys.port_number;
 
 	if (registered == current_number)
 		return;

-- 
2.43.0

Re: [PATCH net-next 2/2] dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] dpaa2_eth_probe() calls dpaa2_eth_setup_irqs()
  (dpaa2-eth.c:4997), which enables DPNI_IRQ_EVENT_ENDPOINT_CHANGED,…
- [Low] The commit message says that making DPNIs without a DPMAC virtual
  means "only the DPNIs that serve a DPMAC report one, and \"p<dpmac…
- [Low] After this patch, a DPNI with no DPMAC is registered as
  DEVLINK_PORT_FLAVOUR_VIRTUAL.

--

dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

This commit registers a DPNI's devlink port as DEVLINK_PORT_FLAVOUR_VIRTUAL
when the DPNI has no DPMAC endpoint at probe time. Otherwise the port is
DEVLINK_PORT_FLAVOUR_PHYSICAL and numbered by the DPMAC id.
dpaa2_eth_dl_port_check() now counts a virtual port as port number 0 when
it compares against the current endpoint.

> A virtual port has no phys_port_name, so only the DPNIs that serve a
> DPMAC report one, and "p<dpmac id>" never collides.

[Severity: Low]
Is this still true when the topology changes after probe?

dpaa2_eth_dl_port_add() picks the flavour and number once, from priv->mac
at probe time. When DPNI_IRQ_EVENT_ENDPOINT_CHANGED fires later,
dpni_irq0_handler_thread() connects or disconnects the MAC. It then only
calls dpaa2_eth_dl_port_check(), which prints a warning and keeps the old
attrs.

For example, DPNI A is probed while connected to dpmac.3 and registers a
physical "p3" port. dpmac.3 is then disconnected from A and connected to
DPNI B, which is probed afterwards (for example, created with restool and
found by the DPRC rescan). B also registers a physical "p3" port, so two
netdevs report phys_port_name "p3".

The same limit applies in two other cases:

- A DPNI probed with no endpoint stays virtual, with no phys_port_name,
  after a DPMAC is connected to it.
- A DPNI whose DPMAC was disconnected stays a physical port with no
  connector behind it.

The earlier patch in this series already says the registered attributes
cannot change at runtime, and it warns the user to rebind. So the issue
here is the wording.

Could the commit message limit the claim to endpoints that are fixed at
probe time, or that are followed by a rebind?

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> index 8b2addd3f83ae..30e6cb2190dbd 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> @@ -230,16 +230,23 @@ int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
>  	struct devlink_port *devlink_port = &priv->devlink_port;
>  	struct devlink_port_attrs attrs = {};
>  
> -	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>  	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
> +	if (attrs.phys.port_number)
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +	else
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL;
>  	devlink_port_attrs_set(devlink_port, &attrs);

[Severity: Medium]
Can this race with the endpoint changed IRQ thread during probe?

dpaa2_eth_probe() enables the IRQ before it sets up the devlink port:

dpaa2_eth_probe() {
    ...
	err = dpaa2_eth_setup_irqs(dpni_dev);
    ...
	err = dpaa2_eth_dl_port_add(priv);
    ...
}

dpaa2_eth_dl_port_number() reads the DPMAC id under mac_lock. The flavour
choice and the struct copy in devlink_port_attrs_set() happen after the
lock is dropped. dpaa2_eth_dl_port_check() reads priv->devlink_port.attrs
without taking any lock.

One possible interleaving:

probe thread:
dpaa2_eth_dl_port_add()
  dpaa2_eth_dl_port_number()        /* returns N */

IRQ thread:
dpni_irq0_handler_thread()
  dpaa2_eth_disconnect_mac()        /* priv->mac = NULL */
  dpaa2_eth_dl_port_check()
    /* attrs are still zeroed from alloc_etherdev_mq(), flavour reads
       as DEVLINK_PORT_FLAVOUR_PHYSICAL (0), registered = 0,
       current_number = 0, no warning */

probe thread:
  attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL   /* number N */
  devlink_port_attrs_set()
  devlink_port_register()

The port is then registered as a physical "pN" port for a DPNI that no
longer has a DPMAC, and nothing is logged. In the reverse case, where a
DPMAC is connected inside this window, the check prints a spurious "port
number 0 is stale" warning.

The plain read and write of devlink_port.attrs at the same time also
looks like a data race that KCSAN would report.

Would any of these avoid the race?

- Register the devlink port before dpaa2_eth_setup_irqs().
- Hold mac_lock across the number read and devlink_port_attrs_set().
- Skip the check until the port is registered.

>  	return devlink_port_register(priv->devlink, devlink_port, 0);
>  }
>  
>  void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
>  {
> -	u32 registered = priv->devlink_port.attrs.phys.port_number;
> +	const struct devlink_port_attrs *attrs = &priv->devlink_port.attrs;
>  	u32 current_number = dpaa2_eth_dl_port_number(priv);
> +	u32 registered = 0;
> +
> +	if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PHYSICAL)
> +		registered = attrs->phys.port_number;
>  
>  	if (registered == current_number)
>  		return;

[Severity: Low]
Is the warning that follows still accurate for a virtual port?

	netdev_warn(priv->net_dev,
		    "devlink port number %u is stale, the DPMAC endpoint is now %u (0: none); rebind the DPNI to update it\n",
		    registered, current_number);

When a virtual DPNI later gains a DPMAC, this prints "devlink port number
0 is stale, the DPMAC endpoint is now N".

Devlink core does not report a port number for virtual ports.
devlink_nl_port_attrs_put() emits DEVLINK_ATTR_PORT_NUMBER only for the
physical, cpu and dsa flavours. __devlink_port_phys_port_name_get()
returns -EOPNOTSUPP for virtual ports.

So the message names a port number that devlink port show never
displayed. It also does not say that the stale parts are the flavour and
the missing phys_port_name.

Could the wording be updated for the virtual case?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-for-upstream-dpaa2_ports-v1-0-b59c865edd00%40free.fr

Re: [PATCH net-next 2/2] dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

[Vincent Jardin]

Hi Mr Bot,

> [Severity: Medium]
> Can this race with the endpoint changed IRQ thread during probe?

Yes, see v2, patch 1/2.

> [Severity: Low]
> Is this still true when the topology changes after probe?

No, let's focus on the DPNIs for DPMAC.

> [Severity: Low]
> Is the warning that follows still accurate for a virtual port?

Yes, but with an edit based on Ioana's reviews.

pw-bot: cr

best regards,
  Vincent

Re: [PATCH net-next 2/2] dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

[Ioana Ciornei]

On Wed, Sep 23, 2026 at 02:58:01PM +0200, Vincent Jardin via B4 Relay wrote:
> From: Vincent Jardin <vjardin@free.fr>
> 
> A DPNI whose endpoint is not a DPMAC has no physical connector: it
> may have no endpoint at all, or be connected to another DPNI or to a
> DPSW port. It is still registered as a physical devlink port, number 0,
> so userspace sees a physical port that does not exist.

Reword this so that it's clear that you are describing the state before
the patch.

> 
> A virtual port has no phys_port_name, so only the DPNIs that serve a
> DPMAC report one, and "p<dpmac id>" never collides.

Reword this as well and mention directly what you are changing.

I would also suggest a change in the subject title:
 dpaa2-eth: mark DPNIs without a DPMAC as virtual devlink ports

> 
> Signed-off-by: Vincent Jardin <vjardin@free.fr>
> ---
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> index 8b2addd3f83a..30e6cb2190db 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c
> @@ -230,16 +230,23 @@ int dpaa2_eth_dl_port_add(struct dpaa2_eth_priv *priv)
>  	struct devlink_port *devlink_port = &priv->devlink_port;
>  	struct devlink_port_attrs attrs = {};
>  
> -	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>  	attrs.phys.port_number = dpaa2_eth_dl_port_number(priv);
> +	if (attrs.phys.port_number)
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +	else
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL;
>  	devlink_port_attrs_set(devlink_port, &attrs);
>  	return devlink_port_register(priv->devlink, devlink_port, 0);
>  }
>  
>  void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
>  {
> -	u32 registered = priv->devlink_port.attrs.phys.port_number;
> +	const struct devlink_port_attrs *attrs = &priv->devlink_port.attrs;
>  	u32 current_number = dpaa2_eth_dl_port_number(priv);
> +	u32 registered = 0;

Move these changes to patch 1/2.

Ioana

Re: [PATCH net-next 2/2] dpaa2-eth: DPNIs without a DPMAC should be virtual devlink ports

[Vincent Jardin]

Hi Ioana,

> > A DPNI whose endpoint is not a DPMAC has no physical connector: it
> > may have no endpoint at all, or be connected to another DPNI or to a
> > DPSW port. It is still registered as a physical devlink port, number 0,
> > so userspace sees a physical port that does not exist.
> 
> Reword this so that it's clear that you are describing the state before
> the patch.

Done, see v2

> > A virtual port has no phys_port_name, so only the DPNIs that serve a
> > DPMAC report one, and "p<dpmac id>" never collides.
> 
> Reword this as well and mention directly what you are changing.

Done,

> 
> I would also suggest a change in the subject title:
>  dpaa2-eth: mark DPNIs without a DPMAC as virtual devlink ports

OK, thanks for the suggestion

> >  void dpaa2_eth_dl_port_check(struct dpaa2_eth_priv *priv)
> >  {
> > -	u32 registered = priv->devlink_port.attrs.phys.port_number;
> > +	const struct devlink_port_attrs *attrs = &priv->devlink_port.attrs;
> >  	u32 current_number = dpaa2_eth_dl_port_number(priv);
> > +	u32 registered = 0;
> 
> Move these changes to patch 1/2.


ok, done

Thanks for the comments,
  Vincent

Re: [PATCH net-next 0/2] dpaa2-eth: devlink port number from the DPMAC

[Ioana Ciornei]

On Wed, Sep 23, 2026 at 02:57:59PM +0200, Vincent Jardin via B4 Relay wrote:
> dpaa2-eth registers every DPNI as a physical devlink port without a
> port number, so all the DPAA2 interfaces report the same
> phys_port_name "p0", including the DPNIs that have no DPMAC behind
> them.
> 
> Patch 1 uses the DPMAC id as the physical port number. The attributes
> of a registered devlink port cannot change, so when the DPNI endpoint
> changes at runtime a warning says the number is stale until the DPNI
> is rebound.
> 
> Patch 2 registers the DPNIs without a DPMAC as virtual ports. They
> have no phys_port_name, so "p<dpmac id>" is unique.
> 
> With both patches, udev can name an interface using its DPMAC, for
> instance:
> 
>   SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", \
>     ATTR{phys_port_name}=="p3", NAME="dpmac3"

Is consistent naming the end goal? Because renaming can already be done
for DPAA2 network interfaces based on the of_node. I usually have
something like below in my udev rules file:

SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", \
	ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@1", NAME="endpmac1"

Anyhow, I will give the series a spin tomorrow.

Ioana

Re: [PATCH net-next 0/2] dpaa2-eth: devlink port number from the DPMAC

[Vincent Jardin]

Hi Ioana,

> Is consistent naming the end goal? Because renaming can already be done
> for DPAA2 network interfaces based on the of_node. I usually have
> something like below in my udev rules file:
> 
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", \
> 	ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@1", NAME="endpmac1"

It was the root of my initial investigation, but I need more:
  - I need to have an attribute that I can use from the userland to name and
    rename many times, so in between it I would need something to rely with
  - cosmetic: just have a propver devlink

> Anyhow, I will give the series a spin tomorrow.

Thanks: it should not hurt, and devlink should be more coherent then.

best regards,
  Vincent

Re: [PATCH net-next 0/2] dpaa2-eth: devlink port number from the DPMAC

[Ioana Ciornei]

On Wed, Sep 23, 2026 at 02:57:59PM +0200, Vincent Jardin via B4 Relay wrote:
> dpaa2-eth registers every DPNI as a physical devlink port without a
> port number, so all the DPAA2 interfaces report the same
> phys_port_name "p0", including the DPNIs that have no DPMAC behind
> them.
> 
> Patch 1 uses the DPMAC id as the physical port number. The attributes
> of a registered devlink port cannot change, so when the DPNI endpoint
> changes at runtime a warning says the number is stale until the DPNI
> is rebound.
> 
> Patch 2 registers the DPNIs without a DPMAC as virtual ports. They
> have no phys_port_name, so "p<dpmac id>" is unique.
> 
> With both patches, udev can name an interface using its DPMAC, for
> instance:
> 
>   SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", \
>     ATTR{phys_port_name}=="p3", NAME="dpmac3"
> 
> Signed-off-by: Vincent Jardin <vjardin@free.fr>

Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>