[PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[Ding Hui]

From: Ding Hui <dinghui@lixiang.com>

On suspend, stmmac_suspend() calls stmmac_disable_all_queues() which
stops the RX NAPI, but the RX DMA engine may still be running for a
short window before stmmac_stop_all_dma() takes effect. During that
window the hardware can write incoming frames into the buffers pointed
to by the RX descriptors and write back the descriptors (clearing the
OWN bit and overwriting RDES0/1/2 with status/timestamp data). Because
NAPI is already disabled, the driver never refills these descriptors,
so the RX ring is left in a "consumed but not refilled" state with
stale content in the descriptor buffer-address fields.

On resume, stmmac_clear_descriptors() only re-arms the OWN bit and
does not repopulate the RX buffer address fields. When the DMA is
restarted it dereferences these stale addresses and triggers a fatal
bus error.

Fix this without any allocation by introducing
stmmac_reinit_rx_descriptors(), called from stmmac_resume() before
stmmac_clear_descriptors(). The helper iterates every RX descriptor
slot and re-programs its buffer address fields:

 - For normal (page_pool) queues: restore RDES0/1 from buf->addr and
   RDES2 from buf->sec_addr. The DMA mapping has remained valid across
   suspend/resume because no pages were freed.

 - For AF_XDP zero-copy queues: restore the DMA address from
   xsk_buff_xdp_get_dma(buf->xdp). Slots with buf->xdp == NULL (TX-only
   XSK socket) are skipped to avoid a NULL-pointer dereference.

 - For chain mode: call stmmac_mode_init() to rebuild the des3 next-
   descriptor pointer chain, which hardware may have overwritten with
   a PTP timestamp value (as noted in chain_mode.c:refill_desc3()).

This approach keeps all RX buffers alive across the PM transition and
avoids any allocation in the resume path, eliminating the OOM risk
raised against the previous approach of freeing and re-allocating
buffers.

Signed-off-by: Ding Hui <dinghui@lixiang.com>
---

Changes in v2:
- Introducing stmmac_reinit_rx_descriptors() to reinitializing rx
  buffers without any allocation.
- Modify commit log.
- Link to v1:
  https://lore.kernel.org/netdev/20260515053856.2310369-1-dinghui1111@163.com/
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3591755ea30b..0dc27d8c66a0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1642,6 +1642,79 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
 		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
 }
 
