On 04/04/2017 07:46 PM, Dr. Philipp Tomsich wrote:
On 04 Apr 2017, at 18:15, Marek Vasut marex@denx.de wrote:
On 04/03/2017 07:49 PM, Philipp Tomsich wrote:
Merely using dma_alloc_coherent does not ensure that there is no stale data left in the caches for the allocated DMA buffer (i.e. that the affected cacheline may still be dirty).
The original code was doing the following (on AArch64, which translates a 'flush' into a 'clean + invalidate'): # during initialisation: 1. allocate buffers via memalign => buffers may still be modified (cached, dirty) # during interrupt processing 2. clean + invalidate buffers => may commit stale data from a modified cacheline 3. read from buffers
This could lead to garbage info being written to buffers before reading them during even-processing.
To make the event processing more robust, we use the following sequence for the cache-maintenance: # during initialisation: 1. allocate buffers via memalign 2. clean + invalidate buffers (we only need the 'invalidate' part, but dwc3_flush_cache() always performs a 'clean + invalidate') # during interrupt processing 3. read the buffers (we know these lines are not cached, due to the previous invalidation and no other code touching them in-between) 4. clean + invalidate buffers => writes back any modification we may have made during event processing and ensures that the lines are not in the cache the next time we enter interrupt processing
Note that with the original sequence, we observe reproducible (depending on the cache state: i.e. running dhcp/usb start before will upset caches to get us around this) issues in the event processing (a fatal synchronous abort in dwc3_gadget_uboot_handle_interrupt on the first time interrupt handling is invoked) when running USB mass storage emulation on our RK3399-Q7 with data-caches on.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
drivers/usb/dwc3/core.c | 2 ++ drivers/usb/dwc3/gadget.c | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index b2c7eb1..f58c7ba 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -125,6 +125,8 @@ static struct dwc3_event_buffer *dwc3_alloc_one_event_buffer(struct dwc3 *dwc, if (!evt->buf) return ERR_PTR(-ENOMEM);
- dwc3_flush_cache((long)evt->buf, evt->length);
Is the length aligned ? If not, you will get cache alignment warning. Also, address should be uintptr_t to avoid 32/64 bit issues .
The length is a well-known value and aligned (it expands to PAGE_SIZE in the end).
Uh, the event buffer is 4k ? That's quite big, but OK.
Good point on the “long”, especially as I just copied this from other occurences and it’s consistently wrong throughout DWC3 in U-Boot:
Hrm, I thought the driver was ported over from Linux, so is this broken in Linux too ?
drivers/usb/dwc3/core.c: dwc3_flush_cache((long)evt->buf, evt->length); drivers/usb/dwc3/ep0.c: dwc3_flush_cache((long)buf_dma, len); drivers/usb/dwc3/ep0.c: dwc3_flush_cache((long)trb, sizeof(*trb)); drivers/usb/dwc3/ep0.c: dwc3_flush_cache((long)trb, sizeof(*trb)); drivers/usb/dwc3/ep0.c: dwc3_flush_cache((long)trb, sizeof(*trb)); drivers/usb/dwc3/ep0.c: dwc3_flush_cache((long)dwc->ep0_bounce, DWC3_EP0_BOUNCE_SIZE); drivers/usb/dwc3/gadget.c: dwc3_flush_cache((long)req->request.dma, req->request.length); drivers/usb/dwc3/gadget.c: dwc3_flush_cache((long)dma, length); drivers/usb/dwc3/gadget.c: dwc3_flush_cache((long)trb, sizeof(*trb)); drivers/usb/dwc3/gadget.c: dwc3_flush_cache((long)trb, sizeof(*trb)); drivers/usb/dwc3/gadget.c: dwc3_flush_cache((long)evt->buf, evt->length); drivers/usb/dwc3/io.h:static inline void dwc3_flush_cache(int addr, int length)
Worst of all: the definition of dwc3_flush_cache in io.h has “int” as a type, which will eat us alive if the DWC3’s physical address is beyond 32-bit.
I’ll revise all of these and make a patch-series out of this.
Maybe you should check the Linux first and see if there are some fixes already.
Thanks
return evt; }
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 1156662..61af71b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2668,11 +2668,12 @@ void dwc3_gadget_uboot_handle_interrupt(struct dwc3 *dwc) int i; struct dwc3_event_buffer *evt;
dwc3_thread_interrupt(0, dwc);/* Clean + Invalidate the buffers after touching them */
This makes me wonder, don't you need to invalidate the event buffer somewhere so that the new data would be fetched from RAM ?
We flush the event buffer before leaving the function. So the cache line will not be present in the cache, when we enter this function again.
Then shouldn't we invalidate it instead ? flush and invalidate are two different things ...
dwc3_thread_interrupt(0, dwc);}
One last thing, is this patch needed in Linux too ?
Linux deals properly with DMA allocations and manages them in appropriate memory regions (e.g. marked uncached). Also, some of the affected code-paths are U-Boot specific.
This really stems from a limitation of the way the DMA areas are allocated in U-Boot (i.e. from the heap, using a memalign) and how the cache-operations have been sequenced relative to the other code in the port to U-Boot.
OK I see, thanks for clarifying!