Re: [PATCH net-next v4 1/3] net: stmmac: enable MAC rx/tx after DMA start

[Abid Ali <dev.taqnialabs@gmail.com>]

On Tue, 2026-08-18 at 15:57 +0200, Paolo Abeni wrote:
> [Severity: Low]
> Could a Fixes: tag be added for each of the two sequences being corrected,
> one for the ordering in stmmac_hw_setup() and one for the same ordering in
> stmmac_xdp_open() which arrived with the AF_XDP path?
>
> Without a Fixes: trailer, how do stable/backport tools determine the
> affected release range, and is this intended for net or net-next?

Yes, I missed the trailers. These are supposed to be fixes.

Fixes: 47dd7a540b8a ("net: add support for STMicroelectronics Ethernet controllers.")
Fixes: ac746c8520d9 ("net: stmmac: enhance XDP ZC driver level switching performance")

The interface stall can happen on any platform if inflow happens between
stmmac_mac_set() and stmmac_start_all_dma(). I added an intentional delay
between these two events to test it, and that made the interface hang
100% reproducible.

I will resend as [PATCH net] with both trailers in the next version.

> [Severity: Medium]
> Does this reordering actually close the window on the dwmac4/dwmac5
> (GMAC4+) cores?
>
> There the MAC receiver and transmitter enables are set from inside the
> per-channel DMA start callbacks:
>
> drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c:dwmac4_dma_start_rx() {
> 	value |= DMA_CONTROL_SR;
> 	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
>
> 	value = readl(ioaddr + GMAC_CONFIG);
> 	value |= GMAC_CONFIG_RE;
> 	writel(value, ioaddr + GMAC_CONFIG);
> }
>
> and dwmac4_dma_start_tx() does the same for GMAC_CONFIG_TE.
>
> stmmac_start_all_dma() starts the RX channels one at a time:
>
> 	for (chan = 0; chan < rx_channels_count; chan++)
> 		stmmac_start_rx_dma(priv, chan);
>
> so the global MAC receiver is already enabled by the first iteration while
> channels 1..n-1 still have SR == 0. On those cores, isn't the relocated
> stmmac_mac_set(priv, priv->ioaddr, true) redundant, and doesn't the
> "MAC classifies frames into an MTL queue whose DMA is not running"
> condition still exist whenever rx_queues_to_use > 1?
>
> The RE-in-DMA-op coupling itself isn't introduced by this patch, but should
> the commit message claim "The EMAC should only accept packets after the DMAs
> are enabled" be qualified, or should the dwmac4 coupling be decoupled too?

Not completely on dwmac4/dwmac5.
dwmac4_dma_start_rx() raises the global GMAC_CONFIG.RE itself, so RE is
up after the chan 0 iteration of stmmac_start_all_dma() while the rest of
the channels still have SR (the start bit) clear. That is still a timing
gap where the MTL queue can fill up.

Mainly, RE is no longer set before stmmac_start_all_dma() is entered at
all. That was the long window, where stmmac_dma_operation_mode(),
stmmac_mmc_setup(), stmmac_set_rings_length() and the rest of the setup
all ran with the receiver already live.

