Skip to contentSkip to conversation

usb: dwc2: invalidate the dcache before starting the DMA

PATCH Superseded Eddie Cai 3 participants 6 messages Apr 6, 2017 – Apr 18, 2017 USB

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Eddie Cai

author
#0 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
 drivers/usb/host/dwc2.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +13 -6
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
 	       (*pid << DWC2_HCTSIZ_PID_OFFSET),
 	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if (in) {
+			invalidate_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		} else {
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
 	}
 
 	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);
Signature · 1 line
2.10.2
#1 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

Quoted 21 lines · Eddie Cai

We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
 drivers/usb/host/dwc2.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +0 -1
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
 	       (*pid << DWC2_HCTSIZ_PID_OFFSET),
 	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer, · line 818

Quoted 10 lines
drivers/usb/host/dwc2.c +5 -5
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if (in) {
+			invalidate_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +

Should be uintptr_t all over the place to deal with 32/64 bit systems, otherwise great, thanks.

Quoted 12 lines
drivers/usb/host/dwc2.c +8 -0
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		} else {
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
 	}
 
 	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);
Signature · 2 lines
Best regards,
Marek Vasut
#2 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

Hi Eddie,

Quoted 45 lines · Marek Vasut
Quoted 21 lines · Eddie Cai

We should invalidate the dcache before starting the DMA. In case there are any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on. Cache-line replacement can happen if the CPU tries to bring some other memory locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
  drivers/usb/host/dwc2.c | 19 +++++++++++++------
  1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +0 -1
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer,
  	       (*pid << DWC2_HCTSIZ_PID_OFFSET),
  	       &hc_regs->hctsiz);
 
-	if (!in && xfer_len) {

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer, · line 818

Quoted 10 lines
drivers/usb/host/dwc2.c +5 -5
-		memcpy(aligned_buffer, buffer, xfer_len);
-
-		flush_dcache_range((unsigned long)aligned_buffer,
-				   (unsigned long)aligned_buffer +
-				   roundup(xfer_len, ARCH_DMA_MINALIGN));
+	if (xfer_len) {
+		if (in) {
+			invalidate_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +

Should be uintptr_t all over the place to deal with 32/64 bit systems, otherwise great, thanks.

Quoted 10 lines
drivers/usb/host/dwc2.c +8 -0
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		} else {
+			memcpy(aligned_buffer, buffer, xfer_len);
+			flush_dcache_range(
+					(unsigned long)aligned_buffer,
+					(unsigned long)aligned_buffer +
+					roundup(xfer_len, ARCH_DMA_MINALIGN));
+		}
  	}
Quoted 2 lines

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer, · line 829

Quoted 2 lines
drivers/usb/host/dwc2.c +0 -0
  	writel(phys_to_bus((unsigned long)aligned_buffer), &hc_regs->hcdma);

Did you test with remove another invalidate_dcache_range() in this function? I still think no need to have 2 invalidate_dcache in this function.

Thanks,
- Kever

Quoted 1 line
#3 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

Quoted 55 lines · Kever Yang

Hi Eddie,

Quoted 29 lines · Marek Vasut
Quoted 28 lines · Eddie Cai

We should invalidate the dcache before starting the DMA. In case there are
any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on.
Cache-line replacement can happen if the CPU tries to bring some other memory
locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
  drivers/usb/host/dwc2.c | 19 +++++++++++++------
  1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +0 -4
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs
 *hc_regs, void *aligned_buffer,
             (*pid << DWC2_HCTSIZ_PID_OFFSET),
             &hc_regs->hctsiz);
  -    if (!in && xfer_len) {
-        memcpy(aligned_buffer, buffer, xfer_len);
-
-        flush_dcache_range((unsigned long)aligned_buffer,
-                   (unsigned long)aligned_buffer +
Quoted 21 lines

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs *hc_regs, void *aligned_buffer, · line 818

Quoted 6 lines
drivers/usb/host/dwc2.c +5 -1
-                   roundup(xfer_len, ARCH_DMA_MINALIGN));
+    if (xfer_len) {
+        if (in) {
+            invalidate_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +

Should be uintptr_t all over the place to deal with 32/64 bit systems, otherwise great, thanks.

Quoted 12 lines
drivers/usb/host/dwc2.c +8 -0
+                    roundup(xfer_len, ARCH_DMA_MINALIGN));
+        } else {
+            memcpy(aligned_buffer, buffer, xfer_len);
+            flush_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +
+                    roundup(xfer_len, ARCH_DMA_MINALIGN));
+        }
      }
        writel(phys_to_bus((unsigned long)aligned_buffer),

&hc_regs->hcdma);

Quoted 3 lines

Did you test with remove another invalidate_dcache_range() in this function?
I still think no need to have 2 invalidate_dcache in this function.

You need the first invalidate to prevent having data stuck in the cache and accidentally being evicted into the RAM while the USB DMA is also running , thus creating a total mess of the result.

Signature · 2 lines
Best regards,
Marek Vasut
#4 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

Hi Marek,

Quoted 55 lines · Marek Vasut
Quoted 54 lines · Kever Yang

Hi Eddie,

Quoted 50 lines · Marek Vasut
Quoted 24 lines · Eddie Cai

