On Fri, Feb 28, 2020 at 08:14:35AM -0800, Doug Anderson wrote:
> 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.
>
I understand your point. Unfortunately that would only work if the driver
doesn't set URB_NO_TRANSFER_DMA_MAP.
$ git grep "=.*URB_NO_TRANSFER_DMA_MAP" | wc
115 498 10104
isn't exactly reassuring - a quick checks suggests that almost 50%
of USB drivers set this flag.
So all we'd really accomplish is to give people a false sense of
security.
In this context, I did play around with configuring the real receive
buffer size (ie in my reproducer 1522 instead of 1536). If I do that,
reading the HCTSIZ register after the transfer reports 0x7fff2
(or -14 = 1522-1536 if I treat the value as signed) as actual transfer
size. Maybe that would be an option, if properly handled, but who knows
what the IP actually does in this case, and what it does on other
implementations (not rk3288).
Guenter
