Re: [PATCH net-next v4 7/8] net: bcmgenet: allow the MTU to be changed

[netdev-bot+sashiko]

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

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

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] An RX filter update can be lost during the MTU change.
- [Medium] The live MTU change leaves the PHY state machine running
  (bcmgenet_netif_stop(dev, false)), but nothing stops the PHY link…
- [Low] The patch fixes two existing bugs, but neither the commit message
  nor a Fixes: tag mentions them.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  priv->datapath_up and leaves this path unguarded.

--

net: bcmgenet: allow the MTU to be changed

This commit sets dev->max_mtu for GENET v2 and later, derived from the
packet ready threshold and the one page descriptor limit. It adds an
ndo_change_mtu that rebuilds the DMA rings and MTU registers in place while
the PHY keeps running, and retries the old MTU if allocation fails. A new
priv->datapath_up flag tracks whether the datapath has already been torn
down.

Link: https://github.com/raspberrypi/linux/issues/5561

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e00661b2414b9..f158d6bc314b9 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -3179,8 +3185,12 @@ static int bcmgenet_init_rx_queues(struct net_device *dev)
>  	/* Initialize Rx priority queues */
>  	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
>  		ret = bcmgenet_init_rx_ring(priv, i, end - start, start, end);
> -		if (ret)
> +		if (ret) {
> +			/* Only the rings before this one hold a NAPI */
> +			while (i--)
> +				netif_napi_del(&priv->rx_rings[i].napi);
>  			return ret;
> +		}

[Severity: Low]
This isn't a bug, but this unwind and the new priv->datapath_up handling in
bcmgenet_close(), bcmgenet_suspend() and bcmgenet_resume() look like they
also fix two existing problems. The commit message doesn't mention either
of them.

Before this patch, if bcmgenet_init_rx_ring() failed for ring i, the NAPIs
of rings 0..i-1 stayed listed after a failed bcmgenet_open() or
bcmgenet_resume(). The next netif_napi_add() would then hit the
already-listed warning.

Also, bcmgenet_close() used to call bcmgenet_netif_stop() unconditionally.
A close after a failed resume would call napi_disable() on NAPIs that were
already disabled during suspend, and then kfree() rx_cbs/tx_cbs a second
time. That path has nothing to do with the MTU, but the new comment in
bcmgenet_close() only says:

    /* A failed MTU change can have torn the datapath down already */

Could these fixes be split into separate patches with Fixes: tags so they
can go to net and stable? The rx unwind is needed for the MTU retry, which
calls bcmgenet_init_dma() twice in a row, so it could come first in the
series.

[ ... ]

> @@ -3622,7 +3633,11 @@ static int bcmgenet_close(struct net_device *dev)
>  
>  	netif_dbg(priv, ifdown, dev, "bcmgenet_close\n");
>  
> -	bcmgenet_netif_stop(dev, false);
> +	/* A failed MTU change can have torn the datapath down already */
> +	if (priv->datapath_up) {
> +		bcmgenet_netif_stop(dev, false);
> +		priv->datapath_up = false;
> +	}

[ ... ]

