[PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Mauro Carvalho Chehab
mchehab at s-opensource.com
Thu Mar 30 03:51:49 PDT 2017
Em Thu, 30 Mar 2017 12:38:42 +0300
Laurent Pinchart <laurent.pinchart at ideasonboard.com> escreveu:
> Hi Mauro,
>
> On Wednesday 29 Mar 2017 22:06:33 Mauro Carvalho Chehab wrote:
> > Em Thu, 30 Mar 2017 01:15:27 +0300 Laurent Pinchart escreveu:
> > > On Wednesday 29 Mar 2017 15:54:21 Mauro Carvalho Chehab wrote:
> > > > Several host controllers, commonly found on ARM, like dwc2,
> > > > require buffers that are CPU-word aligned for they to work.
> > > >
> > > > Failing to do that will cause random troubles at the caller
> > > > drivers, causing them to fail.
> > > >
> > > > Document it.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab at s-opensource.com>
> > > > ---
> > > >
> > > > Documentation/driver-api/usb/URB.rst | 18 ++++++++++++++++++
> > > > drivers/usb/core/message.c | 15 +++++++++++++++
> > > > include/linux/usb.h | 18 ++++++++++++++++++
> > > > 3 files changed, 51 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/usb/URB.rst
> > > > b/Documentation/driver-api/usb/URB.rst index d9ea6a3996e7..b83b557e9891
> > > > 100644
> > > > --- a/Documentation/driver-api/usb/URB.rst
> > > > +++ b/Documentation/driver-api/usb/URB.rst
> > > > @@ -274,6 +274,24 @@ If you specify your own start frame, make sure it's
> > > > several frames in advance of the current frame. You might want this
> > > > model
> > > > if you're synchronizing ISO data with some other event stream.
> > > >
> > > > +.. note::
> > > > +
> > > > + Several host drivers require that the ``transfer_buffer`` to be
> > > > aligned
> > > > + with the CPU word size (e. g. DWORD for 32 bits, QDWORD for 64
> > > > bits).
> > >
> > > Is it the CPU word size or the DMA transfer size ? I assume the latter,
> > > and I wouldn't be surprised if the alignment requirement was 32-bit on at
> > > least some of the 64-bit platforms.
> >
> > Yeah, it is actually the DMA transfer size. Yet, worse case scenario is that
> > the DMA transfer size to be 64 bits on 64 bits CPU.
> >
> > > > + It is up to USB drivers should ensure that they'll only pass buffers
> > > > + with such alignments.
> > > > +
> > > > + Please also notice that, due to such restriction, the host driver
> > >
> > > s/notice/note/ (and below as well) ?
> >
> > OK.
> >
> > > > + may also override PAD bytes at the end of the ``transfer_buffer``,
> > > > up to the
> > > > + size of the CPU word.
> > >
> > > "May" is quite weak here. If some host controller drivers require buffers
> > > to be aligned, then it's an API requirement, and all buffers must be
> > > aligned. I'm not even sure I would mention that some host drivers require
> > > it, I think we should just state that the API requires buffers to be
> > > aligned.
> >
> > What I'm trying to say here is that, on a 32-bits system, if the driver do
> > a USB_DIR_IN transfer using some code similar to:
> >
> > size = 4;
> > buffer = kmalloc(size, GFP_KERNEL);
> >
> > usb_control_msg(udev, pipe, req, type, val, idx, buffer + 2, 2,
> timeout);
> > usb_control_msg(udev, pipe, req, type, val, idx, buffer, size,
> timeout);
> >
> > Drivers like dwc2 will mess with the buffer.
> >
> > The first transfer will actually work, due to a workaround inside the
> > driver that will create a temporary DWORD-aligned buffer, avoiding it
> > to go past the buffer.
> >
> > However, the second transfer will destroy the data received from the
> > first usb_control_msg(), as it will write 4 bytes at the buffer.
> >
> > Not all drivers would do that, though.
> >
> > Please notice that, as kmalloc will always return a CPU-aligned buffer,
> > if the client do something like:
> >
> > size = 2;
> > buffer = kmalloc(size, GFP_KERNEL);
> >
> > usb_control_msg(udev, pipe, req, type, val, idx, buffer, 2, timeout);
> >
> > What happens there is that the DMA engine will still write 4 bytes at
> > the buffer, but the 2 bytes that go past the end of buffer will be
> > written on a memory that will never be used.
>
> I understand that, but stating that host controller drivers "may" do this
> won't help much. If they *may*, all USB device drivers *must* align buffers
> correctly. That's the part that needs to be documented. Let's not confuse
> developers by only stating that something may happened, let's be clear and
> tell what they must do.
Ok, rewrote the entire text. Please see if the new version
works better.
>
> > > > + Please notice that ancillary routines that transfer URBs, like
> > > > + usb_control_msg() also have such restriction.
> > > > +
> > > > + Such word alignment condition is normally ensured if the buffer is
> > > > + allocated with kmalloc(), but this may not be the case if the driver
> > > > + allocates a bigger buffer and point to a random place inside it.
> > > > +
> > > >
> > > > How to start interrupt (INT) transfers?
> > > > =======================================
> > > >
> > > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > > > index 4c38ea41ae96..1662a4446475 100644
> > > > --- a/drivers/usb/core/message.c
> > > > +++ b/drivers/usb/core/message.c
> > > > @@ -128,6 +128,21 @@ static int usb_internal_control_msg(struct
> > > > usb_device
> > > > *usb_dev, * make sure your disconnect() method can wait for it to
> > > > complete.
> > > > Since you * don't have a handle on the URB used, you can't cancel the
> > > > request. *
> > > > + * .. note::
> > > > + *
> > > > + * Several host drivers require that the @data buffer to be aligned
> > > > + * with the CPU word size (e. g. DWORD for 32 bits, QDWORD for 64
> > > > bits).
> > > > + * It is up to USB drivers should ensure that they'll only pass
> > > > buffers
> > > > + * with such alignments.
> > > > + *
> > > > + * Please also notice that, due to such restriction, the host driver
> > > > + * may also override PAD bytes at the end of the @data buffer, up to
> > > > the
> > > > + * size of the CPU word.
> > > > + *
> > > > + * Such word alignment condition is normally ensured if the buffer is
> > > > + * allocated with kmalloc(), but this may not be the case if the
> > > > driver
> > > > + * allocates a bigger buffer and point to a random place inside it.
> > > > + *
> > > >
> > > > * Return: If successful, the number of bytes transferred. Otherwise, a
> > > >
> > > > negative * error number.
> > > >
> > > > */
> > > >
> > > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > > index 7e68259360de..8b5ad6624708 100644
> > > > --- a/include/linux/usb.h
> > > > +++ b/include/linux/usb.h
> > > > @@ -1373,6 +1373,24 @@ typedef void (*usb_complete_t)(struct urb *);
> > > >
> > > > * capable, assign NULL to it, so that usbmon knows not to use the
> > > > value.
> > > > * The setup_packet must always be set, so it cannot be located in
> > > > highmem.
> > > >
> > > > *
> > > > + * .. note::
> > > > + *
> > > > + * Several host drivers require that the @transfer_buffer to be
> > > > aligned
> > > > + * with the CPU word size (e. g. DWORD for 32 bits, QDWORD for 64
> > > > bits).
> > > > + * It is up to USB drivers should ensure that they'll only pass
> > > > buffers
> > > > + * with such alignments.
> > > > + *
> > > > + * Please also notice that, due to such restriction, the host driver
> > > > + * may also override PAD bytes at the end of the @transfer_buffer, up
> > > > to
> > > > the + * size of the CPU word.
> > > > + *
> > > > + * Please notice that ancillary routines that start URB transfers,
> > > > like
> > > > + * usb_control_msg() also have such restriction.
> > > > + *
> > > > + * Such word alignment condition is normally ensured if the buffer is
> > > > + * allocated with kmalloc(), but this may not be the case if the
> > > > driver
> > > > + * allocates a bigger buffer and point to a random place inside it.
> > > > + *
> > >
> > > Couldn't we avoid three copies of the same text ? The chance they will get
> > > out-of-sync is quite high.
> >
> > IMHO, it is better to document it at those 3 parts, as this issue
> > cause buffer overflows, which is pretty serious, as it corrupts data.
> >
> > The URB.rst file contains a quick overview of the URB data transfers,
> > and it is likely where a kernel newbie would read first. Experienced
> > programmers will look at urb.h.
> >
> > usb_control_msg() is a different function, that one might not be
> > expecting to have the same issues.
>
> If you add the complete explanation to URB.rst, you can then reference it from
> the functions' kerneldoc instead of copying it.
True, but URB.txt has a note saying that the document may not be
fully updated, implying that urb.h header is the main reference.
So, I stand that the best here is to have this warning duplicated,
as this is a pretty serious issue and will require people to review
all USB drivers.
Thanks,
Mauro
More information about the linux-rpi-kernel
mailing list