* [PATCH] net: macb: Fix race caused by flushing unwanted descriptors
@ 2022-11-10 17:31 Yaron Micher
2022-11-28 19:52 ` Tom Rini
0 siblings, 1 reply; 2+ messages in thread
From: Yaron Micher @ 2022-11-10 17:31 UTC (permalink / raw)
To: u-boot; +Cc: joe.hershberger, rfried.dev, 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@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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] net: macb: Fix race caused by flushing unwanted descriptors
2022-11-10 17:31 [PATCH] net: macb: Fix race caused by flushing unwanted descriptors Yaron Micher
@ 2022-11-28 19:52 ` Tom Rini
0 siblings, 0 replies; 2+ messages in thread
From: Tom Rini @ 2022-11-28 19:52 UTC (permalink / raw)
To: Yaron Micher; +Cc: u-boot, joe.hershberger, rfried.dev
[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]
On Thu, Nov 10, 2022 at 07:31:34PM +0200, Yaron Micher wrote:
> 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@hailo.ai>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-28 19:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-10 17:31 [PATCH] net: macb: Fix race caused by flushing unwanted descriptors Yaron Micher
2022-11-28 19:52 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.