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

[Dr. Philipp Tomsich]

> On 04 Apr 2017, at 18:15, Marek Vasut <marex at 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 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).
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.