Re: [PATCH 1/1] net: stmmac: rebuild RX descriptors before restarting DMA on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] net: stmmac: stmmac_resume_descriptors() fails to restore chained des3 links
- [High] net: stmmac: stmmac_resume_rx_descriptors() fails on empty or partial XSK pool
- [Medium] net: stmmac: stmmac_init_rx_buffers() ignores flags and uses GFP_ATOMIC
--

commit 8d3c0459b1ad22247b62f5f4182ad5e15b2eab5d
Author: Thomas Bourgoin <thomas.bourgoin@foss.st.com>

net: stmmac: rebuild RX descriptors before restarting DMA on resume

This commit reconstructs RX descriptors and restores their DMA buffer
addresses during system resume before clearing descriptors and restarting
DMA, preventing the DMA engine from treating write-back status fields as
buffer addresses.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9ded31150fbe6..198629265c904 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -8425,6 +8414,63 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
>  	rx_q->dirty_rx = 0;
>  }
>  
> +static int stmmac_resume_rx_descriptors(struct stmmac_priv *priv,
> +					struct stmmac_dma_conf *dma_conf,
> +					u32 queue)
> +{
> +	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < dma_conf->dma_rx_size; i++) {
> +		struct dma_desc *p = stmmac_get_rx_desc(priv, rx_q, i);
> +		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
> +		dma_addr_t dma_addr;
> +
> +		if (rx_q->xsk_pool) {
> +			if (!buf->xdp) {
> +				buf->xdp = xsk_buff_alloc(rx_q->xsk_pool);
> +				if (!buf->xdp)
> +					return -ENOMEM;
> +				}
> +			dma_addr = xsk_buff_xdp_get_dma(buf->xdp);
> +			stmmac_set_desc_addr(priv, p, dma_addr);
> +			stmmac_set_desc_sec_addr(priv, p, 0, false);

[Severity: High]
Can this break resume when an AF_XDP zero-copy pool is attached with an
empty or partially populated fill ring?

In __init_dma_rx_desc_rings(), an unpopulated XSK pool (such as TX-only
xdpsock) is explicitly allowed:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__init_dma_rx_desc_rings() {
	if (rx_q->xsk_pool) {
		/* RX XDP ZC buffer pool may not be populated, e.g.
		 * xdpsock TX-only.
		 */
		stmmac_alloc_rx_buffers_zc(priv, dma_conf, queue);
	}
}

Similarly, if RX frames were consumed in stmmac_rx_zc() before suspend
(setting buf->xdp = NULL) and the frozen userspace fill ring has fewer than
dma_conf->dma_rx_size buffers available, xsk_buff_alloc() returns NULL.

Returning -ENOMEM here causes stmmac_resume() to abort at error_unlock,
skipping stmmac_hw_setup(), stmmac_enable_all_queues(), and
netif_device_attach() while netif_running(ndev) remains true. Because
stmmac_suspend() already called stmmac_disable_all_queues(), leaving NAPI
disabled on a running interface causes a subsequent stmmac_xdp_disable_pool()
or stmmac_release() to call napi_disable() a second time and hang under
rtnl_lock().

Additionally, stmmac_resume_rx_descriptors() does not update
rx_q->buf_alloc_num, which stmmac_hw_setup() -> stmmac_init_dma_engine()
uses to program the hardware RX tail pointer and
stmmac_set_queue_rx_buf_size() uses to select
xsk_pool_get_rx_frame_size(rx_q->xsk_pool).

> +		} else {
> +			ret = stmmac_init_rx_buffers(priv, dma_conf, p, i,
> +						     GFP_KERNEL, queue);
> +			if (ret)
> +				return ret;
> +		}
> +	}

[Severity: Medium]
Here GFP_KERNEL is passed as the flags argument to stmmac_init_rx_buffers(),
but does stmmac_init_rx_buffers() actually honour the caller's flags?

Looking at stmmac_init_rx_buffers() in
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:

static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
				  struct stmmac_dma_conf *dma_conf,
				  struct dma_desc *p,
				  int i, gfp_t flags, u32 queue)
{
	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
	struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
	gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);

It ignores the flags parameter and forces GFP_ATOMIC | __GFP_NOWARN for both
buf->page and buf->sec_page allocations, preventing direct reclaim during
resume and risking avoidable -ENOMEM failures under memory pressure.

> +
> +	return 0;
> +}
> +
> +static int stmmac_resume_descriptors(struct stmmac_priv *priv,
> +				     struct stmmac_dma_conf *dma_conf)
> +{
> +	u32 rx_queue_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queue_cnt = priv->plat->tx_queues_to_use;
> +	u32 queue;
> +	int ret;
> +
> +	for (queue = 0; queue < rx_queue_cnt; queue++) {
> +		ret = stmmac_resume_rx_descriptors(priv, dma_conf, queue);
> +		if (ret)
> +			return ret;
> +
> +		stmmac_clear_rx_descriptors(priv, dma_conf, queue);
> +	}
> +
> +	for (queue = 0; queue < tx_queue_cnt; queue++) {
> +		dma_free_tx_skbufs(priv, &priv->dma_conf, queue);
> +		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
> +	}
> +
> +	return 0;
> +}

[Severity: Critical]
When priv->descriptor_mode == STMMAC_CHAIN_MODE and IEEE 1588-2002
timestamping is enabled without extended descriptors (!priv->extend_desc
with priv->hwts_rx_en or priv->hwts_tx_en), doesn't the hardware overwrite
p->des3 with the upper 32 bits of the timestamp on descriptor write-back?

In drivers/net/ethernet/stmicro/stmmac/chain_mode.c, refill_desc3() and
clean_desc3() note that the device overwrites des3 with a timestamp value:

static void refill_desc3(struct stmmac_rx_queue *rx_q, struct dma_desc *p)
{
	struct stmmac_priv *priv = rx_q->priv_data;

	if (priv->hwts_rx_en && !priv->extend_desc)
		/* NOTE: Device will overwrite des3 with timestamp value if
		 * 1588-2002 time stamping is enabled, hence reinitialize it
		 * to keep explicit chaining in the descriptor.
		 */
		p->des3 = cpu_to_le32((unsigned int)(rx_q->dma_rx_phy +
				      (((rx_q->dirty_rx) + 1) %
				       priv->dma_conf.dma_rx_size) *
				      sizeof(struct dma_desc)));
}

If an RX or TX descriptor was written back before suspend without being
cleaned by NAPI, p->des3 still holds the timestamp rather than the next
descriptor's physical DMA address. Neither stmmac_resume_rx_descriptors(),
stmmac_clear_rx_descriptors(), nor stmmac_clear_tx_descriptors() calls
stmmac_mode_init() (as __init_dma_rx_desc_rings() and
__init_dma_tx_desc_rings() do) to restore the chained des3 pointers when
priv->descriptor_mode == STMMAC_CHAIN_MODE. Could the DMA engine then
fetch the next descriptor from the timestamp address after resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027150556.51209-1-thomas.bourgoin@foss.st.com?part=1