+/**
+ * stmmac_reinit_rx_descriptors - re-program RX descriptors from existing
+ *				   buffers (allocation-free)
+ * @priv: driver private structure
+ * @dma_conf: structure holding the dma data
+ * @queue: RX queue index
+ *
+ * Description: walk rx_q->buf_pool[] and re-program every RX descriptor's
+ * buffer-address fields from the buffers that are already attached to the
+ * queue. This is intended for the resume path: between suspend and resume
+ * the descriptor buffer-address fields may have been overwritten by HW
+ * writeback (RDESx are reused for status/length on completion), but the
+ * underlying RX buffers (page_pool pages or XSK frames) are still alive
+ * in buf_pool[]. By re-using them we avoid any allocation on resume,
+ * which is unsafe under memory pressure.
+ *
+ * This helper is expected to be called only in stmmac_resume.
+ */
+static void stmmac_reinit_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;
+
+	for (i = 0; i < dma_conf->dma_rx_size; i++) {
+		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
+		struct dma_desc *p = stmmac_get_rx_desc(priv, rx_q, i);
+
+		if (rx_q->xsk_pool) {
+			dma_addr_t dma_addr;
+
+			/* The XSK pool may not be fully populated (e.g.
+			 * xdpsock TX-only); skip empty slots.
+			 */
+			if (!buf->xdp)
+				continue;
+
+			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);
+		} else {
+			/* Theoretically unreachable: napi_disable() in
+			 * stmmac_suspend() ensures all initialized slots
+			 * have a valid page before we get here.
+			 * Defensive check only.
+			 */
+			if (!buf->page)
+				continue;
+
+			stmmac_set_desc_addr(priv, p, buf->addr);
+			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
+						 priv->sph_active &&
+						 buf->sec_page);
+
+			if (dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
+				stmmac_init_desc3(priv, p);
+		}
+	}
+
+	/* Chain mode: re-link descriptor 'next' pointers. This is
+	 * allocation-free; it just rewrites the per-descriptor next
+	 * field which may have been clobbered by HW writeback.
+	 */
+	if (priv->descriptor_mode == STMMAC_CHAIN_MODE) {
+		void *des = priv->extend_desc ? (void *)rx_q->dma_erx
+					      : (void *)rx_q->dma_rx;
+
+		stmmac_mode_init(priv, des, rx_q->dma_rx_phy,
+				 dma_conf->dma_rx_size, priv->extend_desc);
+	}
+}
+
 /**
  * stmmac_init_rx_buffers - init the RX descriptor buffer.
  * @priv: driver private structure
@@ -8272,6 +8345,7 @@ int stmmac_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
+	u32 queue;
 	int ret;
 
 	if (priv->plat->resume) {
@@ -8316,6 +8390,22 @@ int stmmac_resume(struct device *dev)
 
 	mutex_lock(&priv->lock);
 
+	/* Re-program the RX descriptors from the buffers that are still
+	 * attached to priv->dma_conf.rx_queue[].buf_pool[]. The buffer-
+	 * address fields of the RX descriptors may have been overwritten
+	 * by HW writeback while the DMA was being stopped on suspend
+	 * (RDESx are reused for status/length on completion), so they
+	 * must be repopulated before the DMA is restarted in
+	 * stmmac_hw_setup() below; otherwise the controller would
+	 * dereference stale addresses and trigger a fatal bus error.
+	 *
+	 * This path is allocation-free: it relies entirely on the RX
+	 * buffers preserved across suspend, which makes the resume path
+	 * safe under memory pressure.
+	 */
+	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
+		stmmac_reinit_rx_descriptors(priv, &priv->dma_conf, queue);
+
 	stmmac_reset_queues_param(priv);
 
 	stmmac_free_tx_skbufs(priv);
-- 
2.34.1

Re: [PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

This patch adds stmmac_reinit_rx_descriptors() and calls it from
stmmac_resume() before stmmac_clear_descriptors() to re-program the RX
descriptor buffer-address fields from buffers that survived across the
PM transition, avoiding any allocation in the resume path.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3591755ea30b..0dc27d8c66a0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1642,6 +1642,79 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
>  		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
>  }
>  
> +/**
> + * stmmac_reinit_rx_descriptors - re-program RX descriptors from existing
> + *				   buffers (allocation-free)

[ ... ]

> +static void stmmac_reinit_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;
> +
> +	for (i = 0; i < dma_conf->dma_rx_size; i++) {
> +		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
> +		struct dma_desc *p = stmmac_get_rx_desc(priv, rx_q, i);
> +
> +		if (rx_q->xsk_pool) {
> +			dma_addr_t dma_addr;
> +
> +			/* The XSK pool may not be fully populated (e.g.
> +			 * xdpsock TX-only); skip empty slots.
> +			 */
> +			if (!buf->xdp)
> +				continue;

[High]
Does the continue here regress to the same fatal bus error this patch
sets out to fix?

When the slot is skipped, its descriptor address fields keep whatever
HW writeback left behind. Then stmmac_clear_descriptors() runs right
after stmmac_reinit_rx_descriptors() in stmmac_resume() and unconditionally
re-arms OWN on every slot. For dwmac4 that is dwmac4_set_rx_owner():

	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR |
			       RDES3_INT_ON_COMPLETION_EN);

and for enh_desc / ndesc the equivalent OR of RDES0_OWN / RDES1 fields.
None of these clear the address fields.