> +static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu)
> +{

[ ... ]

> +	/* The watchdog trips on an idle queue once the rings are gone */
> +	netif_device_detach(dev);
> +
> +	/* Only the buffers and the MTU registers change, leave the PHY up */
> +	bcmgenet_netif_stop(dev, false);
> +	priv->datapath_up = false;
> +
> +	WRITE_ONCE(dev->mtu, new_mtu);
> +	priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
> +	bcmgenet_set_mtu_regs(priv, new_mtu);

[Severity: Medium]
Can the first link-up enable the MAC again in the middle of this
reconfiguration?

After ifup and before the first link-up, UMAC_CMD still has CMD_SW_RESET
set from init_umac()->reset_umac(). In that state umac_enable_set() returns
without writing anything:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:umac_enable_set() {
    ...
	if (reg & CMD_SW_RESET) {
		spin_unlock_bh(&priv->reg_lock);
		return;
	}
    ...
}

So the MAC disable inside bcmgenet_netif_stop() has no effect.

The PHY state machine keeps running here. A link-up in this window runs
bcmgenet_mii_setup()->bcmgenet_mac_config() under phydev->lock, not RTNL:

drivers/net/ethernet/broadcom/genet/bcmmii.c:bcmgenet_mac_config() {
    ...
	if (reg & CMD_SW_RESET) {
		reg &= ~CMD_SW_RESET;
		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
		udelay(2);
		reg |= CMD_TX_EN | CMD_RX_EN;
	}
    ...
}

The MAC would then be enabled while bcmgenet_set_mtu_regs() rewrites
UMAC_MAX_FRAME_LEN and the RBUF/TBUF thresholds. That function's own
comment says "Call with the MAC disabled". It would also be enabled while
bcmgenet_init_dma(priv, true) flushes the RBUF and rebuilds the rings.

Open and resume program these registers before phy_start(), so only the
new path is exposed. The window includes the 10-20 ms sleep in the DMA
teardown, so a network manager that sets the MTU right after ifup could
hit it. Does anything prevent this?

> +
> +	ret = bcmgenet_init_dma(priv, true);

[ ... ]

> +	bcmgenet_hfb_restore(priv);
> +	bcmgenet_netif_start(dev, false);
> +
> +	/* bcmgenet_netif_start() only restores the link interrupt */
> +	if (bcmgenet_has_mdio_intr(priv))
> +		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_MDIO_EVENT,
> +					 INTRL2_CPU_MASK_CLEAR);
> +
> +	/* A link event latched while the interrupts were off is gone. Internal
> +	 * PHYs on GENET v1-v4 are not polled, so resync the state machine.
> +	 */
> +	phy_mac_interrupt(dev->phydev);
> +
> +	priv->datapath_up = true;
> +	netif_device_attach(dev);

[Severity: Medium]
Can a concurrent RX address list update get lost here?

bcmgenet_netif_start() programs the RX filters through
bcmgenet_set_rx_mode() under netif_addr_lock_bh() and then drops the lock.
The device stays detached until the netif_device_attach() above. A unicast
or multicast list change in that window reaches __dev_set_rx_mode(), which
drops it:

net/core/dev_addr_lists.c:__dev_set_rx_mode() {
    ...
	if (!netif_device_present(dev))
		return;
    ...
}

netif_device_attach() doesn't resync the RX mode, and bcmgenet has no
ndo_change_rx_flags, so nothing queues the update for later.

RTNL is held here. However, IPV6_ADD_MEMBERSHIP
(ipv6_sock_mc_join()->__ipv6_dev_mc_inc()->dev_mc_add()) runs under the
socket lock and mc_lock, not RTNL, so it can race with this. The new group
would then sit in dev->mc without reaching the MDF filter until some
unrelated RX mode change.

Would it help to call bcmgenet_set_rx_mode() again after
netif_device_attach()?

[ ... ]

> @@ -4510,6 +4602,7 @@ static int bcmgenet_suspend(struct device *d)
>  	} else {
>  		/* Teardown the interface */
>  		bcmgenet_netif_stop(dev, true);
> +		priv->datapath_up = false;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the new priv->datapath_up
flag tracks exactly this state, and bcmgenet_suspend() still never checks
it. Can a second system suspend hang after a failed resume?

The sequence would be:

1. A non-WoL suspend runs bcmgenet_netif_stop(dev, true). Its
   napi_disable() calls leave NAPI_STATE_SCHED|NPSVC set.
   bcmgenet_fini_dma() then deletes the NAPIs and frees rx_cbs/tx_cbs
   without NULLing them.

2. bcmgenet_resume() fails in bcmgenet_init_dma() (kzalloc failure, Tx/Rx
   DMA halt timeout, or Rx buffer allocation failure):

	ret = bcmgenet_init_dma(priv, false);
	if (ret) {
		netdev_err(dev, "failed to initialize DMA\n");
		goto out_clk_disable;
	}

   The device is left detached, netif_running() is still true and
   datapath_up is false.

3. On the next system suspend, bcmgenet_suspend() sees netif_running() and
   reaches bcmgenet_disable_tx_napi(). In this branch that happens through
   bcmgenet_netif_stop(), and the WoL branch calls it directly.

napi_disable_locked() would then loop forever in:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

That happens because the earlier disable left those bits set. Even if
suspend got past that point, bcmgenet_fini_dma() would kfree() rx_cbs and
tx_cbs a second time.

bcmgenet_close() is now guarded by priv->datapath_up. Should the teardown
in both branches of bcmgenet_suspend() check it too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-nb-genet-mtu-nn-v2-v4-0-2e2e3bfaf8cc%40tipi-net.de