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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

On Thursday 30 Mar 2017 07:28:00 Mauro Carvalho Chehab wrote:
> Em Thu, 30 Mar 2017 12:34:32 +0300 Laurent Pinchart escreveu:
> > On Thursday 30 Mar 2017 10:11:31 Oliver Neukum wrote:
> >> Am Donnerstag, den 30.03.2017, 01:15 +0300 schrieb Laurent Pinchart:
> >>>> +   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.
> >> 
> >> That effectively changes the API. Many network drivers are written with
> >> the assumption that any contiguous buffer is valid. In fact you could
> >> argue that those drivers are buggy and must use bounce buffers in those
> >> cases.
> 
> Blaming the dwc2 driver was my first approach, but such patch got nacked ;)
> 
> Btw, the dwc2 driver has a routine that creates a temporary buffer if the
> buffer pointer is not DWORD aligned. My first approach were to add
> a logic there to also use the temporary buffer if the buffer size is
> not DWORD aligned:
> 	https://patchwork.linuxtv.org/patch/40093/
> 
> While debugging this issue, I saw *a lot* of network-generated URB
> traffic from RPi3 Ethernet port drivers that were using non-aligned
> buffers and were subject to the temporary buffer conversion.
> 
> My understanding here is that having a temporary bounce buffer sucks,
> as the performance and latency are affected. So, I see the value of
> adding this constraint to the API, pushing the task of getting
> aligned buffers to the USB drivers,

This could however degrade performances when the HCD can handle unaligned 
buffers.

> but you're right: that means a lot of work, as all USB drivers should be
> reviewed.

If we decide in the end to push the constraint on the USB device driver side, 
then the dwc2 HCD driver should return an error when the buffer isn't 
correctly aligned.

> Btw, I'm a lot more concerned about USB storage drivers. When I was
> discussing about this issue at the #raspberrypi-devel IRC channel,
> someone complained that, after switching from the RPi downstream Kernel
> to upstream, his USB data storage got corrupted. Well, if the USB
> storage drivers also assume that the buffer can be continuous,
> that can corrupt data.
> 
> That's why I think that being verbose here is a good idea.
> 
> I'll rework on this patch to put more emphasis about this issue.
> 
> >> So we need to include the full story here.
> > 
> > I personally don't care much about whose side is responsible for handling
> > the alignment constraints, but I want it to be documented before "fixing"
> > any USB driver.

-- 
Regards,

Laurent Pinchart