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

[Nicolai Buchwitz <nb@tipi-net.de>]

On 14.10.2026 16:22, netdev-bot+sashiko@kernel.org wrote:

> [...]

> [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?

In theory. bcmgenet_close() and suspend walk the same path today and I
have never seen the disable time out on a BCM2711, so I would rather not
rework the teardown on something I cannot trigger.

> [...]

> [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?

It would, but IMHO suspend and resume drop the setting the same way 
today,
so should be fixed in net rather than here.

> [...]

> [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?

Yes, still pre-existing as you say. It goes into the same net series as
the dangling rx_cbs/tx_cbs which make the second teardown a double free.
I will check the clock counts there too.