[PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames

[PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames

[Padmarao Begari]

The MRMAC Rx path drops frames when two arrive back to back. recv()
always starts at rx_bd[0] and clears the status of both descriptors,
and the status field is the only record that a frame arrived, so the
second frame is lost without any error being reported. This shows up
with multiple boards on a switch, where the extra traffic makes
back-to-back arrivals common and network transfers time out.

Fixing that needs an Rx ring the driver can index, so the series first
makes the ring scalable and then fixes the bug:

  1-3  Independent cleanups: check memalign() failures, give the
       driver its own Rx buffer pool instead of borrowing the shared
       net_rx_packets[], and read the link speed from the standard
       max-speed property.

  4-6  Make the descriptor ring scale: index the contiguous BD blocks
       directly, build the Rx chain in a loop over RX_DESC, and
       program CURDESC only once the ring is complete in memory.

  7    Track the descriptor to consume next in rx_bd_idx and take
       completion from the per-descriptor COMPLETE bit, so a
       descriptor goes back to hardware only after the network stack
       has read it.

  8    Size the Rx ring from ETH_PACKETS_BATCH_RECV, so a full
       eth_rx() call can be served without hardware running out of
       descriptors.

Padmarao Begari (8):
  net: mrmac: check memalign() return values
  net: mrmac: use a driver-owned RX buffer pool
  net: mrmac: switch to max-speed property
  net: mrmac: use contiguous BD arrays
  net: mrmac: initialize the Rx BD ring in a loop
  net: mrmac: write CURDESC after ring setup
  net: mrmac: fix Rx packet loss on back-to-back frames
  net: mrmac: increase the Rx BD ring

 drivers/net/xilinx_axi_mrmac.c | 235 +++++++++++++++++----------------
 drivers/net/xilinx_axi_mrmac.h |  20 ++-
 2 files changed, 137 insertions(+), 118 deletions(-)

-- 
2.34.1

[PATCH 1/8] net: mrmac: check memalign() return values

[Padmarao Begari]

The memalign() allocations in probe() are used without checking for
failure. Bail out with -ENOMEM if any of them fails.

Fixes: 258ce79cfce4 ("net: xilinx: axi_mrmac: Add MRMAC driver")
Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index 56f877c20a6..5ff304fc971 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -484,14 +484,20 @@ static int axi_mrmac_probe(struct udevice *dev)
 
 	/* Align buffers to ARCH_DMA_MINALIGN */
 	priv->tx_bd[0] = memalign(ARCH_DMA_MINALIGN, TX_BD_TOTAL_SIZE);
+	if (!priv->tx_bd[0])
+		return -ENOMEM;
 	priv->tx_bd[1] = (struct mcdma_bd *)((ulong)priv->tx_bd[0] +
 					     sizeof(struct mcdma_bd));
 
 	priv->rx_bd[0] = memalign(ARCH_DMA_MINALIGN, RX_BD_TOTAL_SIZE);
+	if (!priv->rx_bd[0])
+		return -ENOMEM;
 	priv->rx_bd[1] = (struct mcdma_bd *)((ulong)priv->rx_bd[0] +
 					     sizeof(struct mcdma_bd));
 
 	priv->txminframe = memalign(ARCH_DMA_MINALIGN, MIN_PKT_SIZE);
+	if (!priv->txminframe)
+		return -ENOMEM;
 
 	return 0;
 }
-- 
2.34.1

[PATCH 2/8] net: mrmac: use a driver-owned RX buffer pool

[Padmarao Begari]

The RX path borrows the shared net_rx_packets[] global for its buffer
descriptors. That global is capped at PKTBUFSRX system-wide and is not
private to this device, so it cannot scale to a deeper RX ring and does
not compose well with multiple different drivers sharing it.

Allocate a driver-owned RX buffer pool (rx_buf, sized
RX_DESC * PKTSIZE_ALIGN) in probe() and point the RX descriptors at
slices of it instead.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 24 +++++++++++++++++-------
 drivers/net/xilinx_axi_mrmac.h |  1 +
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index 5ff304fc971..d9d4756d14f 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -185,17 +185,17 @@ static int axi_mrmac_start(struct udevice *dev)
 	memset(priv->rx_bd[0], 0, RX_BD_TOTAL_SIZE);
 
 	priv->rx_bd[0]->next_desc = lower_32_bits((u64)priv->rx_bd[1]);
-	priv->rx_bd[0]->buf_addr = lower_32_bits((u64)net_rx_packets[0]);
+	priv->rx_bd[0]->buf_addr = lower_32_bits((u64)priv->rx_buf);
 
 	priv->rx_bd[1]->next_desc = lower_32_bits((u64)priv->rx_bd[0]);
-	priv->rx_bd[1]->buf_addr = lower_32_bits((u64)net_rx_packets[1]);
+	priv->rx_bd[1]->buf_addr = lower_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
 
 	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
 		priv->rx_bd[0]->next_desc_msb = upper_32_bits((u64)priv->rx_bd[1]);
-		priv->rx_bd[0]->buf_addr_msb = upper_32_bits((u64)net_rx_packets[0]);
+		priv->rx_bd[0]->buf_addr_msb = upper_32_bits((u64)priv->rx_buf);
 
 		priv->rx_bd[1]->next_desc_msb = upper_32_bits((u64)priv->rx_bd[0]);
-		priv->rx_bd[1]->buf_addr_msb = upper_32_bits((u64)net_rx_packets[1]);
+		priv->rx_bd[1]->buf_addr_msb = upper_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
 	}
 
 	priv->rx_bd[0]->cntrl = PKTSIZE_ALIGN;
@@ -207,7 +207,7 @@ static int axi_mrmac_start(struct udevice *dev)
 	/* It is necessary to flush rx buffers because if you don't do it
 	 * then cache can contain uninitialized data
 	 */
-	flush_cache((phys_addr_t)priv->rx_bd[0]->buf_addr, RX_BUFF_TOTAL_SIZE);
+	flush_cache((phys_addr_t)priv->rx_buf, RX_BUFF_TOTAL_SIZE);
 
 	/* Start the hardware */
 	setbits_le32(&priv->s2mm_cmn->control, XMCDMA_CR_RUNSTOP_MASK);
@@ -413,7 +413,7 @@ static int axi_mrmac_free_pkt(struct udevice *dev, uchar *packet, int length)
 
 #ifdef DEBUG
 	/* It is useful to clear buffer to be sure that it is consistent */
