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

[Marek Vasut]

On 04/04/2017 07:46 PM, Dr. Philipp Tomsich wrote:
> 
>> 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).

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 */
>>> 		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.

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!

-- 
Best regards,
Marek Vasut