Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
Hmm good question. I went with flush because that's what's done in the transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
Regards, Stefan
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@gmail.com] Gesendet: Donnerstag, 13. Dezember 2018 14:13 An: Stefan Theil Cc: U-Boot Mailing List Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
On Thu, Dec 13, 2018 at 6:18 PM Stefan Theil stefan.theil@mixed-mode.de wrote:
The cache was only flushed before *transmitting* packets, but not when receiving them, leading to an issue where new packets were handed to the receive handler with old contents in cache. This only happens when a lot of packets are received without sending packages every now and then.
Signed-off-by: Stefan Theil stefan.theil@mixed-mode.de
drivers/net/zynq_gem.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index 9bd79b198a..5825b6dd99 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -621,6 +621,9 @@ static int zynq_gem_recv(struct udevice *dev, int flags, uchar **packetp)
*packetp = (uchar *)(uintptr_t)addr;
flush_dcache_range(addr, addr + roundup(PKTSIZE_ALIGN,- ARCH_DMA_MINALIGN));
Shouldn't it be invalidate_dcache_range()?
barrier();return frame_len;}
Regards, Bin
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil Stefan.Theil@mixed-mode.de wrote:
Hmm good question. I went with flush because that's what's done in the transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
Regards, Bin
On 13. 12. 18 14:37, Bin Meng wrote:
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil Stefan.Theil@mixed-mode.de wrote:
Hmm good question. I went with flush because that's what's done in the transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
+1 yes. Please retest with invalidate.
Thanks, Michal
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@gmail.com] Gesendet: Donnerstag, 13. Dezember 2018 14:37 An: Stefan Theil Cc: U-Boot Mailing List Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed- mode.de> wrote:
Hmm good question. I went with flush because that's what's done in the
transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
addr
- size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I
have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
Regards, Bin
Then why are all receive buffers flushed before sending a packet? In any case, I'll try it with invalidate and submit an updated version.
Regards, Stefan
On 13. 12. 18 14:41, Stefan Theil wrote:
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@gmail.com] Gesendet: Donnerstag, 13. Dezember 2018 14:37 An: Stefan Theil Cc: U-Boot Mailing List Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed- mode.de> wrote:
Hmm good question. I went with flush because that's what's done in the
transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
addr
- size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I
have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
Regards, Bin
Then why are all receive buffers flushed before sending a packet? In any case, I'll try it with invalidate and submit an updated version.
When you creating packet, cpu is used and caches are filled with data which you are asking by DMA to send. That's why you need to flush them to DDR for dma to take it. If you don't do that DMA can work with incorrect data.
On RX path if cpu touches that memory space before DMA receive data to the memory, cpu can work with incorrect data. That's why you need to invalidate that range to ensure that data which you will read is that one got from DMA.
It is kind of interesting that we didn't observe this issue.
Thanks, Michal
-----Ursprüngliche Nachricht----- Von: Michal Simek [mailto:monstr@monstr.eu] Gesendet: Donnerstag, 13. Dezember 2018 14:48 An: Stefan Theil; Bin Meng Cc: U-Boot Mailing List Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
On 13. 12. 18 14:41, Stefan Theil wrote:
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@gmail.com] Gesendet: Donnerstag, 13. Dezember 2018 14:37 An: Stefan Theil Cc: U-Boot Mailing List Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed- mode.de> wrote:
Hmm good question. I went with flush because that's what's done in the
transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
addr
- size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I
have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
Regards, Bin
Then why are all receive buffers flushed before sending a packet? In any
case, I'll try it with invalidate and submit an updated version.
When you creating packet, cpu is used and caches are filled with data which you are asking by DMA to send. That's why you need to flush them to DDR for dma to take it. If you don't do that DMA can work with incorrect data.
I know, but you don't create *receive* packets when you're sending a packet, are you? Why would you need to flush those before sending a packet? That's what I'm wondering about.
On RX path if cpu touches that memory space before DMA receive data to the memory, cpu can work with incorrect data. That's why you need to invalidate that range to ensure that data which you will read is that one got from DMA.
It is kind of interesting that we didn't observe this issue.
Probably because most people don't receive enough packets between sending packets, so the receive buffers are flushed often enough.
Thanks, Michal
-- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Xilinx Microblaze Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
Hi Stefan,
On Thu, Dec 13, 2018 at 9:51 PM Stefan Theil Stefan.Theil@mixed-mode.de wrote:
-----Ursprüngliche Nachricht----- Von: Michal Simek [mailto:monstr@monstr.eu] Gesendet: Donnerstag, 13. Dezember 2018 14:48 An: Stefan Theil; Bin Meng Cc: U-Boot Mailing List Betreff: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
On 13. 12. 18 14:41, Stefan Theil wrote:
-----Ursprüngliche Nachricht----- Von: Bin Meng [mailto:bmeng.cn@gmail.com] Gesendet: Donnerstag, 13. Dezember 2018 14:37 An: Stefan Theil Cc: U-Boot Mailing List Betreff: Re: Re: [U-Boot] [PATCH] zynq-gem: Flush cache before handing RX packet to receive handler
Hi Stefan,
On Thu, Dec 13, 2018 at 9:26 PM Stefan Theil <Stefan.Theil@mixed- mode.de> wrote:
Hmm good question. I went with flush because that's what's done in the
transmit function:
addr = (ulong) ptr; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup(len, ARCH_DMA_MINALIGN); flush_dcache_range(addr,
addr
- size);
addr = (ulong)priv->rxbuffers; addr &= ~(ARCH_DMA_MINALIGN - 1); size = roundup((RX_BUF * PKTSIZE_ALIGN), ARCH_DMA_MINALIGN); flush_dcache_range(addr, addr + size); barrier();
But since we actually want the uncached data invalidation seems logical. I
have to admit though, I don't have much experience with caches. This patch completely fixed my problem... Maybe somebody with a bit more expertise can add their opinion?
It should be 'invalidate' primitive when it comes to the RX path. For TX path, it should be 'flush'.
Regards, Bin
Then why are all receive buffers flushed before sending a packet? In any
case, I'll try it with invalidate and submit an updated version.
When you creating packet, cpu is used and caches are filled with data which you are asking by DMA to send. That's why you need to flush them to DDR for dma to take it. If you don't do that DMA can work with incorrect data.
I know, but you don't create *receive* packets when you're sending a packet, are you? Why would you need to flush those before sending a packet? That's what I'm wondering about.
Why did you mention *receive* packets and sending a packet? The receive packets and send packets are two different buffers.
On RX path if cpu touches that memory space before DMA receive data to the memory, cpu can work with incorrect data. That's why you need to invalidate that range to ensure that data which you will read is that one got from DMA.
It is kind of interesting that we didn't observe this issue.
Probably because most people don't receive enough packets between sending packets, so the receive buffers are flushed often enough.
Regards, Bin
participants (3)
-
Bin Meng -
Michal Simek -
Stefan Theil