[PATCH] net: macb: Fix race caused by flushing unwanted descriptors

[Yaron Micher]

The rx descriptor list is in cached memory, and there may be multiple
descriptors per cache-line. After reclaim_rx_buffers marks a descriptor
as unused it does a cache flush, which causes the entire cache-line to
be written to memory, which may override other descriptors in the same
cache-line that the controller may have written to.

The fix skips freeing descriptors that are not the last in a cache-line,
and if the freed descriptor is the last one in a cache-line, it marks
all the descriptors in the cache-line as unused.
This is similarly to what is done in drivers/net/fec_mxc.c

In my case this bug caused tftpboot to fail some times when other
packets are sent to u-boot in addition to the ongoing tftp (e.g. ping).
The driver would stop receiving new packets because it is waiting
on a descriptor that is marked unused, when in reality the descriptor
contains a new unprocessed packet but while freeing the previous buffer
descriptor & flushing the cache, the driver accidentally marked the
descriptor as unused.

Signed-off-by: Yaron Micher <yaronm at hailo.ai>
---
 drivers/net/macb.c | 51 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index e02a57b411..65ec1f24ad 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -98,6 +98,9 @@ struct macb_dma_desc_64 {
 #define MACB_RX_DMA_DESC_SIZE	(DMA_DESC_BYTES(MACB_RX_RING_SIZE))
 #define MACB_TX_DUMMY_DMA_DESC_SIZE	(DMA_DESC_BYTES(1))
 
+#define DESC_PER_CACHELINE_32	(ARCH_DMA_MINALIGN/sizeof(struct macb_dma_desc))
+#define DESC_PER_CACHELINE_64	(ARCH_DMA_MINALIGN/DMA_DESC_SIZE)
+
 #define RXBUF_FRMLEN_MASK	0x00000fff
 #define TXBUF_FRMLEN_MASK	0x000007ff
 
@@ -401,32 +404,56 @@ static int _macb_send(struct macb_device *macb, const char *name, void *packet,
 	return 0;
 }
 
+static void reclaim_rx_buffer(struct macb_device *macb,
+			      unsigned int idx)
+{
+	unsigned int mask;
+	unsigned int shift;
+	unsigned int i;
+
+	/*
+	 * There may be multiple descriptors per CPU cacheline,
+	 * so a cache flush would flush the whole line, meaning the content of other descriptors
+	 * in the cacheline would also flush. If one of the other descriptors had been
+	 * written to by the controller, the flush would cause those changes to be lost.
+	 *
+	 * To circumvent this issue, we do the actual freeing only when we need to free
+	 * the last descriptor in the current cacheline. When the current descriptor is the
+	 * last in the cacheline, we free all the descriptors that belong to that cacheline.
+	 */
+	if (macb->config->hw_dma_cap & HW_DMA_CAP_64B) {
+		mask = DESC_PER_CACHELINE_64 - 1;
+		shift = 1;
+	} else {
+		mask = DESC_PER_CACHELINE_32 - 1;
+		shift = 0;
+	}
+
+	/* we exit without freeing if idx is not the last descriptor in the cacheline */
+	if ((idx & mask) != mask)
+		return;
+
+	for (i = idx & (~mask); i <= idx; i++)
+		macb->rx_ring[i << shift].addr &= ~MACB_BIT(RX_USED);
+}
+
 static void reclaim_rx_buffers(struct macb_device *macb,
 			       unsigned int new_tail)
 {
 	unsigned int i;
-	unsigned int count;
 
 	i = macb->rx_tail;
 
 	macb_invalidate_ring_desc(macb, RX);
 	while (i > new_tail) {
-		if (macb->config->hw_dma_cap & HW_DMA_CAP_64B)
-			count = i * 2;
-		else
-			count = i;
-		macb->rx_ring[count].addr &= ~MACB_BIT(RX_USED);
+		reclaim_rx_buffer(macb, i);
 		i++;
-		if (i > MACB_RX_RING_SIZE)
+		if (i >= MACB_RX_RING_SIZE)
 			i = 0;
 	}
 
 	while (i < new_tail) {
-		if (macb->config->hw_dma_cap & HW_DMA_CAP_64B)
-			count = i * 2;
-		else
-			count = i;
-		macb->rx_ring[count].addr &= ~MACB_BIT(RX_USED);
+		reclaim_rx_buffer(macb, i);
 		i++;
 	}
 
-- 
2.17.1