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

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.