We should invalidate the dcache before starting the DMA. In case there are
any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on.
Cache-line replacement can happen if the CPU tries to bring some other memory
locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
   drivers/usb/host/dwc2.c | 19 +++++++++++++------
   1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +0 -0
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs
 *hc_regs, void *aligned_buffer,
              (*pid << DWC2_HCTSIZ_PID_OFFSET),
              &hc_regs->hctsiz);
   -    if (!in && xfer_len) {

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs · line 819

Quoted 10 lines
drivers/usb/host/dwc2.c +5 -5
-        memcpy(aligned_buffer, buffer, xfer_len);
-
-        flush_dcache_range((unsigned long)aligned_buffer,
-                   (unsigned long)aligned_buffer +
-                   roundup(xfer_len, ARCH_DMA_MINALIGN));
+    if (xfer_len) {
+        if (in) {
+            invalidate_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +

Should be uintptr_t all over the place to deal with 32/64 bit systems, otherwise great, thanks.

Quoted 12 lines
drivers/usb/host/dwc2.c +8 -0
+                    roundup(xfer_len, ARCH_DMA_MINALIGN));
+        } else {
+            memcpy(aligned_buffer, buffer, xfer_len);
+            flush_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +
+                    roundup(xfer_len, ARCH_DMA_MINALIGN));
+        }
       }
         writel(phys_to_bus((unsigned long)aligned_buffer),

&hc_regs->hcdma);

Quoted 8 lines
Quoted 3 lines

Did you test with remove another invalidate_dcache_range() in this function?
I still think no need to have 2 invalidate_dcache in this function.

You need the first invalidate to prevent having data stuck in the cache and accidentally being evicted into the RAM while the USB DMA is also running , thus creating a total mess of the result.

Yes, I'm very clear about what happen here, because it's me report the origin bug.
What I mean is the fix should be _move_ the invalidate_dcache_range() operation
from "after start DMA transfer" to "before DMA transter" for in transfer, but not
_add_ a invalidate_dcache_range() operation.

If you apply this patch, then there will be two invalidate_dcache_range() operation
for in transfer, and the second one is no need, because CPU should not have any
access to the buffer during dma start and done.

Thanks,
- Kever

author
#5 lore raw

PATCH v2 1/1 usb: dwc2: invalidate the dcache before starting the DMA

Hi

2017-04-18 11:55 GMT+08:00 Kever Yang <[email protected]>:

Quoted 74 lines

Hi Marek,

Quoted 59 lines · Marek Vasut
Quoted 57 lines · Kever Yang

Hi Eddie,

Quoted 52 lines · Marek Vasut
Quoted 25 lines · Eddie Cai

We should invalidate the dcache before starting the DMA. In case there are
any dirty lines from the DMA buffer in the cache, subsequent cache-line replacements may corrupt the buffer in memory while the DMA is still going on.
Cache-line replacement can happen if the CPU tries to bring some other memory
locations into the cache while the DMA is going on.

Signed-off-by Eddie Cai <[email protected]>
Reviewed-by Stefan Brüns <[email protected]>
---
   drivers/usb/host/dwc2.c | 19 +++++++++++++------
   1 file changed, 13 insertions(+), 6 deletions(-)
drivers/usb/host/dwc2.c +0 -1
index 5ac602e..e3a5d7d 100644
@@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs
 *hc_regs, void *aligned_buffer,
              (*pid << DWC2_HCTSIZ_PID_OFFSET),
              &hc_regs->hctsiz);
   -    if (!in && xfer_len) {
-        memcpy(aligned_buffer, buffer, xfer_len);

drivers/usb/host/dwc2.c static int transfer_chunk(struct dwc2_hc_regs · line 820

Quoted 10 lines
drivers/usb/host/dwc2.c +5 -4
-
-        flush_dcache_range((unsigned long)aligned_buffer,
-                   (unsigned long)aligned_buffer +
-                   roundup(xfer_len, ARCH_DMA_MINALIGN));
+    if (xfer_len) {
+        if (in) {
+            invalidate_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +

Should be uintptr_t all over the place to deal with 32/64 bit systems, otherwise great, thanks.

+ roundup(xfer_len, ARCH_DMA_MINALIGN));

Quoted 11 lines
drivers/usb/host/dwc2.c +7 -0
+        } else {
+            memcpy(aligned_buffer, buffer, xfer_len);
+            flush_dcache_range(
+                    (unsigned long)aligned_buffer,
+                    (unsigned long)aligned_buffer +
+                    roundup(xfer_len, ARCH_DMA_MINALIGN));
+        }
       }
         writel(phys_to_bus((unsigned long)aligned_buffer),

&hc_regs->hcdma);

Quoted 10 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line

Did you test with remove another invalidate_dcache_range() in this

function?
I still think no need to have 2 invalidate_dcache in this function.

You need the first invalidate to prevent having data stuck in the cache and accidentally being evicted into the RAM while the USB DMA is also running , thus creating a total mess of the result.

Quoted 8 lines

Yes, I'm very clear about what happen here, because it's me report the origin bug.
What I mean is the fix should be _move_ the invalidate_dcache_range() operation
from "after start DMA transfer" to "before DMA transter" for in transfer, but not
_add_ a invalidate_dcache_range() operation.

I follow doc/README.arm-caches. I says
Peripheral to Memory DMA:

- Invalidate the buffer before starting the DMA. In case there are any dirty
lines from the DMA buffer in the cache, subsequent cache-line
replacements
may corrupt the buffer in memory while the DMA is still going on.
Cache-line
replacement can happen if the CPU tries to bring some other memory
locations
into the cache while the DMA is going on.

- Invalidate the buffer after the DMA is complete and before the MPU reads

it. This may be needed in addition to the invalidation before the DMA

mentioned above, because in some processors memory contents can
spontaneously
come to the cache due to speculative memory access by the CPU. If this

happens with the DMA buffer while DMA is going on we have a coherency
problem.
I guess that means we need to Invalidate the buffer before and after the DMA Thanks
Eddie

Quoted 10 lines

If you apply this patch, then there will be two invalidate_dcache_range() operation
for in transfer, and the second one is no need, because CPU should not have any
access to the buffer during dma start and done.

Thanks,
- Kever