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

[netdev-bot+sashiko@kernel.org]

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

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

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] On a running interface, bcmgenet_change_mtu() silently resets
  the user's TX interrupt coalescing (ethtool -C tx-frames) to 1.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is High
  severity.
- [Medium] This problem wasn't introduced by this patch.

--

net: bcmgenet: allow the MTU to be changed

This commit sets dev->max_mtu for GENET v2+ from the packet ready threshold
and the page size. It also adds ndo_change_mtu, which resizes the buffers
and rewrites the MTU registers in place while the PHY stays up. A new
datapath_up flag tracks whether the rings exist, and a failed allocation
retries at the old MTU and closes the interface if that also fails.

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..9f28006957d4d 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -3870,6 +3885,91 @@ 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)
> +{

[ ... ]

> +	/* Only the buffers and the MTU registers change, leave the PHY up.
> +	 * Hold its lock meanwhile, a link event would otherwise take the MAC
> +	 * out of reset and enable it mid rewrite.
> +	 */
> +	mutex_lock(&dev->phydev->lock);
> +	bcmgenet_netif_stop(dev, false);

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce, but this
patch adds a new caller that reaches it.

bcmgenet_netif_stop() drops the return value of bcmgenet_dma_teardown()
and then frees the RX pages anyway:

bcmgenet_netif_stop()
    bcmgenet_dma_teardown(priv);   /* -ETIMEDOUT ignored */
    ...
    bcmgenet_fini_dma(priv)
        bcmgenet_free_rx_buffers(priv);
        bcmgenet_destroy_rx_page_pools(priv);

Suppose bcmgenet_rdma_disable() times out because DMA_STATUS never
acknowledges the disable. Could the RDMA engine then keep writing into
pages that have already gone back to the page pool?

The later bcmgenet_init_dma() failure and netif_close() in
bcmgenet_change_mtu() would only notice the stuck DMA after the pages are
already freed.

bcmgenet_close() reaches the same path today. Nothing in the tree shows
RDMA actually failing to halt, so this may only be theoretical.

> +	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]
Does this restart lose the user's TX interrupt coalescing setting?

bcmgenet_set_coalesce() writes ec->tx_max_coalesced_frames straight into
the DMA_MBUF_DONE_THRESH register of each TDMA ring and keeps no software
copy. bcmgenet_get_coalesce() reads the value back from ring 0.

This path rebuilds the TX rings with a hard-coded threshold:

bcmgenet_init_dma()
    bcmgenet_init_tx_queues()
        bcmgenet_init_tx_ring()
            bcmgenet_tdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);

bcmgenet_init_rx_coalesce() reapplies the RX coalescing, but nothing
restores the TX value.

Take "ethtool -C ethX tx-frames N" followed by an MTU change across
ETH_DATA_LEN, such as 1500 to 3000. Would ethtool -c then report
tx-frames 1?

> +	if (ret) {

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but datapath_up now tracks
exactly this state. Should bcmgenet_suspend() check it as well?

Suppose bcmgenet_init_dma() fails in bcmgenet_resume(). The interface
then stays netif_running() and detached:

- the earlier bcmgenet_fini_dma() has already deleted the NAPIs
- rx_cbs and tx_cbs are freed but not set to NULL
- the clock is gated

bcmgenet_resume()
	ret = bcmgenet_init_dma(priv, false);
	if (ret) {
		netdev_err(dev, "failed to initialize DMA\n");
		goto out_clk_disable;
	}
	...
out_clk_disable:
	if (priv->internal_phy)
		bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
	clk_disable_unprepare(priv->clk);

On the next system suspend, bcmgenet_suspend() checks only
netif_running(dev). Both of its branches reach bcmgenet_disable_tx_napi():
the WoL branch directly, the other branch through bcmgenet_netif_stop().

The TX NAPIs were deleted with SCHED and NPSVC still set. Would
napi_disable_locked() then wait here forever?

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

If suspend ever got past that point, bcmgenet_tx_reclaim_all() would walk
the freed tx_cbs. bcmgenet_fini_dma() would also kfree() rx_cbs and tx_cbs
a second time, with the block unclocked.

bcmgenet_change_mtu() already handles this state ("A datapath left down by
a failed resume has nothing to reprogram either"). bcmgenet_suspend() only
gets the assignment, not an early return.

If the interface is closed after the failed resume, the new guard skips
bcmgenet_netif_stop(). bcmgenet_close() still calls bcmgenet_power_down()
and clk_disable_unprepare(priv->clk) unconditionally. The failed resume
already balanced the clock, so does this underflow the clock's enable and
prepare counts?

Could bcmgenet_suspend() skip the teardown when !priv->datapath_up? Could
the driver also track that the failed resume already gated the clock, so
close and suspend_noirq do not disable it again?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-nb-genet-mtu-nn-v2-v5-0-076b6fc240be%40tipi-net.de