Re: [PATCH net-next 5/7] 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: 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] In this patch, a single RX descriptor is sized for
  GENET_RBUF_ALIGN + MTU + ETH_HLEN + VLAN_HLEN; this applies to…
- [Medium] bcmgenet_change_mtu() leaves the PHY state machine running but
  reuses bcmgenet_netif_stop(dev, false).
- [Low] The commit message does not describe all of the behaviour this
  patch changes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  datapath_up flag does not cover it.

--

net: bcmgenet: allow the MTU to be changed

This commit sets dev->max_mtu to the largest MTU that fits in one RX
descriptor and adds an ndo_change_mtu handler. On a running interface the
handler resizes the rings and rewrites the MTU registers while leaving the
PHY up. If the allocation fails, it retries at the old size and closes the
interface when that also fails.

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

> The driver never sets dev->max_mtu, so the MTU is stuck at ETH_DATA_LEN.

[Severity: Low]
Is this accurate? ether_setup() leaves min_mtu at 68. Before this patch
there was no ndo_change_mtu, so userspace could already lower the MTU.
That changed only dev->mtu and never touched the hardware.

With this patch, lowering the MTU has effects the commit message does not
mention.

bcmgenet_set_mtu_regs() now writes ENET_MAX_FRAME_LEN(mtu) to
UMAC_MAX_FRAME_LEN. The MAC receive limit therefore drops below the old
fixed 1536 bytes, for example to 1436 for an MTU of 1400. This also
happens on v1, because the write comes before the GENET_IS_V1() return.

On a running interface, bcmgenet_change_mtu() always runs the full
netif_device_detach() -> bcmgenet_netif_stop() -> bcmgenet_init_dma() ->
bcmgenet_netif_start() sequence. It does this even when
bcmgenet_rx_buf_len() returns the same size. That is the case for every
MTU up to about 2028, because bcmgenet_pkt_rdy_thld() clamps to
ENET_THLD_DEFAULT.

So a plain decrease interrupts traffic. It can also end in netif_close()
if both init_dma attempts fail.

GENET v1 gets the same ndo_change_mtu and the same rebuild on a decrease.
The new probe comment "v1 cannot program the thresholds, so it stays at
the default MTU" only holds for the maximum.

Could the commit message and the probe comment describe this? Could the
rebuild be skipped when rx_buf_len does not change?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bf889558f6ade..4634b6521ab8e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -77,6 +77,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: Medium]
Does one descriptor really hold the largest frame the MAC accepts at this
MTU?

bcmgenet_pkt_rdy_thld() and bcmgenet_rx_buf_len() size a descriptor for
GENET_RBUF_ALIGN + mtu + ETH_HLEN + VLAN_HLEN. However,
bcmgenet_set_mtu_regs() programs UMAC_MAX_FRAME_LEN to
mtu + ENET_FRAME_OVERHEAD (mtu + 36). That also counts the FCS, the
Broadcom tag and the padding.

With 4K pages, ENET_MAX_MTU is 3564 and the threshold is exactly 3584, so
there is no slack. Two kinds of frame are accepted by the MAC but end up
spanning two descriptors:

- a QinQ frame at full MTU (2 + 3564 + 22 = 3588 bytes)
- a VLAN tagged frame with a forwarded FCS (crc_fwd_en)

bcmgenet_desc_rx() then drops them:

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

The same happens just below each 256 byte threshold step, for example at
MTU 2025-2028.

Later in the series, "net: bcmgenet: reassemble jumbo frames from status
block fragments" seems to fix this. It removes ENET_MAX_MTU, uses
ENET_MAX_JUMBO_MTU and reassembles frames that span several descriptors.

As of this patch, though, are these frames silently dropped and counted
as fragmented_errors?

> +
>  /* 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))

[ ... ]

> @@ -3549,7 +3556,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 the new flag doesn't cover
it. datapath_up is only updated in open, close and change_mtu. The
suspend and resume paths never touch it.

A non-WoL bcmgenet_suspend() calls bcmgenet_netif_stop(dev, true). That
disables every TX and RX NAPI and then runs bcmgenet_fini_dma(), which
calls netif_napi_del() and frees priv->rx_cbs and priv->tx_cbs without
setting them to NULL. datapath_up stays set.

The WoL branch of resume that finds the MAC was reset also calls
bcmgenet_fini_dma() directly.

bcmgenet_resume() can then fail in bcmgenet_init_dma(), from a DMA halt
timeout or an allocation failure:

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

In that case it returns with the clock off, netif_running() still true
and datapath_up still set.

A later bcmgenet_close() takes the new branch and stops the datapath a
second time:

bcmgenet_close()
  bcmgenet_netif_stop()
    bcmgenet_disable_tx_napi()
      napi_disable()
        napi_disable_locked()
          while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC))
                  usleep_range(20, 200);

Can this loop forever while holding RTNL and the netdev instance lock? If
it ever got past the loop, bcmgenet_fini_dma() would free the stale
rx_cbs and tx_cbs a second time.

With this patch, bcmgenet_change_mtu() on such a device reaches the same
loop through its own bcmgenet_netif_stop() call.

Should suspend and the resume error paths also keep datapath_up up to
date?

>  
>  	/* Really kill the PHY state machine and disconnect from it */
>  	phy_disconnect(dev->phydev);

[ ... ]

> @@ -3797,6 +3808,66 @@ 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)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +	unsigned int old_mtu = dev->mtu;
> +	int ret;
> +
> +	if (!netif_running(dev)) {
> +		WRITE_ONCE(dev->mtu, new_mtu);
> +		priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
> +		return 0;
> +	}
> +
> +	/* 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]
The commit message says "The PHY keeps running and the link stays up".
Can a link event be lost here, though?

bcmgenet_netif_stop() calls bcmgenet_intr_disable(), which does:

	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_MASK_SET);
	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_CLEAR);

It then calls cancel_work_sync(&priv->bcmgenet_irq_work).

A UMAC_IRQ_LINK_EVENT or UMAC_IRQ_PHY_DET_R that has latched but not been
handled can be lost in two ways:

- the CLEAR wipes it before bcmgenet_isr0() reads it
- bcmgenet_isr0() has already saved it in priv->irq0_stat, but the work
  is cancelled before bcmgenet_irq_task() runs

For internal PHYs on GENET v1-v4, bcmgenet_mii_probe() does:

	dev->phydev->irq = PHY_MAC_INTERRUPT;

So phylib never polls. The only notification path is
bcmgenet_irq_task() -> phy_mac_interrupt().

Nothing on the restart path re-triggers the PHY state machine. That path
is bcmgenet_netif_start(dev, false) plus re-enabling MDIO_EVENT.

Would phylib's link state and the carrier then stay stale until the next
link event? One example is carrier still up after the cable is pulled.

A lost PHY_DET_R on v1-v3 with forced speed would also skip the
phy_init_hw() and genphy_config_aneg() calls in bcmgenet_irq_task().

Before this patch, bcmgenet_netif_stop() was only used when the PHY was
also being stopped or disconnected. Is a phy_mac_interrupt() or
phy_trigger_machine() call needed after the restart?

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

[ ... ]

> +	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);
> +
> +	priv->datapath_up = true;
> +	netif_device_attach(dev);
> +
> +	return ret;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de