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

[Marek Vasut]

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 .

>  	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 ?

> -		dwc3_thread_interrupt(0, dwc);
>  	}
>  }
> 

One last thing, is this patch needed in Linux too ?

-- 
Best regards,
Marek Vasut