On XGMAC even that is gone once 3/3 ("net: stmmac: xgmac: decouple the
MAC Rx/Tx enables from the DMA ops") is added, since it takes the RE/TE
writes out of dwxgmac2_dma_start_rx()/dwxgmac2_dma_start_tx() entirely.

Ideally the same should be done for dwmac4/dwmac5, but I left that out
as that was not the hardware with me. Seems like should follow the same
and will do that in the next version.

> [Severity: Low]
> This isn't a bug introduced by this patch, but the mirror-image teardown
> ordering used to be the reverse of what this patch establishes, i.e.
> stopping the DMA before disabling the MAC Rx in __stmmac_release(),
> stmmac_xdp_release() and stmmac_suspend(). The follow-on commit "net:
> stmmac: xgmac: tear the datapath down in the documented order" introduces
> stmmac_datapath_teardown() and converts those callers, so the MAC is
> disabled before stmmac_stop_rx_dma() runs:
>
> 	/* Disable the MAC TX and RX */
> 	stmmac_mac_set(priv, priv->ioaddr, false);
>
> 	/* Wait for the MTL RX queues to drain into the RX DMA */
> 	...
> 	for (chan = 0; chan < rx_channels_count; chan++)
> 		stmmac_stop_rx_dma(priv, chan);
>
> Noting it here only for completeness, since the remaining "MAC Rx on, DMA
> off" case is the WoL branch of stmmac_suspend(), which that commit
> documents as intentional.

Right, that is 2/3 ("net: stmmac: xgmac: tear the datapath down in the
documented order") of this series. The teardown is not the exact inverse
of the bring-up; the databook gives its own order for it.

Also, the WoL branch in stmmac_suspend() is left on stmmac_stop_all_dma()
on purpose. That path only stops the DMA channels and never calls
stmmac_mac_set(priv, priv->ioaddr, false), because the receiver has to
stay enabled for the wake frame to be seen. 

> [Severity: High]
> This isn't a bug introduced by this patch, since the DMA was already started
> here before the reorder, but the MAC enable now sits inside a region whose
> error path never unwinds either the DMA or the MAC.
>
> If stmmac_request_irq() fails, the unwind in stmmac_xdp_open() is:
>
> irq_error:
> 	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> 		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
>
> init_error:
> 	free_dma_desc_resources(priv, &priv->dma_conf);
>
> There is no stmmac_stop_all_dma() and no stmmac_mac_set(priv, priv->ioaddr,
> false), while free_dma_desc_resources() reaches:
>
> __free_dma_rx_desc_resources() {
> 	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
> 	...
> 	if (rx_q->page_pool)
> 		page_pool_destroy(rx_q->page_pool);
> }
>
> Can the RX DMA then write received frames into the freed coherent
> descriptor ring and the freed page-pool pages? stmmac_xdp_set_prog() also
> discards the stmmac_xdp_open() return value:
>
> 	if (if_running && need_update)
> 		stmmac_xdp_open(dev);
> 	...
> 	return 0;
>
> so does the device stay in that state indefinitely rather than being
> reported and cleaned up?
>
> The same gap looks present on the ifup path: __stmmac_open()'s irq_error
> label only does phylink_stop(), the per-queue hrtimer_cancel() and
> stmmac_release_ptp(), and stmmac_open() then falls through to
> err_dma_resources: free_dma_desc_resources(priv, dma_conf), which frees the
> rings that were programmed into the live DMA engine.
>
> Should these error paths be made symmetric with the release path, which
> does:
>
> 	/* Stop the MAC and the TX/RX DMA channels */
> 	stmmac_datapath_teardown(priv);
>
> 	/* Release and free the Rx/Tx resources */
> 	free_dma_desc_resources(priv, &priv->dma_conf);

Yes, it can. The rings are freed while the Rx DMA is still running, SR is
set and nothing on that path stops it, so the engine can write into freed
memory. stmmac_xdp_set_prog() then drops the return value, so the device
is left that way. The similar gap exists in the ifup path too.

It is a real issue, but it existed before the reorder too. Both the MAC
and the DMA were already live at irq_error:, so this could happen even
before the reordering. I do understand the concern, but the AI review
does seem to flag everything it finds wrong in the surrounding code even
when the patch did not introduce it.

I could look into a fix for this, but cleaning up all the surrounding
code flagged by sashiko seems like an endless loop.

> [Severity: Medium]
> This is a pre-existing race rather than something the patch adds, but it
> bears on the invariant the patch is establishing: the ordering here is only
> statement order inside stmmac_xdp_open(), and nothing serializes it against
> phylink's resolve worker.
>
> phylink_resolve() runs from a work item holding only pl->phydev_mutex,
> phy->lock and pl->state_mutex, with no rtnl:
>
> drivers/net/phy/phylink.c:phylink_resolve() {
> 	mutex_lock(&pl->phydev_mutex);
> 	phy = pl->phydev;
> 	if (phy)
> 		mutex_lock(&phy->lock);
> 	mutex_lock(&pl->state_mutex);
> 	...
> 		else
> 			phylink_link_up(pl, link_state);
> }
>
> and that reaches stmmac_mac_link_up() -> stmmac_mac_set(priv, priv->ioaddr,
> true). The AF_XDP reconfiguration never quiesces phylink:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
> 	if (if_running && need_update)
> 		stmmac_xdp_release(dev);
> 	...
> 	if (if_running && need_update)
> 		stmmac_xdp_open(dev);
> }
>
> So can a link-up resolved on another CPU re-enable MAC Rx after
> stmmac_xdp_release() has stopped the DMA and before stmmac_xdp_open()
> reaches stmmac_start_all_dma(), which is the same "MAC accepting frames with
> no DMA to drain the MTL FIFO" condition described in the commit message?
>
> Separately, stmmac_mac_link_up() and stmmac_mac_set() both do unlocked
> read-modify-write on MAC_CTRL_REG:
>
> 	old_ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
> 	ctrl = old_ctrl & ~priv->hw->link.speed_mask;
> 	...
> 	if (ctrl != old_ctrl)
> 		writel(ctrl, priv->ioaddr + MAC_CTRL_REG);
> 	...
> 	stmmac_mac_set(priv, priv->ioaddr, true);
>
> Can concurrent speed/duplex and RE/TE updates lose each other's bits?

They also look valid, but they are not related to or introduced by this change.
Again, I am not able to see how either of these follows from the reorder proposed here.

In this 3-patch series, the primary fix I intended was for the interface
hang, and based on the initial reviews it made sense to fix the teardown
path too even though teardown was left untouched.

I also took a look at the comments on 2/3 ("net: stmmac: xgmac: tear the
datapath down in the documented order"), and some of them seem valid, but
addressing those corner cases does not look straightforward and will 
definitely need some time.

Would it make sense to send 1/3 ("net: stmmac: enable MAC rx/tx after DMA
start") and 3/3 ("net: stmmac: xgmac: decouple the MAC Rx/Tx enables from
the DMA ops") as the [PATCH net] fixes for the bootup interface hang, once the
review comments on them are addressed, and leave out 2/3 ("net: stmmac:
xgmac: tear the datapath down in the documented order") for now, since
the teardown path was never intended to be the fixed here?

Would like your feedback on how to proceed.


Best Regards,

Abid