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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 (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] bcmgenet_change_mtu() leaves the PHY state machine running,
  but it needs the MAC disabled while it reprograms the MAC.
- [Low] At this commit, ENET_MAX_MTU (bcmgenet.c:90) and
  bcmgenet_pkt_rdy_thld() (bcmgenet.c:2688) size one RX descriptor for 2…
- [Low] bcmgenet_change_mtu() leaves the PHY state machine running, but
  bcmgenet_netif_stop() -> bcmgenet_intr_disable() (line 3596) writes…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  priv->datapath_up flag does not cover it.
- [Medium] This problem wasn't introduced by this patch, but
  bcmgenet_netif_stop() ignores the -ETIMEDOUT result of…

--

net: bcmgenet: allow the MTU to be changed

On GENET v2 and later, this commit sets dev->max_mtu from the RX packet
ready threshold. It also adds an ndo_change_mtu handler that tears down and
rebuilds the DMA rings in place while the PHY keeps running. A new
datapath_up flag lets bcmgenet_close() skip the teardown when a failed MTU
change has already taken the datapath 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 17c8318c20388..6d21ac2d78d51 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -84,6 +84,12 @@
>  				      ENET_THLD_MAX * ENET_THLD_UNIT, \
>  				      ENET_THLD_PAGE_LEN)
>  
> +/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
> + * interface can use the parent MTU.
> + */
> +#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
> +				 ETH_HLEN - VLAN_HLEN)

[Severity: Low]
At this commit, ENET_MAX_MTU and bcmgenet_pkt_rdy_thld() both size one RX
descriptor as GENET_RBUF_ALIGN + mtu + ETH_HLEN + VLAN_HLEN. Meanwhile,
bcmgenet_set_mtu_regs() programs the MAC limit with the larger
ENET_FRAME_OVERHEAD:

    bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);

If the MAC accepts a frame that is bigger than one descriptor, for example
QinQ at the parent MTU, that frame spans two descriptors. Would it then be
dropped in bcmgenet_desc_rx()?

    if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
        netif_err(priv, rx_status, dev,
                  "dropping fragmented packet!\n");

The forwarded FCS case doesn't look reachable here. reset_umac() in open
clears CMD_CRC_FWD, and bcmgenet_set_features() then reads crc_fwd_en back as
0. So only frames longer than the MTU plus one VLAN tag seem to be affected.

The later patch in this series, "net: bcmgenet: reassemble jumbo frames from
status block fragments", removes ENET_MAX_MTU and replaces the drop with
bcmgenet_add_frag() reassembly. That means this only applies to the
intermediate state at this commit.

> +
>  /* Tx/Rx DMA register offset, skip 256 descriptors */
>  #define WORDS_PER_BD(p)		(p->hw_params->words_per_bd)
>  #define DMA_DESC_SIZE		(WORDS_PER_BD(priv) * sizeof(u32))

[ ... ]

> @@ -3605,7 +3612,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;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does datapath_up cover the
suspend/resume path?

Only bcmgenet_open() and bcmgenet_change_mtu() update the flag. The non-WoL
branch of bcmgenet_suspend() calls bcmgenet_netif_stop(dev, true). That
disables the NAPIs and frees rx_cbs/tx_cbs in bcmgenet_fini_dma() without
NULLing the pointers, and it leaves datapath_up set.

If bcmgenet_resume() then fails in bcmgenet_init_dma():

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

then the interface is still running, but it has no rings and datapath_up is
still 1. A later bcmgenet_close() from ifdown, unregister or shutdown would
call bcmgenet_netif_stop() a second time.

The NAPIs are already disabled at that point. Can napi_disable() in
bcmgenet_disable_tx_napi() then spin forever with RTNL held? If it did
return, bcmgenet_fini_dma() would kfree() rx_cbs and tx_cbs a second time.

A failed bcmgenet_init_dma() on resume looks realistic. The earlier patch in
this series, "net: bcmgenet: pad transmit frames out of the packet ready
window", describes TX DMA refusing to halt so that every later
bcmgenet_init_dma() fails. The RX buffers also come from
page_pool_dev_alloc(), which doesn't sleep.

bcmgenet_change_mtu() adds another way to hit this, because it calls
bcmgenet_netif_stop() without checking datapath_up.

Would clearing datapath_up in the suspend teardown, and setting it again only
after a successful resume, close this gap?

>  
>  	/* Really kill the PHY state machine and disconnect from it */
>  	phy_disconnect(dev->phydev);
> @@ -3853,6 +3864,71 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier)
>  	return 0;
>  }
>  
> +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);

[Severity: Medium]
This is a pre-existing issue, but bcmgenet_netif_stop() ignores the
-ETIMEDOUT result of bcmgenet_dma_teardown():

    bcmgenet_dma_teardown(priv);

    /* Disable MAC transmit. TX DMA disabled must be done before this */
    umac_enable_set(priv, CMD_TX_EN, false);

If RDMA or TDMA doesn't halt in time, bcmgenet_fini_dma() still returns the
RX pages to the page pool and destroys the pools. Hardware descriptors may
still point at those pages. Could the device then DMA into freed pages, or
read freed skbs on TX?

MAC RX is disabled before the teardown, so this needs a real halt timeout to
trigger. This new caller lets an admin run the teardown repeatedly with ip
link set mtu.

> +	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);
> +
> +	ret = bcmgenet_init_dma(priv, true);

[Severity: Medium]
Is the MAC actually disabled at this point if the interface was opened
without a link?

After an open with no link, reset_umac() leaves CMD_SW_RESET set.
umac_enable_set() returns early while that bit is set:

    if (reg & CMD_SW_RESET) {
        spin_unlock_bh(&priv->reg_lock);
        return;
    }

So the CMD_RX_EN/CMD_TX_EN disable in bcmgenet_netif_stop() does nothing.
The PHY state machine keeps running and doesn't take RTNL. If the first
link-up arrives during the teardown and rebuild,
bcmgenet_mii_setup()->bcmgenet_mac_config() does:

    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;
    }

bcmgenet_set_mtu_regs() is documented as "Call with the MAC disabled".
Wouldn't it, along with the RBUF flush in bcmgenet_init_dma(), then run while
the MAC is receiving?

Nothing seems to serialize the adjust_link callback against this window.
reg_lock only covers single read-modify-write sequences.

> +	if (ret) {

[ ... ]

> +	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);

[Severity: Low]
The PHY keeps running during the MTU change, but
bcmgenet_netif_stop()->bcmgenet_intr_disable() also masks
UMAC_IRQ_MDIO_EVENT. It is only unmasked here, after the whole teardown and
rebuild.

bcmgenet_mii_wait() depends on that interrupt to wake priv->wq:

    wait_event_timeout(priv->wq,
                       !(bcmgenet_umac_readl(priv, UMAC_MDIO_CMD)
                       & MDIO_START_BUSY),
                       HZ / 100);

Would every phylib MDIO access in this window wait the full 10 ms timeout
while holding phydev->lock? bcmgenet_mii_wait() always returns 0, so the
effect looks like added latency rather than a failure.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-nb-genet-mtu-nn-v2-v3-0-74a796c019ce%40tipi-net.de