Re: [RFT PATCH 1/4] usb: dwc2: Simplify and fix DMA alignment code

[Doug Anderson]

Hi,

On Thu, Feb 27, 2020 at 8:28 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 2/27/20 2:27 PM, Guenter Roeck wrote:
> > On 2/27/20 2:06 PM, Doug Anderson wrote:
> [ ... ]
> >>> -       if (urb->num_sgs || urb->sg ||
> >>> -           urb->transfer_buffer_length == 0 ||
> >>> +       if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
> >>> +           (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) ||
> >>>             !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
> >>
> >> Maybe I'm misunderstanding things, but it feels like we need something
> >> more here.  Specifically I'm worried about the fact when the transfer
> >> buffer is already aligned but the length is not a multiple of the
> >> endpoint's maximum transfer size.  You need to handle that, right?
> >> AKA something like this (untested):
> >>
> >> /* Simple case of not having to allocate a bounce buffer */
> >> if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
> >>     (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
> >>   return 0;
> >>
> >> /* Can also avoid bounce buffer if alignment and size are good */
> >> maxp = usb_endpoint_maxp(&ep->desc);
> >> if (maxp == urb->transfer_buffer_length &&
> >
> > No, transfer_buffer_length would have to be a multiple of maxp. There
> > are many situations where roundup(transfer_buffer_length, maxp) !=
> > transfer_buffer_length. I agree, this would be the prudent approach
> > (and it was my original implementation), but then it didn't seem to
> > cause trouble so far, and I was hesitant to add it in because it results
> > in creating temporary buffers for almost every receive operation.
> > I'd like to get some test feedback from Boris - if the current code
> > causes crashes with his use case, we'll know that it is needed.
> > Otherwise, we'll have to decide if the current approach (with fewer
> > copies) is worth the risk, or if we want to play save and always
> > copy if roundup(transfer_buffer_length, maxp) != transfer_buffer_length.
> >
>
> Thinking more about this, the situation is actually much worse:
> In Boris' testing, he found inbound transactions requested by usb
> storage code with a requested transfer size of 13 bytes ... with
> URB_NO_TRANSFER_DMA_MAP set. This means the requesting code has
> provided a DMA ready buffer, transfer_buffer isn't even used,
> and we can not reallocate it. In this situation we can just hope
> that the chip (and the connected USB device) don't send more data
> than requested.
>
> With that in mind, I think we should stick with the current
> scheme (ie only allocate a new buffer if the provided buffer is
> unaligned) unless Boris comes back and tells us that it doesn't
> work.

I dunno.  I'd rather see correctness over performance.  Certainly we'd
only need to do the extra bounce buffer for input buffers at least.

Although I don't love the idea, is this something where we want to
introduce a config option (either runtime or through KConfig),
something like:

CONFIG_DWC2_FAST_AND_LOOSE - Avoid bounce buffers and thus run faster
at the risk of a bad USB device being able to clobber some of your
memory.  Only do this if you really care about speed and have some
trust in the USB devices connected to your system.


-Doug