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

[netdev-bot+sashiko@kernel.org]

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