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

[Marek Vasut]

On 04/05/2017 10:18 AM, Felipe Balbi wrote:
> 
> 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.

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 ?
> 
> haven't seen a problem in almost 6 years dealing with this IP.

:)

-- 
Best regards,
Marek Vasut