Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Thu, 30 Mar 2017 01:15:27 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> 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@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.

> 
> > +   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.

Thanks,
Mauro