When stmmac_hw_setup() restarts the DMA, the controller sees an owned
descriptor with VALID_ADDR set and a stale address — the exact failure
mode the commit message describes. Could the skipped slots be cleared
(stmmac_clear_desc() or zeroing des0/des1/des2) instead of left intact,
or arranged so clear_descriptors() does not arm OWN on them?

> +
> +			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);
> +		} else {
> +			/* Theoretically unreachable: napi_disable() in
> +			 * stmmac_suspend() ensures all initialized slots
> +			 * have a valid page before we get here.
> +			 * Defensive check only.
> +			 */
> +			if (!buf->page)
> +				continue;

[High]
Is the comment "Theoretically unreachable: napi_disable() in
stmmac_suspend() ensures all initialized slots have a valid page"
accurate?

stmmac_rx() assigns buf->page = NULL at multiple points when handing the
page on to the stack or to XDP, for example:

	buf->page = NULL;

in stmmac_rx() after the page has been consumed.

stmmac_rx_refill() allocates with GFP_ATOMIC | __GFP_NOWARN and breaks
on allocation failure without advancing dirty_rx, leaving the failing
slot (and any subsequent slots) with buf->page == NULL:

	if (!buf->page) {
		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
		if (!buf->page)
			break;
		...
	}

stmmac_disable_all_queues() only does synchronize_rcu() and
napi_disable() — it does not retry refills. Under the memory-pressure
scenario this patch is explicitly designed to make safe, a prior
GFP_ATOMIC failure can leave the ring with NULL-page slots at the
moment suspend happens.

If such a slot is then skipped here, the same OWN-with-stale-address
condition described above applies on resume. Should the page_pool path
also clear the descriptor for skipped slots, or allocate a replacement
buffer rather than leave a stale-address descriptor for
stmmac_clear_descriptors() to arm?

> +
> +			stmmac_set_desc_addr(priv, p, buf->addr);
> +			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
> +						 priv->sph_active &&
> +						 buf->sec_page);
> +
> +			if (dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
> +				stmmac_init_desc3(priv, p);
> +		}
> +	}

[ ... ]

> +}
> +
-- 
This is an AI-generated review.

Re:Re: [PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[Ding Hui]

At 2026-05-28 20:02:02, "Paolo Abeni" <pabeni@redhat.com> wrote:

>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 3591755ea30b..0dc27d8c66a0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1642,6 +1642,79 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
>>  		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
>>  }
>>  
>> +/**
>> + * stmmac_reinit_rx_descriptors - re-program RX descriptors from existing
>> + *				   buffers (allocation-free)
>
>[ ... ]
>
>> +static void stmmac_reinit_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;
>> +
>> +	for (i = 0; i < dma_conf->dma_rx_size; i++) {
>> +		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>> +		struct dma_desc *p = stmmac_get_rx_desc(priv, rx_q, i);
>> +
>> +		if (rx_q->xsk_pool) {
>> +			dma_addr_t dma_addr;
>> +
>> +			/* The XSK pool may not be fully populated (e.g.
>> +			 * xdpsock TX-only); skip empty slots.
>> +			 */
>> +			if (!buf->xdp)
>> +				continue;
>
>[High]
>Does the continue here regress to the same fatal bus error this patch
>sets out to fix?
>
>When the slot is skipped, its descriptor address fields keep whatever
>HW writeback left behind. Then stmmac_clear_descriptors() runs right
>after stmmac_reinit_rx_descriptors() in stmmac_resume() and unconditionally
>re-arms OWN on every slot. For dwmac4 that is dwmac4_set_rx_owner():
>
>	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR |
>			       RDES3_INT_ON_COMPLETION_EN);
>
>and for enh_desc / ndesc the equivalent OR of RDES0_OWN / RDES1 fields.
>None of these clear the address fields.
>
>When stmmac_hw_setup() restarts the DMA, the controller sees an owned
>descriptor with VALID_ADDR set and a stale address — the exact failure
>mode the commit message describes. Could the skipped slots be cleared
>(stmmac_clear_desc() or zeroing des0/des1/des2) instead of left intact,
>or arranged so clear_descriptors() does not arm OWN on them?

