[U-Boot] [PATCH v5 02/19] usb: dwc2: Use separate input and output buffers

[Simon Glass]

Hi Stefan,

On 3 April 2017 at 12:24, Brüns, Stefan <Stefan.Bruens at rwth-aachen.de> wrote:
> On Montag, 3. April 2017 20:18:42 CEST Brüns, Stefan wrote:
>> On Montag, 3. April 2017 17:38:40 CEST you wrote:
>> >
>> > What am I missing?
>>
>> The following is a gross oversimplification, but might explain it:
>>
>> 1. Assume all of the 64kB of the aligned_buffer is dirty. (Likely, if the
>> buffer is calloced.)
>> 2. We are doing some transfers. All transfers are small, e.g. 64 byte.
>> 3. In accordance with the two cases you mentioned above, the first 64 byte
>> are no longer dirty, as the last out transfer has flushed the cacheline. 4.
>> We are doing our first large in transfer (i.e. larger than 64 byte). 5. Bad
>> Things (TM) may happen to any data at aligned_buffer[64] and beyond.
>>
>> If this holds, a single invalidate_dcache_range(aligned_buffer,
>> aligned_buffer +65536,...) during the initialization of the controller
>> would suffice.
>
> I just read "[U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on
> event buffers more robust" from Philipp Tomsich, which mentions the same
> problem:
>
> 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')

Yes that explains why the cache is dirty when the driver starts. I
hadn't thought of that. IMO changing the init sequence is a better
solution.

Regards,
Simon