Hi, Marek Vasut <ma...@denx.de> writes: >>>> 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). > > Uh, the event buffer is 4k ? That's quite big, but OK.
it really isn't when you're dealing with LPM. I've seen 4k cause overflow events before. >> 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 ? haven't seen a problem in almost 6 years dealing with this IP. -- balbi _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot