[U-Boot] [PATH V2] usb: dwc2: invalidate the dcache before starting the DMA

[Kever Yang]

Hi Marek,

On 04/06/2017 06:20 PM, Marek Vasut wrote:
> On 04/06/2017 10:34 AM, Kever Yang wrote:
>> Hi Eddie,
>>
>>
>> On 04/06/2017 10:14 AM, Marek Vasut wrote:
>>> On 04/06/2017 04:03 AM, Eddie Cai wrote:
>>>> We should invalidate the dcache before starting the DMA. In case
>>>> there are
>>>> any dirty lines from the DMA buffer in the cache, subsequent cache-line
>>>> replacements may corrupt the buffer in memory while the DMA is still
>>>> going on.
>>>> Cache-line replacement can happen if the CPU tries to bring some
>>>> other memory
>>>> locations into the cache while the DMA is going on.
>>>>
>>>> Signed-off-by: Eddie Cai <eddie.cai.linux at gmail.com>
>>>> Reviewed-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
>>>> ---
>>>>    drivers/usb/host/dwc2.c | 19 +++++++++++++------
>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
>>>> index 5ac602e..e3a5d7d 100644
>>>> --- a/drivers/usb/host/dwc2.c
>>>> +++ b/drivers/usb/host/dwc2.c
>>>> @@ -811,12 +811,19 @@ static int transfer_chunk(struct dwc2_hc_regs
>>>> *hc_regs, void *aligned_buffer,
>>>>               (*pid << DWC2_HCTSIZ_PID_OFFSET),
>>>>               &hc_regs->hctsiz);
>>>>    -    if (!in && xfer_len) {
>>>> -        memcpy(aligned_buffer, buffer, xfer_len);
>>>> -
>>>> -        flush_dcache_range((unsigned long)aligned_buffer,
>>>> -                   (unsigned long)aligned_buffer +
>>>> -                   roundup(xfer_len, ARCH_DMA_MINALIGN));
>>>> +    if (xfer_len) {
>>>> +        if (in) {
>>>> +            invalidate_dcache_range(
>>>> +                    (unsigned long)aligned_buffer,
>>>> +                    (unsigned long)aligned_buffer +
>>> Should be uintptr_t all over the place to deal with 32/64 bit systems,
>>> otherwise great, thanks.
>>>
>>>> +                    roundup(xfer_len, ARCH_DMA_MINALIGN));
>>>> +        } else {
>>>> +            memcpy(aligned_buffer, buffer, xfer_len);
>>>> +            flush_dcache_range(
>>>> +                    (unsigned long)aligned_buffer,
>>>> +                    (unsigned long)aligned_buffer +
>>>> +                    roundup(xfer_len, ARCH_DMA_MINALIGN));
>>>> +        }
>>>>        }
>>>>          writel(phys_to_bus((unsigned long)aligned_buffer),
>>>> &hc_regs->hcdma);
>>>>
>> Did you test with remove another invalidate_dcache_range() in this
>> function?
>> I still think no need to have 2 invalidate_dcache in this function.
> You need the first invalidate to prevent having data stuck in the cache
> and accidentally being evicted into the RAM while the USB DMA is also
> running , thus creating a total mess of the result.
>
>

Yes, I'm very clear about what happen here, because it's me report
the origin bug.
What I mean is the fix should be _move_ the invalidate_dcache_range() 
operation
from "after start DMA transfer" to "before DMA transter" for in 
transfer, but not
_add_ a invalidate_dcache_range() operation.

If you apply this patch, then there will be two 
invalidate_dcache_range() operation
for in transfer, and the second one is no need, because CPU should not 
have any
access to the buffer during dma start and done.

Thanks,
- Kever