It's right that the skipped slots must be cleared instead of left intact.
This change will be reflected in version v3.

>
>> +
>> +			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);
>> +		} else {
>> +			/* Theoretically unreachable: napi_disable() in
>> +			 * stmmac_suspend() ensures all initialized slots
>> +			 * have a valid page before we get here.
>> +			 * Defensive check only.
>> +			 */
>> +			if (!buf->page)
>> +				continue;
>
>[High]
>Is the comment "Theoretically unreachable: napi_disable() in
>stmmac_suspend() ensures all initialized slots have a valid page"
>accurate?
>
>stmmac_rx() assigns buf->page = NULL at multiple points when handing the
>page on to the stack or to XDP, for example:
>
>	buf->page = NULL;
>
>in stmmac_rx() after the page has been consumed.
>
>stmmac_rx_refill() allocates with GFP_ATOMIC | __GFP_NOWARN and breaks
>on allocation failure without advancing dirty_rx, leaving the failing
>slot (and any subsequent slots) with buf->page == NULL:
>
>	if (!buf->page) {
>		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>		if (!buf->page)
>			break;
>		...
>	}
>
>stmmac_disable_all_queues() only does synchronize_rcu() and
>napi_disable() — it does not retry refills. Under the memory-pressure
>scenario this patch is explicitly designed to make safe, a prior
>GFP_ATOMIC failure can leave the ring with NULL-page slots at the
>moment suspend happens.
>
>If such a slot is then skipped here, the same OWN-with-stale-address
>condition described above applies on resume. Should the page_pool path
>also clear the descriptor for skipped slots, or allocate a replacement
>buffer rather than leave a stale-address descriptor for
>stmmac_clear_descriptors() to arm?
>
>> +
>> +			stmmac_set_desc_addr(priv, p, buf->addr);
>> +			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
>> +						 priv->sph_active &&
>> +						 buf->sec_page);
>> +
>> +			if (dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
>> +				stmmac_init_desc3(priv, p);
>> +		}
>> +	}
>
>[ ... ]
>
>> +}
>> +

Same as above, the skipped slots must be cleared instead of left intact.
This change will be reflected in version v3.

Thanks for the review. I will upload the v3 version soon.

Ding Hui

Re: [PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[Jakub Raczynski]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Tue, May 26, 2026 at 10:26:17AM +0800, Ding Hui wrote:
> From: Ding Hui <dinghui@lixiang.com>
> +		} else {
> +			/* Theoretically unreachable: napi_disable() in
> +			 * stmmac_suspend() ensures all initialized slots
> +			 * have a valid page before we get here.
> +			 * Defensive check only.
> +			 */
> +			if (!buf->page)
> +				continue;
> +
> +			stmmac_set_desc_addr(priv, p, buf->addr);
> +			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
> +						 priv->sph_active &&
> +						 buf->sec_page);

It this generally sufficient?  Or, in fact, isn't that overkill?
stmmac_rx_refill() generally does a bit more preparation of descriptors.
The issue seems to be that during suspend there is mismatch,
caused by writeback format, between rx_dirty and rx_cur pointers and
there is bad handling of this case, since there is no verification
of leftover stuff and there will be leftover bad address crashing platform.
So stmmac needs to refill/reinit descriptors that were consumed but not
refilled. So isn't going through whole dma_rx_size overkill?
Wouldn't it be better to iterate over buffer from cur_rx as long as descriptors
are 0 and only apply refill to those corrupted?

Could you paste panic that occurs during this issue?
You mention "fatal bus error" which I would assume is system panic?

Thanks

BR
Jakub Raczynski

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re:Re: [PATCH v2] net: stmmac: fix fatal bus error on resume by reinitializing RX buffers