-	memset(priv->rx_bd[0]->buf_addr, 0, RX_BUFF_TOTAL_SIZE);
+	memset(priv->rx_buf, 0, RX_BUFF_TOTAL_SIZE);
 #endif
 	/* Disable all Rx interrupts before RxBD space setup */
 	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
@@ -430,7 +430,7 @@ static int axi_mrmac_free_pkt(struct udevice *dev, uchar *packet, int length)
 	/* It is necessary to flush rx buffers because if you don't do it
 	 * then cache will contain previous packet
 	 */
-	flush_cache((phys_addr_t)priv->rx_bd[0]->buf_addr, RX_BUFF_TOTAL_SIZE);
+	flush_cache((phys_addr_t)priv->rx_buf, RX_BUFF_TOTAL_SIZE);
 
 	/* Enable all IRQ */
 	setbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
@@ -495,6 +495,15 @@ static int axi_mrmac_probe(struct udevice *dev)
 	priv->rx_bd[1] = (struct mcdma_bd *)((ulong)priv->rx_bd[0] +
 					     sizeof(struct mcdma_bd));
 
+	/*
+	 * Use a driver-owned RX buffer pool rather than the shared
+	 * net_rx_packets[] global, which is capped at PKTBUFSRX system-wide
+	 * and not private to this device.
+	 */
+	priv->rx_buf = memalign(ARCH_DMA_MINALIGN, RX_BUFF_TOTAL_SIZE);
+	if (!priv->rx_buf)
+		return -ENOMEM;
+
 	priv->txminframe = memalign(ARCH_DMA_MINALIGN, MIN_PKT_SIZE);
 	if (!priv->txminframe)
 		return -ENOMEM;
@@ -509,6 +518,7 @@ static int axi_mrmac_remove(struct udevice *dev)
 	/* Free buffer descriptors */
 	free(priv->tx_bd[0]);
 	free(priv->rx_bd[0]);
+	free(priv->rx_buf);
 	free(priv->txminframe);
 
 	return 0;
diff --git a/drivers/net/xilinx_axi_mrmac.h b/drivers/net/xilinx_axi_mrmac.h
index e2c2105450c..17849d39873 100644
--- a/drivers/net/xilinx_axi_mrmac.h
+++ b/drivers/net/xilinx_axi_mrmac.h
@@ -37,6 +37,7 @@ struct axi_mrmac_priv {
 	struct mcdma_bd *tx_bd[TX_DESC];
 	struct mcdma_bd *rx_bd[RX_DESC];
 	u8 *txminframe;		/* Pointer to hold min length Tx frame(60) */
+	u8 *rx_buf;		/* Driver-owned RX buffer pool (RX_DESC * PKTSIZE_ALIGN) */
 	u32 mrmac_rate;		/* Speed to configure(Read from DT 10G/25G..) */
 };
 
-- 
2.34.1

[PATCH 3/8] net: mrmac: switch to max-speed property

[Padmarao Begari]

Read the link speed from the standard "max-speed" property instead
of the vendor-specific "xlnx,mrmac-rate".

Fall back to "xlnx,mrmac-rate" if "max-speed" is not present, and
default to 10000 (10G) if neither is set.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index d9d4756d14f..8778ab1aaa9 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -547,8 +547,9 @@ static int axi_mrmac_of_to_plat(struct udevice *dev)
 		return -EINVAL;
 	}
 
-	/* Set default MRMAC rate to 10000 */
-	plat->mrmac_rate = dev_read_u32_default(dev, "xlnx,mrmac-rate", 10000);
+	if (dev_read_u32(dev, "max-speed", &plat->mrmac_rate))
+		/* Set default MRMAC rate to 10000 */
+		plat->mrmac_rate = dev_read_u32_default(dev, "xlnx,mrmac-rate", 10000);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 4/8] net: mrmac: use contiguous BD arrays

[Padmarao Begari]

The Tx and Rx buffer descriptors are each allocated as one contiguous
block by a single memalign() call, but they are tracked as an array of
pointers into that block, with every pointer past the first derived by
hand:

	priv->tx_bd[1] = (struct mcdma_bd *)((ulong)priv->tx_bd[0] +
					     sizeof(struct mcdma_bd));

That per-descriptor pointer arithmetic has to be repeated for every
descriptor added, so it does not scale beyond the current two.

Keep only the base pointer of each ring and index it directly.
priv->tx_bd[0] and priv->tx_bd[1] then address the first and second
descriptor of the contiguous block, and the hand-written pointer setup
in probe() disappears.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 100 ++++++++++++++++-----------------
 drivers/net/xilinx_axi_mrmac.h |   4 +-
 2 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index 8778ab1aaa9..d77b0e3445b 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -179,30 +179,30 @@ static int axi_mrmac_start(struct udevice *dev)
 	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
 
 	/* Update current descriptor */
-	axi_mrmac_dma_write(priv->rx_bd[0], &priv->mcdma_rx->current);
+	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
 
 	/* Setup Rx BD. MRMAC needs atleast two descriptors */
-	memset(priv->rx_bd[0], 0, RX_BD_TOTAL_SIZE);
+	memset(priv->rx_bd, 0, RX_BD_TOTAL_SIZE);
 
-	priv->rx_bd[0]->next_desc = lower_32_bits((u64)priv->rx_bd[1]);
-	priv->rx_bd[0]->buf_addr = lower_32_bits((u64)priv->rx_buf);
+	priv->rx_bd[0].next_desc = lower_32_bits((u64)&priv->rx_bd[1]);
+	priv->rx_bd[0].buf_addr = lower_32_bits((u64)priv->rx_buf);
 
-	priv->rx_bd[1]->next_desc = lower_32_bits((u64)priv->rx_bd[0]);
-	priv->rx_bd[1]->buf_addr = lower_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
+	priv->rx_bd[1].next_desc = lower_32_bits((u64)&priv->rx_bd[0]);
+	priv->rx_bd[1].buf_addr = lower_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
 
 	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
-		priv->rx_bd[0]->next_desc_msb = upper_32_bits((u64)priv->rx_bd[1]);
-		priv->rx_bd[0]->buf_addr_msb = upper_32_bits((u64)priv->rx_buf);
+		priv->rx_bd[0].next_desc_msb = upper_32_bits((u64)&priv->rx_bd[1]);
+		priv->rx_bd[0].buf_addr_msb = upper_32_bits((u64)priv->rx_buf);
 
