> On 04 Apr 2017, at 18:15, Marek Vasut <ma...@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.toms...@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). Good point on the “long”, especially as I just copied this from other occurences and it’s consistently wrong throughout DWC3 in U-Boot: 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. >> 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 */ >> for (i = 0; i < dwc->num_event_buffers; i++) { >> evt = dwc->ev_buffs[i]; >> dwc3_flush_cache((long)evt->buf, evt->length); >> } >> - > > 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. >> - 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. Regards, Philipp. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot