Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer

[Guenter Roeck <linux@roeck-us.net>]

On 2/23/20 10:20 AM, Antti Seppälä wrote:
> On Sun, 23 Feb 2020 at 15:45, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 2/23/20 3:00 AM, Antti Seppälä wrote:
>>> Hi Guenter,
>>>
>>> On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Yes, those patches didn't address the core problem. Can you test with the
>>>> attached two patches ?
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> I took a look at your patch (usb: dwc2: Simplify DMA alignment code)
>>> and I don't believe it is correct.
>>>
>>> The patch re-introduces the dma_aligned_buffer struct and takes some
>>> care to align the beginning of the struct to dma cache lines. However
>>> we should be aligning the data[0] pointer inside the struct instead.
>>> With the code in the patch data[0] gets pushed to be at an offset from
>>> the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other
>>> words data[0] is now not aligned to dma cache boundaries.
>>>
>>
>> I thought so too initially. However,
>>
>> temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1;
>>
>> aligns the structure pointer such that its _end_ is DMA-aligned,
>> which is ->data[0].
>>
> 
> Hmm, looks like you're right. I somehow missed the - 1 at the end.
> Sorry for the noise I guess.
> 
No worries. It took me a while to understand hat code, and I initially
also thought it was wrong, so you are in good company.

> Would be nice to know what makes the previous code prone to pointer
> corruption issues though. With the added padding that pointer should
> also be on another dma cache line.
> 
Padding to DMA cache line size doesn't fix the real problem. The dwc2
IP expects input buffer size to be a multiple of wMaxPacketSize.
dwc2_hc_start_transfer() has the following code(where wMaxPacketSize ==
chan->max_packet).

		if (chan->xfer_len > 0) {
                         num_packets = (chan->xfer_len + chan->max_packet - 1) /
                                         chan->max_packet;
			...
		}
		...
		if (chan->ep_is_in)
			chan->xfer_len = num_packets * chan->max_packet;

If, for example, wMaxPacketSize is 64, and the original xfer_len is, say,
1522 (as observed in our case), xfer_len is updated to 1536, and the chip
is programmed to receive up to 1536 bytes. In most cases, padding the
buffer to the DMA cache line size (64 in our case) catches this, but not
if xfer_len is much lower than wMaxPacketSize.

My code tries to address that situation, but as Boris noticed there is
still something wrong with my fix.

Guenter