[RFC] dwc2: Don't assume URB transfer_buffer are dword-aligned
Commit Message
The dwc2 hardware doesn't like to do DMA transfers without
aligning data in DWORD. The driver also assumes that, even
when there's no DMA, at dwc2_read_packet().
That cause buffer overflows, preventing some drivers to work.
In the specific case of uvc_driver, it uses an array where
it caches the content of video controls, passing it to the
USB stack via usb_control_msg(). Typical controls use 1 or 2
bytes. The net result is that other values of the buffer
gets overriden when this function is called.
Fix it by changing the logic at dwc2_alloc_dma_aligned_buffer()
to ensure that the buffer used for DMA will be DWORD-aligned.
Detected with uvc driver.
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
On Raspberry Pi 3, I was unable to test dwc2_read_packet(), as this was
never called there. However, the change at the second hunk, e. g. at
dwc2_alloc_dma_aligned_buffer() made UVC to answer the same way
as on x86, while reading the values for the device controls.
drivers/usb/dwc2/hcd.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
Comments
Em Wed, 29 Mar 2017 11:57:22 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> On Tue, Mar 28, 2017 at 06:48:02AM -0300, Mauro Carvalho Chehab wrote:
> > Em Fri, 17 Mar 2017 10:24:15 +0900
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> >
> > > On Thu, Mar 16, 2017 at 09:08:40PM -0300, Mauro Carvalho Chehab wrote:
> > > > The dwc2 hardware doesn't like to do DMA transfers without
> > > > aligning data in DWORD. The driver also assumes that, even
> > > > when there's no DMA, at dwc2_read_packet().
> > > >
> > > > That cause buffer overflows, preventing some drivers to work.
> > >
> > > Why aren't the drivers being fixed? This is a well-known (hopefully)
> > > restriction on USB data buffers, can't the uvc_driver be fixed?
> >
> > I talked to Laurent about on IRC. He told that he is willing to consider
> > that option, if the USB API explicitly states that all buffers must be
> > dword-aligned (and/or buffer sizes).
> >
> > IMHO, he has a point: if this is a restriction of for usb transfer
> > buffers, it should be documented somewhere.
> >
> > Yet, a quick check with:
> > $ git grep -i dword Documentation/usb/
> > $ git grep -i align Documentation/usb/
> >
> > Didn't hit anything related to it.
>
> Hm, most of the USB documentation is in the kerneldoc in the USB code
> itself, and is built that way. I'm pretty sure this is documented
> somewhere, but I can't seem to find it at the moment either :(
>
> Care to write a patch for that? :)
Sure. Btw, I noticed that not all USB documents were converted
yet to ReST, so I took the time to convert the core documents to ReST
too.
I kept the driver-specific documentation at Documentation/usb.
The final result is at:
http://www.infradead.org/~mchehab/kernel_docs/driver-api/usb/index.html
I'll be sending the documentation patches in a few.
Thanks,
Mauro
@@ -572,20 +572,26 @@ u32 dwc2_calc_frame_interval(struct dwc2_hsotg *hsotg)
void dwc2_read_packet(struct dwc2_hsotg *hsotg, u8 *dest, u16 bytes)
{
u32 __iomem *fifo = hsotg->regs + HCFIFO(0);
- u32 *data_buf = (u32 *)dest;
- int word_count = (bytes + 3) / 4;
- int i;
-
- /*
- * Todo: Account for the case where dest is not dword aligned. This
- * requires reading data from the FIFO into a u32 temp buffer, then
- * moving it into the data buffer.
- */
+ u32 *data_buf = (u32 *)dest, tmp;
+ int word_count = bytes >> 2;
+ int i, j;
dev_vdbg(hsotg->dev, "%s(%p,%p,%d)\n", __func__, hsotg, dest, bytes);
for (i = 0; i < word_count; i++, data_buf++)
*data_buf = dwc2_readl(fifo);
+
+ /* Handle the case where the buffer is not dword-aligned */
+ if (bytes & 0x3) {
+ u8 *buf = (u8 *)data_buf;
+
+ tmp = dwc2_readl(fifo);
+
+ i <<= 2;
+ for (j = 0; i < bytes; j++, i++, dest++)
+ *buf = tmp >> (8 * j);
+ }
+
}
/**
@@ -2594,8 +2600,9 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
size_t kmalloc_size;
if (urb->num_sgs || urb->sg ||
- urb->transfer_buffer_length == 0 ||
- !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
+ urb->transfer_buffer_length == 0 || (
+ !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)) &&
+ !((uintptr_t)(urb->transfer_buffer + urb->transfer_buffer_length) & (DWC2_USB_DMA_ALIGN - 1))))
return 0;
/* Allocate a buffer with enough padding for alignment */