[U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on event buffers more robust

[Felipe Balbi]

Hi,

Marek Vasut <marex at 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.tomsich at 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