-		priv->rx_bd[1]->next_desc_msb = upper_32_bits((u64)priv->rx_bd[0]);
-		priv->rx_bd[1]->buf_addr_msb = upper_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
+		priv->rx_bd[1].next_desc_msb = upper_32_bits((u64)&priv->rx_bd[0]);
+		priv->rx_bd[1].buf_addr_msb = upper_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
 	}
 
-	priv->rx_bd[0]->cntrl = PKTSIZE_ALIGN;
-	priv->rx_bd[1]->cntrl = PKTSIZE_ALIGN;
+	priv->rx_bd[0].cntrl = PKTSIZE_ALIGN;
+	priv->rx_bd[1].cntrl = PKTSIZE_ALIGN;
 
 	/* Flush the last BD so DMA core could see the updates */
-	flush_cache((phys_addr_t)priv->rx_bd[0], RX_BD_TOTAL_SIZE);
+	flush_cache((phys_addr_t)priv->rx_bd, RX_BD_TOTAL_SIZE);
 
 	/* It is necessary to flush rx buffers because if you don't do it
 	 * then cache can contain uninitialized data
@@ -218,7 +218,7 @@ static int axi_mrmac_start(struct udevice *dev)
 	setbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
 
 	/* Update tail descriptor. Now it's ready to receive data */
-	axi_mrmac_dma_write(priv->rx_bd[1], &priv->mcdma_rx->tail);
+	axi_mrmac_dma_write(&priv->rx_bd[1], &priv->mcdma_rx->tail);
 
 	/* Enable Tx */
 	setbits_le32(&regs->tx_config, MRMAC_TX_EN_MASK);
@@ -267,32 +267,32 @@ static int axi_mrmac_send(struct udevice *dev, void *ptr, int len)
 	flush_cache((phys_addr_t)ptr, len);
 
 	/* Setup Tx BD. MRMAC needs atleast two descriptors */
-	memset(priv->tx_bd[0], 0, TX_BD_TOTAL_SIZE);
+	memset(priv->tx_bd, 0, TX_BD_TOTAL_SIZE);
 
-	priv->tx_bd[0]->next_desc = lower_32_bits((u64)priv->tx_bd[1]);
-	priv->tx_bd[0]->buf_addr = lower_32_bits((u64)ptr);
+	priv->tx_bd[0].next_desc = lower_32_bits((u64)&priv->tx_bd[1]);
+	priv->tx_bd[0].buf_addr = lower_32_bits((u64)ptr);
 
 	/* At the end of the ring, link the last BD back to the top */
-	priv->tx_bd[1]->next_desc = lower_32_bits((u64)priv->tx_bd[0]);
-	priv->tx_bd[1]->buf_addr = lower_32_bits((u64)ptr + len / 2);
+	priv->tx_bd[1].next_desc = lower_32_bits((u64)&priv->tx_bd[0]);
+	priv->tx_bd[1].buf_addr = lower_32_bits((u64)ptr + len / 2);
 
 	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
-		priv->tx_bd[0]->next_desc_msb = upper_32_bits((u64)priv->tx_bd[1]);
-		priv->tx_bd[0]->buf_addr_msb = upper_32_bits((u64)ptr);
+		priv->tx_bd[0].next_desc_msb = upper_32_bits((u64)&priv->tx_bd[1]);
+		priv->tx_bd[0].buf_addr_msb = upper_32_bits((u64)ptr);
 
-		priv->tx_bd[1]->next_desc_msb = upper_32_bits((u64)priv->tx_bd[0]);
-		priv->tx_bd[1]->buf_addr_msb = upper_32_bits((u64)ptr + len / 2);
+		priv->tx_bd[1].next_desc_msb = upper_32_bits((u64)&priv->tx_bd[0]);
+		priv->tx_bd[1].buf_addr_msb = upper_32_bits((u64)ptr + len / 2);
 	}
 
 	/* Split Tx data in to half and send in two descriptors */
-	priv->tx_bd[0]->cntrl = (len / 2) | XMCDMA_BD_CTRL_TXSOF_MASK;
-	priv->tx_bd[1]->cntrl = (len - len / 2) | XMCDMA_BD_CTRL_TXEOF_MASK;
+	priv->tx_bd[0].cntrl = (len / 2) | XMCDMA_BD_CTRL_TXSOF_MASK;
+	priv->tx_bd[1].cntrl = (len - len / 2) | XMCDMA_BD_CTRL_TXEOF_MASK;
 
 	/* Flush the last BD so DMA core could see the updates */
-	flush_cache((phys_addr_t)priv->tx_bd[0], TX_BD_TOTAL_SIZE);
+	flush_cache((phys_addr_t)priv->tx_bd, TX_BD_TOTAL_SIZE);
 
 	if (readl(&priv->mcdma_tx->status) & XMCDMA_CH_IDLE) {
-		axi_mrmac_dma_write(priv->tx_bd[0], &priv->mcdma_tx->current);
+		axi_mrmac_dma_write(&priv->tx_bd[0], &priv->mcdma_tx->current);
 		/* Channel fetch */
 		setbits_le32(&priv->mcdma_tx->control, XMCDMA_CR_RUNSTOP_MASK);
 	} else {
@@ -303,7 +303,7 @@ static int axi_mrmac_send(struct udevice *dev, void *ptr, int len)
 	setbits_le32(&priv->mcdma_tx->control, XMCDMA_IRQ_ALL_MASK);
 
 	/* Start transfer */
-	axi_mrmac_dma_write(priv->tx_bd[1], &priv->mcdma_tx->tail);
+	axi_mrmac_dma_write(&priv->tx_bd[1], &priv->mcdma_tx->tail);
 
 	/* Wait for transmission to complete */
 	ret = wait_for_bit_le32(&priv->mcdma_tx->status, XMCDMA_IRQ_IOC_MASK,
@@ -314,8 +314,8 @@ static int axi_mrmac_send(struct udevice *dev, void *ptr, int len)
 	}
 
 	/* Clear status */
-	priv->tx_bd[0]->sband_stats = 0;
-	priv->tx_bd[1]->sband_stats = 0;
+	priv->tx_bd[0].sband_stats = 0;
+	priv->tx_bd[1].sband_stats = 0;
 
 	log_debug("Sending complete\n");
 
@@ -372,25 +372,25 @@ static int axi_mrmac_recv(struct udevice *dev, int flags, uchar **packetp)
 	/* Disable channel fetch */
 	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
 
-	rx_bd_end = (ulong)priv->rx_bd[0] + roundup(RX_BD_TOTAL_SIZE,
-						    ARCH_DMA_MINALIGN);
+	rx_bd_end = (ulong)priv->rx_bd + roundup(RX_BD_TOTAL_SIZE,
+						 ARCH_DMA_MINALIGN);
 	/* Invalidate Rx descriptors to see proper Rx length */
-	invalidate_dcache_range((phys_addr_t)priv->rx_bd[0], rx_bd_end);
+	invalidate_dcache_range((phys_addr_t)priv->rx_bd, rx_bd_end);
 
-	length = priv->rx_bd[0]->status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
-	*packetp = (uchar *)(ulong)priv->rx_bd[0]->buf_addr;
+	length = priv->rx_bd[0].status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
+	*packetp = (uchar *)(ulong)priv->rx_bd[0].buf_addr;
 
 	if (!length) {
-		length = priv->rx_bd[1]->status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
-		*packetp = (uchar *)(ulong)priv->rx_bd[1]->buf_addr;
+		length = priv->rx_bd[1].status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
+		*packetp = (uchar *)(ulong)priv->rx_bd[1].buf_addr;
 	}
 
 #ifdef DEBUG
 	print_buffer(*packetp, *packetp, 1, length, 16);
 #endif
 	/* Clear status */
-	priv->rx_bd[0]->status = 0;
-	priv->rx_bd[1]->status = 0;
+	priv->rx_bd[0].status = 0;
+	priv->rx_bd[1].status = 0;
 
 	return length;
 }
@@ -422,10 +422,10 @@ static int axi_mrmac_free_pkt(struct udevice *dev, uchar *packet, int length)
 	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
 
 	/* Update current descriptor */
-	axi_mrmac_dma_write(priv->rx_bd[0], &priv->mcdma_rx->current);
+	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
 
 	/* Write bd to HW */
-	flush_cache((phys_addr_t)priv->rx_bd[0], RX_BD_TOTAL_SIZE);
+	flush_cache((phys_addr_t)priv->rx_bd, RX_BD_TOTAL_SIZE);
 
 	/* It is necessary to flush rx buffers because if you don't do it
 	 * then cache will contain previous packet
@@ -439,7 +439,7 @@ static int axi_mrmac_free_pkt(struct udevice *dev, uchar *packet, int length)
 	setbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
 
 	/* Update tail descriptor. Now it's ready to receive data */
-	axi_mrmac_dma_write(priv->rx_bd[1], &priv->mcdma_rx->tail);
+	axi_mrmac_dma_write(&priv->rx_bd[1], &priv->mcdma_rx->tail);
 
 	log_debug("Rx completed, framelength = %x\n", length);
 
@@ -483,17 +483,13 @@ static int axi_mrmac_probe(struct udevice *dev)
 	priv->mrmac_rate = plat->mrmac_rate;
 
 	/* Align buffers to ARCH_DMA_MINALIGN */
-	priv->tx_bd[0] = memalign(ARCH_DMA_MINALIGN, TX_BD_TOTAL_SIZE);
-	if (!priv->tx_bd[0])
+	priv->tx_bd = memalign(ARCH_DMA_MINALIGN, TX_BD_TOTAL_SIZE);
+	if (!priv->tx_bd)
 		return -ENOMEM;
-	priv->tx_bd[1] = (struct mcdma_bd *)((ulong)priv->tx_bd[0] +
-					     sizeof(struct mcdma_bd));
 
-	priv->rx_bd[0] = memalign(ARCH_DMA_MINALIGN, RX_BD_TOTAL_SIZE);
-	if (!priv->rx_bd[0])
+	priv->rx_bd = memalign(ARCH_DMA_MINALIGN, RX_BD_TOTAL_SIZE);
+	if (!priv->rx_bd)
 		return -ENOMEM;
-	priv->rx_bd[1] = (struct mcdma_bd *)((ulong)priv->rx_bd[0] +
-					     sizeof(struct mcdma_bd));
 
 	/*
 	 * Use a driver-owned RX buffer pool rather than the shared
@@ -516,8 +512,8 @@ static int axi_mrmac_remove(struct udevice *dev)
 	struct axi_mrmac_priv *priv = dev_get_priv(dev);
 
 	/* Free buffer descriptors */
-	free(priv->tx_bd[0]);
-	free(priv->rx_bd[0]);
+	free(priv->tx_bd);
+	free(priv->rx_bd);
 	free(priv->rx_buf);
 	free(priv->txminframe);
 
diff --git a/drivers/net/xilinx_axi_mrmac.h b/drivers/net/xilinx_axi_mrmac.h
index 17849d39873..baf936f6b66 100644
--- a/drivers/net/xilinx_axi_mrmac.h
+++ b/drivers/net/xilinx_axi_mrmac.h
@@ -34,8 +34,8 @@ struct axi_mrmac_priv {
 	struct mcdma_common_regs *s2mm_cmn;
 	struct mcdma_chan_reg *mcdma_tx;
 	struct mcdma_chan_reg *mcdma_rx;
-	struct mcdma_bd *tx_bd[TX_DESC];
-	struct mcdma_bd *rx_bd[RX_DESC];
+	struct mcdma_bd *tx_bd;	/* Base of the contiguous Tx BD ring */
+	struct mcdma_bd *rx_bd;	/* Base of the contiguous Rx BD ring */
 	u8 *txminframe;		/* Pointer to hold min length Tx frame(60) */
 	u8 *rx_buf;		/* Driver-owned RX buffer pool (RX_DESC * PKTSIZE_ALIGN) */
 	u32 mrmac_rate;		/* Speed to configure(Read from DT 10G/25G..) */
-- 
2.34.1

[PATCH 5/8] net: mrmac: initialize the Rx BD ring in a loop

[Padmarao Begari]

The Rx descriptors are chained by hand, one assignment per descriptor
and per field. Adding a descriptor means adding another block of
next_desc/buf_addr/cntrl assignments, so the number of Rx descriptors
is effectively frozen at two.

Build the same chain in a loop over RX_DESC instead. Each descriptor
points at its successor, the last one wraps back to the first, and each
gets its own PKTSIZE_ALIGN slice of the Rx buffer pool. The tail
descriptor is now the last one of the ring rather than a hardcoded
rx_bd[1]. The resulting ring is identical to the hand-written one for
RX_DESC = 2, but the descriptor count is now a single constant to
change.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index d77b0e3445b..2c97e9576c9 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -162,6 +162,7 @@ static int axi_mrmac_start(struct udevice *dev)
 {
 	struct axi_mrmac_priv *priv = dev_get_priv(dev);
 	struct mrmac_regs *regs = priv->iobase;
+	int i;
 
 	/*
 	 * Initialize MCDMA engine. MCDMA engine must be initialized before
@@ -181,27 +182,30 @@ static int axi_mrmac_start(struct udevice *dev)
 	/* Update current descriptor */
 	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
 
-	/* Setup Rx BD. MRMAC needs atleast two descriptors */
+	/*
+	 * Setup Rx BDs as a closed ring: every descriptor points at the next
+	 * one and the last one wraps back to the first, each with its own
+	 * slice of the Rx buffer pool. MRMAC needs at least two descriptors.
+	 */
 	memset(priv->rx_bd, 0, RX_BD_TOTAL_SIZE);
 
-	priv->rx_bd[0].next_desc = lower_32_bits((u64)&priv->rx_bd[1]);
-	priv->rx_bd[0].buf_addr = lower_32_bits((u64)priv->rx_buf);
+	for (i = 0; i < RX_DESC; i++) {
+		struct mcdma_bd *next = &priv->rx_bd[(i + 1) % RX_DESC];
+		u8 *buf = priv->rx_buf + i * PKTSIZE_ALIGN;
+		struct mcdma_bd *bd = &priv->rx_bd[i];
 
-	priv->rx_bd[1].next_desc = lower_32_bits((u64)&priv->rx_bd[0]);
-	priv->rx_bd[1].buf_addr = lower_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
+		bd->next_desc = lower_32_bits((u64)next);
+		bd->buf_addr = lower_32_bits((u64)buf);
 
-	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
-		priv->rx_bd[0].next_desc_msb = upper_32_bits((u64)&priv->rx_bd[1]);
-		priv->rx_bd[0].buf_addr_msb = upper_32_bits((u64)priv->rx_buf);
+		if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
+			bd->next_desc_msb = upper_32_bits((u64)next);
+			bd->buf_addr_msb = upper_32_bits((u64)buf);
+		}
 
-		priv->rx_bd[1].next_desc_msb = upper_32_bits((u64)&priv->rx_bd[0]);
-		priv->rx_bd[1].buf_addr_msb = upper_32_bits((u64)priv->rx_buf + PKTSIZE_ALIGN);
+		bd->cntrl = PKTSIZE_ALIGN;
 	}
 
-	priv->rx_bd[0].cntrl = PKTSIZE_ALIGN;
-	priv->rx_bd[1].cntrl = PKTSIZE_ALIGN;
-
-	/* Flush the last BD so DMA core could see the updates */
+	/* Flush the BDs so DMA core could see the updates */
 	flush_cache((phys_addr_t)priv->rx_bd, RX_BD_TOTAL_SIZE);
 
 	/* It is necessary to flush rx buffers because if you don't do it
@@ -218,7 +222,7 @@ static int axi_mrmac_start(struct udevice *dev)
 	setbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
 
 	/* Update tail descriptor. Now it's ready to receive data */
-	axi_mrmac_dma_write(&priv->rx_bd[1], &priv->mcdma_rx->tail);
+	axi_mrmac_dma_write(&priv->rx_bd[RX_DESC - 1], &priv->mcdma_rx->tail);
 
 	/* Enable Tx */
 	setbits_le32(&regs->tx_config, MRMAC_TX_EN_MASK);
-- 
2.34.1

[PATCH 6/8] net: mrmac: write CURDESC after ring setup

[Padmarao Begari]

The current descriptor register is written before the Rx descriptors
are filled in and flushed, so the register is programmed against a ring
that does not exist yet.

Write CURDESC after the descriptor setup and the flush_cache() calls,
so it is programmed only once the ring it refers to is complete in
memory.

The channel is halted for the whole of that window, so the old order
reads no half-built descriptor either.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index 2c97e9576c9..45112a42758 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -179,9 +179,6 @@ static int axi_mrmac_start(struct udevice *dev)
 	/* Disable all Rx interrupts before RxBD space setup */
 	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
 
-	/* Update current descriptor */
-	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
-
 	/*
 	 * Setup Rx BDs as a closed ring: every descriptor points at the next
 	 * one and the last one wraps back to the first, each with its own
@@ -213,6 +210,9 @@ static int axi_mrmac_start(struct udevice *dev)
 	 */
 	flush_cache((phys_addr_t)priv->rx_buf, RX_BUFF_TOTAL_SIZE);
 
+	/* Update current descriptor once the ring is built and flushed */
+	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
+
 	/* Start the hardware */
 	setbits_le32(&priv->s2mm_cmn->control, XMCDMA_CR_RUNSTOP_MASK);
 	setbits_le32(&priv->mm2s_cmn->control, XMCDMA_CR_RUNSTOP_MASK);
-- 
2.34.1

[PATCH 7/8] net: mrmac: fix Rx packet loss on back-to-back frames

[Padmarao Begari]

The Rx path keeps no record of which descriptor holds the packet handed
to the network stack. recv() always starts at rx_bd[0], falls back to
rx_bd[1] only when rx_bd[0] carries no length, and then clears the
status of both descriptors:

	priv->rx_bd[0].status = 0;
	priv->rx_bd[1].status = 0;

The status field is the only record that a frame arrived, since it
holds both the COMPLETE bit and the length. free_pkt() then re-arms the
whole ring and lets hardware reuse every buffer.

So when two frames arrive back to back, recv() returns the one in
rx_bd[0] and wipes the status of rx_bd[1] in the same call. The second
frame still sits in memory, but nothing is left to show it is there.
free_pkt() hands its buffer back to hardware and it never reaches the
network stack. Nothing reports an error.

This shows up with multiple boards on a switch. The switch sends extra
traffic to the board, so packets often arrive back to back and one of
them is dropped. Network transfers then time out.

Completion also comes from the channel interrupt status register, which
only reports that a packet arrived on the channel, not which descriptor
holds it. That signal cannot drive a ring of more than one descriptor.

Track the descriptor to consume next in rx_bd_idx and touch only that
one:

 - recv() reads rx_bd[rx_bd_idx] and takes completion from the
   per-descriptor COMPLETE bit, so the descriptor is unambiguous. It
   leaves the status alone, keeping the descriptor owned by the driver
   while the network stack reads the buffer.

 - free_pkt() clears the status, restores the buffer length, flushes
   the descriptor and extends the tail pointer to it, then steps
   rx_bd_idx. Only the descriptor whose packet the stack has finished
   with goes back to hardware.

Clearing the status after delivery rather than before it means an
unread frame in the next descriptor keeps both its data and its
COMPLETE bit, and the following recv() returns it.

isrxready() loses its only caller once completion comes from the
descriptor, so drop it.

A completed descriptor with no length, or one flagged with an error,
holds no frame for the network stack. Report an empty packet for it, so
that the caller recycles the descriptor through axi_mrmac_free_pkt().

Fixes: 258ce79cfce4 ("net: xilinx: axi_mrmac: Add MRMAC driver")
Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.c | 124 +++++++++++++++------------------
 drivers/net/xilinx_axi_mrmac.h |   1 +
 2 files changed, 57 insertions(+), 68 deletions(-)

diff --git a/drivers/net/xilinx_axi_mrmac.c b/drivers/net/xilinx_axi_mrmac.c
index 45112a42758..e1531a4b548 100644
--- a/drivers/net/xilinx_axi_mrmac.c
+++ b/drivers/net/xilinx_axi_mrmac.c
@@ -202,6 +202,8 @@ static int axi_mrmac_start(struct udevice *dev)
 		bd->cntrl = PKTSIZE_ALIGN;
 	}
 