[Ding Hui]

At 2026-05-28 22:57:38, "Jakub Raczynski" <j.raczynski@samsung.com> wrote:
>On Tue, May 26, 2026 at 10:26:17AM +0800, Ding Hui wrote:
>> From: Ding Hui <dinghui@lixiang.com>
>> +		} else {
>> +			/* Theoretically unreachable: napi_disable() in
>> +			 * stmmac_suspend() ensures all initialized slots
>> +			 * have a valid page before we get here.
>> +			 * Defensive check only.
>> +			 */
>> +			if (!buf->page)
>> +				continue;
>> +
>> +			stmmac_set_desc_addr(priv, p, buf->addr);
>> +			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
>> +						 priv->sph_active &&
>> +						 buf->sec_page);
>
>It this generally sufficient?  Or, in fact, isn't that overkill?
>stmmac_rx_refill() generally does a bit more preparation of descriptors.

You are right that stmmac_rx_refill() does more work — it allocates new
pages and maps them. The key difference here is that in v2 we intentionally
keep all RX buffers alive across suspend/resume, so no allocation is needed.
The only thing that needs to be restored is the buffer address fields in the
descriptors, which were overwritten by hardware write-back.

>The issue seems to be that during suspend there is mismatch,
>caused by writeback format, between rx_dirty and rx_cur pointers and
>there is bad handling of this case, since there is no verification
>of leftover stuff and there will be leftover bad address crashing platform.
>So stmmac needs to refill/reinit descriptors that were consumed but not
>refilled. So isn't going through whole dma_rx_size overkill?
>Wouldn't it be better to iterate over buffer from cur_rx as long as descriptors
>are 0 and only apply refill to those corrupted?

Actually, The hardware may have consumed additional descriptors in the window
between stmmac_disable_all_queues() and stmmac_stop_all_dma(), so cur_rx can lag
behind the hardware's actual position. So maybe not only the descriptors between
rx_dirty and rx_cur pointers need to be refilled. 
You are right that we should only refill the consumed descriptors. But checking the
OWN bit requires a new lightweight get_rx_owner() helper across all descriptor
variants (dwmac4, dwxgmac2, norm_desc, enh_desc), adding complexity for marginal gain.

>Could you paste panic that occurs during this issue?
>You mention "fatal bus error" which I would assume is system panic?

Apologies for the misleading wording — this does not cause a kernel panic.
The issue manifests as a Fatal Bus Error interrupt on the DMA controller.
Taking XGMAC as an example, dwxgmac2_dma_interrupt() detects XGMAC_FBE,
increments fatal_bus_error_irq, and returns tx_hard_error, which triggers
stmmac_tx_err() to stop and reset the TX DMA channel. But this has no effect
for the RX DMA engine (may be we should reset RX DMA here). The practical effect
is that the RX DMA engine halts after dereferencing the invalid buffer address,
and the network interface becomes non-functional after resume — no packets can be
received until the driver is reloaded or the device is re-probed.

To reproduce the issue on my platform:
  1. Connect the DUT and a PC, configure IP addresses so they can ping
     each other (e.g. DUT: 192.168.1.1, PC: 192.168.1.100).

  2. On the PC, start an iperf3 server:
       iperf3 -s

  3. On the DUT, start a high-rate reverse UDP stream to keep the RX DMA
     busy during suspend:
       iperf3 -c 192.168.1.100 -u -b 900M -R -t 0

  4. While iperf3 is running, trigger a suspend/resume cycle on the DUT.

  5. After resume, check the fatal_bus_error_irq counter:
       ethtool -S <iface> | grep fatal_bus_error_irq

     Without this fix the counter increments and the interface stops
     receiving packets. With this fix the counter stays at zero and
     normal operation resumes.

I will update the commit message to clarify "fatal bus error causing RX
DMA to stop".

Thanks for the review.

Ding Hui