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 12:38:42 +0300
Laurent Pinchart <laurent.pinchart@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@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