+	priv->rx_bd_idx = 0;
+
 	/* Flush the BDs so DMA core could see the updates */
 	flush_cache((phys_addr_t)priv->rx_bd, RX_BD_TOTAL_SIZE);
 
@@ -326,26 +328,6 @@ static int axi_mrmac_send(struct udevice *dev, void *ptr, int len)
 	return 0;
 }
 
-static bool isrxready(struct axi_mrmac_priv *priv)
-{
-	u32 status;
-
-	/* Read pending interrupts */
-	status = readl(&priv->mcdma_rx->status);
-
-	/* Acknowledge pending interrupts */
-	writel(status & XMCDMA_IRQ_ALL_MASK, &priv->mcdma_rx->status);
-
-	/*
-	 * If Reception done interrupt is asserted, call Rx call back function
-	 * to handle the processed BDs and then raise the according flag.
-	 */
-	if (status & (XMCDMA_IRQ_IOC_MASK | XMCDMA_IRQ_DELAY_MASK))
-		return 1;
-
-	return 0;
-}
-
 /**
  * axi_mrmac_recv - MRMAC Rx function
  * @dev:	udevice structure
@@ -354,47 +336,62 @@ static bool isrxready(struct axi_mrmac_priv *priv)
  *
  * Return:	received data length on success, negative value on errors
  *
- * This is a Rx function of MRMAC. Check if any data is received on MCDMA.
- * Copy buffer pointer to packetp and return received data length.
+ * This is a Rx function of MRMAC. Check whether the descriptor that
+ * rx_bd_idx points at has been completed by the DMA engine, and if so copy
+ * its buffer pointer to packetp and return the received data length. The
+ * descriptor stays owned by the driver until axi_mrmac_free_pkt() gives it
+ * back to hardware.
+ *
+ * A completed descriptor with no length, or one flagged with an error, holds
+ * no frame for the network stack. Report an empty packet for it, so that the
+ * caller recycles the descriptor through axi_mrmac_free_pkt().
  */
 static int axi_mrmac_recv(struct udevice *dev, int flags, uchar **packetp)
 {
 	struct axi_mrmac_priv *priv = dev_get_priv(dev);
-	u32 rx_bd_end;
+	struct mcdma_bd *bd;
+	uchar *buf;
 	u32 length;
 
+	bd = &priv->rx_bd[priv->rx_bd_idx];
+
+	/* Invalidate the descriptor to see the status written by DMA */
+	invalidate_dcache_range((phys_addr_t)bd,
+				(phys_addr_t)bd + roundup(sizeof(*bd),
+							  ARCH_DMA_MINALIGN));
+
 	/* Wait for an incoming packet */
-	if (!isrxready(priv))
+	if (!(bd->status & XMCDMA_BD_STS_COMPLETE))
 		return -EAGAIN;
 
-	/* Clear all interrupts */
-	writel(XMCDMA_IRQ_ALL_MASK, &priv->mcdma_rx->status);
-
-	/* Disable IRQ for a moment till packet is handled */
-	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
+	/*
+	 * A completed descriptor with an error, or with no data in it, holds
+	 * no frame to pass up. Report an empty packet so that the caller
+	 * recycles the descriptor through free_pkt() and moves on.
+	 */
+	if (bd->status & XMCDMA_BD_STS_ALL_ERR) {
+		*packetp = NULL;
+		return 0;
+	}
 
-	/* Disable channel fetch */
-	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
+	length = bd->status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
+	if (!length) {
+		*packetp = NULL;
+		return 0;
+	}
 
-	rx_bd_end = (ulong)priv->rx_bd + roundup(RX_BD_TOTAL_SIZE,
-						 ARCH_DMA_MINALIGN);
-	/* Invalidate Rx descriptors to see proper Rx length */
-	invalidate_dcache_range((phys_addr_t)priv->rx_bd, rx_bd_end);
+	buf = (uchar *)(ulong)(((u64)bd->buf_addr_msb << 32) | bd->buf_addr);
 
-	length = priv->rx_bd[0].status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
-	*packetp = (uchar *)(ulong)priv->rx_bd[0].buf_addr;
+	/* Invalidate the buffer before the network stack reads it */
+	invalidate_dcache_range((phys_addr_t)buf,
+				(phys_addr_t)buf + roundup(PKTSIZE_ALIGN,
+							   ARCH_DMA_MINALIGN));
 
-	if (!length) {
-		length = priv->rx_bd[1].status & XMCDMA_BD_STS_ACTUAL_LEN_MASK;
-		*packetp = (uchar *)(ulong)priv->rx_bd[1].buf_addr;
-	}
+	*packetp = buf;
 
 #ifdef DEBUG
 	print_buffer(*packetp, *packetp, 1, length, 16);
 #endif
-	/* Clear status */
-	priv->rx_bd[0].status = 0;
-	priv->rx_bd[1].status = 0;
 
 	return length;
 }
@@ -407,43 +404,34 @@ static int axi_mrmac_recv(struct udevice *dev, int flags, uchar **packetp)
  *
  * Return:	0 on success, negative value on errors
  *
- * This is Rx free packet function of MRMAC. Prepare MRMAC for reception of
- * data again. Invalidate previous data from Rx buffers and set Rx buffer
- * descriptors. Trigger reception by updating tail descriptor.
+ * This is Rx free packet function of MRMAC. The caller is done with the
+ * descriptor that axi_mrmac_recv() looked at, so give that one descriptor
+ * back to hardware by extending the tail pointer to it. The channel is
+ * never stopped, so nothing else has to be touched: no halt, no current
+ * descriptor rewrite and no re-arming of the whole ring.
  */
 static int axi_mrmac_free_pkt(struct udevice *dev, uchar *packet, int length)
 {
 	struct axi_mrmac_priv *priv = dev_get_priv(dev);
+	struct mcdma_bd *bd = &priv->rx_bd[priv->rx_bd_idx];
 
 #ifdef DEBUG
 	/* It is useful to clear buffer to be sure that it is consistent */
 	memset(priv->rx_buf, 0, RX_BUFF_TOTAL_SIZE);
 #endif
-	/* Disable all Rx interrupts before RxBD space setup */
-	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
-
-	/* Disable channel fetch */
-	clrbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
-
-	/* Update current descriptor */
-	axi_mrmac_dma_write(&priv->rx_bd[0], &priv->mcdma_rx->current);
-
-	/* Write bd to HW */
-	flush_cache((phys_addr_t)priv->rx_bd, RX_BD_TOTAL_SIZE);
-
-	/* It is necessary to flush rx buffers because if you don't do it
-	 * then cache will contain previous packet
+	/*
+	 * Clear the status written by the DMA engine, restore the buffer
+	 * length, flush the descriptor and extend the tail pointer to it so
+	 * the engine may reuse this slot.
 	 */
-	flush_cache((phys_addr_t)priv->rx_buf, RX_BUFF_TOTAL_SIZE);
+	bd->status = 0;
+	bd->cntrl = PKTSIZE_ALIGN;
 
-	/* Enable all IRQ */
-	setbits_le32(&priv->mcdma_rx->control, XMCDMA_IRQ_ALL_MASK);
+	flush_cache((phys_addr_t)bd, roundup(sizeof(*bd), ARCH_DMA_MINALIGN));
 
-	/* Channel fetch */
-	setbits_le32(&priv->mcdma_rx->control, XMCDMA_CR_RUNSTOP_MASK);
+	axi_mrmac_dma_write(bd, &priv->mcdma_rx->tail);
 
-	/* Update tail descriptor. Now it's ready to receive data */
-	axi_mrmac_dma_write(&priv->rx_bd[1], &priv->mcdma_rx->tail);
+	priv->rx_bd_idx = (priv->rx_bd_idx + 1) % RX_DESC;
 
 	log_debug("Rx completed, framelength = %x\n", length);
 
diff --git a/drivers/net/xilinx_axi_mrmac.h b/drivers/net/xilinx_axi_mrmac.h
index baf936f6b66..2c5c83421e5 100644
--- a/drivers/net/xilinx_axi_mrmac.h
+++ b/drivers/net/xilinx_axi_mrmac.h
@@ -38,6 +38,7 @@ struct axi_mrmac_priv {
 	struct mcdma_bd *rx_bd;	/* Base of the contiguous Rx BD ring */
 	u8 *txminframe;		/* Pointer to hold min length Tx frame(60) */
 	u8 *rx_buf;		/* Driver-owned RX buffer pool (RX_DESC * PKTSIZE_ALIGN) */
+	u32 rx_bd_idx;		/* Next Rx descriptor to consume: 0 .. RX_DESC-1 */
 	u32 mrmac_rate;		/* Speed to configure(Read from DT 10G/25G..) */
 };
 
-- 
2.34.1

[PATCH 8/8] net: mrmac: increase the Rx BD ring

[Padmarao Begari]

With only two Rx descriptors the ring is full as soon as two frames
arrive before the network stack drains them, and any further frame is
dropped by hardware until a descriptor is freed.

eth_rx() processes up to ETH_PACKETS_BATCH_RECV packets in one call, so
size the Rx ring from that constant. A full call can then be served
from the ring without hardware running out of descriptors, and the ring
depth follows if that constant ever changes.

Add static_assert() for the hardware minimum of two descriptors per
direction, so a smaller value fails the build instead of silently
making MRMAC drop packets.

This is headroom rather than a speed up. A TFTP transfer sends one
block at a time and waits for the ACK, so it keeps at most one packet
in flight and does not benefit from the deeper ring.

Signed-off-by: Padmarao Begari <padmarao.begari@amd.com>
---
 drivers/net/xilinx_axi_mrmac.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xilinx_axi_mrmac.h b/drivers/net/xilinx_axi_mrmac.h
index 2c5c83421e5..29f7c440238 100644
--- a/drivers/net/xilinx_axi_mrmac.h
+++ b/drivers/net/xilinx_axi_mrmac.h
@@ -11,14 +11,26 @@
 #ifndef __XILINX_AXI_MRMAC_H
 #define __XILINX_AXI_MRMAC_H
 
+#include <net.h>
+#include <linux/build_bug.h>
+
 #define MIN_PKT_SIZE	60
 
 /* MRMAC needs atleast two buffer descriptors for Tx/Rx to work.
  * Otherwise MRMAC will drop the packets. So, have atleast two Tx and
  * two Rx bd's.
+ *
+ * Tx keeps the minimum because send() waits for the transfer to
+ * complete, so a deeper Tx ring would never hold more than one frame.
+ * Rx matches the number of packets eth_rx() retires in one call, so a
+ * full batch can be taken from the ring without hardware dropping a
+ * frame for want of a free descriptor.
  */
 #define TX_DESC		2
-#define RX_DESC		2
+#define RX_DESC		ETH_PACKETS_BATCH_RECV
+
+static_assert(TX_DESC >= 2, "MRMAC needs at least two Tx descriptors");
+static_assert(RX_DESC >= 2, "MRMAC needs at least two Rx descriptors");
 
 /* MRMAC platform data structure */
 struct axi_mrmac_plat {
-- 
2.34.1

Re: [PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames

[Peter Robinson]

On Thu, 20 Aug 2026 at 10:26, Padmarao Begari <padmarao.begari@amd.com> wrote:
>
> The MRMAC Rx path drops frames when two arrive back to back. recv()
> always starts at rx_bd[0] and clears the status of both descriptors,
> and the status field is the only record that a frame arrived, so the
> second frame is lost without any error being reported. This shows up
> with multiple boards on a switch, where the extra traffic makes
> back-to-back arrivals common and network transfers time out.

Out of interest is this using the legacy IP or LWIP stack?

> Fixing that needs an Rx ring the driver can index, so the series first
> makes the ring scalable and then fixes the bug:
>
>   1-3  Independent cleanups: check memalign() failures, give the
>        driver its own Rx buffer pool instead of borrowing the shared
>        net_rx_packets[], and read the link speed from the standard
>        max-speed property.
>
>   4-6  Make the descriptor ring scale: index the contiguous BD blocks
>        directly, build the Rx chain in a loop over RX_DESC, and
>        program CURDESC only once the ring is complete in memory.
>
>   7    Track the descriptor to consume next in rx_bd_idx and take
>        completion from the per-descriptor COMPLETE bit, so a
>        descriptor goes back to hardware only after the network stack
>        has read it.
>
>   8    Size the Rx ring from ETH_PACKETS_BATCH_RECV, so a full
>        eth_rx() call can be served without hardware running out of
>        descriptors.
>
> Padmarao Begari (8):
>   net: mrmac: check memalign() return values
>   net: mrmac: use a driver-owned RX buffer pool
>   net: mrmac: switch to max-speed property
>   net: mrmac: use contiguous BD arrays
>   net: mrmac: initialize the Rx BD ring in a loop
>   net: mrmac: write CURDESC after ring setup
>   net: mrmac: fix Rx packet loss on back-to-back frames
>   net: mrmac: increase the Rx BD ring
>
>  drivers/net/xilinx_axi_mrmac.c | 235 +++++++++++++++++----------------
>  drivers/net/xilinx_axi_mrmac.h |  20 ++-
>  2 files changed, 137 insertions(+), 118 deletions(-)
>
> --
> 2.34.1
>

RE: [PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames

[Begari, Padmarao]

Public

Hi Peter,

> From: Peter Robinson <pbrobinson@gmail.com>
> Sent: Thursday, August 20, 2026 3:59 PM
> To: Begari, Padmarao <Padmarao.Begari@amd.com>
> Cc: u-boot@lists.u-boot-project.org; Simek, Michal <michal.simek@amd.com>; git
> (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames
>
> On Thu, 20 Aug 2026 at 10:26, Padmarao Begari <padmarao.begari@amd.com>
> wrote:
> >
> > The MRMAC Rx path drops frames when two arrive back to back. recv()
> > always starts at rx_bd[0] and clears the status of both descriptors,
> > and the status field is the only record that a frame arrived, so the
> > second frame is lost without any error being reported. This shows up
> > with multiple boards on a switch, where the extra traffic makes
> > back-to-back arrivals common and network transfers time out.
>
> Out of interest is this using the legacy IP or LWIP stack?

Presently using LWIP but we tested it on both.

Regards
Padmarao
>
> > Fixing that needs an Rx ring the driver can index, so the series first
> > makes the ring scalable and then fixes the bug:
> >
> >   1-3  Independent cleanups: check memalign() failures, give the
> >        driver its own Rx buffer pool instead of borrowing the shared
> >        net_rx_packets[], and read the link speed from the standard
> >        max-speed property.
> >
> >   4-6  Make the descriptor ring scale: index the contiguous BD blocks
> >        directly, build the Rx chain in a loop over RX_DESC, and
> >        program CURDESC only once the ring is complete in memory.
> >
> >   7    Track the descriptor to consume next in rx_bd_idx and take
> >        completion from the per-descriptor COMPLETE bit, so a
> >        descriptor goes back to hardware only after the network stack
> >        has read it.
> >
> >   8    Size the Rx ring from ETH_PACKETS_BATCH_RECV, so a full
> >        eth_rx() call can be served without hardware running out of
> >        descriptors.
> >
> > Padmarao Begari (8):
> >   net: mrmac: check memalign() return values
> >   net: mrmac: use a driver-owned RX buffer pool
> >   net: mrmac: switch to max-speed property
> >   net: mrmac: use contiguous BD arrays
> >   net: mrmac: initialize the Rx BD ring in a loop
> >   net: mrmac: write CURDESC after ring setup
> >   net: mrmac: fix Rx packet loss on back-to-back frames
> >   net: mrmac: increase the Rx BD ring
> >
> >  drivers/net/xilinx_axi_mrmac.c | 235
> > +++++++++++++++++----------------  drivers/net/xilinx_axi_mrmac.h |
> > 20 ++-
> >  2 files changed, 137 insertions(+), 118 deletions(-)
> >
> > --
> > 2.34.1
> >

Re: [PATCH 0/8] net: mrmac: Fix Rx packet loss on back-to-back frames

[Michal Simek]

On 8/20/26 11:25, Padmarao Begari wrote:
> The MRMAC Rx path drops frames when two arrive back to back. recv()
> always starts at rx_bd[0] and clears the status of both descriptors,
> and the status field is the only record that a frame arrived, so the
> second frame is lost without any error being reported. This shows up
> with multiple boards on a switch, where the extra traffic makes
> back-to-back arrivals common and network transfers time out.
> 
> Fixing that needs an Rx ring the driver can index, so the series first
> makes the ring scalable and then fixes the bug:
> 
>    1-3  Independent cleanups: check memalign() failures, give the
>         driver its own Rx buffer pool instead of borrowing the shared
>         net_rx_packets[], and read the link speed from the standard
>         max-speed property.
> 
>    4-6  Make the descriptor ring scale: index the contiguous BD blocks
>         directly, build the Rx chain in a loop over RX_DESC, and
>         program CURDESC only once the ring is complete in memory.
> 
>    7    Track the descriptor to consume next in rx_bd_idx and take
>         completion from the per-descriptor COMPLETE bit, so a
>         descriptor goes back to hardware only after the network stack
>         has read it.
> 
>    8    Size the Rx ring from ETH_PACKETS_BATCH_RECV, so a full
>         eth_rx() call can be served without hardware running out of
>         descriptors.
> 
> Padmarao Begari (8):
>    net: mrmac: check memalign() return values
>    net: mrmac: use a driver-owned RX buffer pool
>    net: mrmac: switch to max-speed property
>    net: mrmac: use contiguous BD arrays
>    net: mrmac: initialize the Rx BD ring in a loop
>    net: mrmac: write CURDESC after ring setup
>    net: mrmac: fix Rx packet loss on back-to-back frames
>    net: mrmac: increase the Rx BD ring
> 
>   drivers/net/xilinx_axi_mrmac.c | 235 +++++++++++++++++----------------
>   drivers/net/xilinx_axi_mrmac.h |  20 ++-
>   2 files changed, 137 insertions(+), 118 deletions(-)
> 

Applied.
M