Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Oliver Neukum wrote: > Am Donnerstag, den 30.03.2017, 11:55 -0400 schrieb Alan Stern: > > > > I'm pretty sure that usb-storage does not do this, at least, not when > > operating in its normal Bulk-Only-Transport mode. It never tries to > > read the results of an earlier transfer after carrying out a later > > transfer to any part of the same buffer. > > The storage driver takes buffers as the block layer (or sg) provide > them, does it not? Yes. But it does not read the data from an earlier transfer after carrying out a later transfer on the same buffer. The only possible example would be if the sense buffer for a command occupied part of the same block as the data buffer. But this can't happen, because the sense buffer is allocated separately by its own kzalloc_node() call in scsi_init_request(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Am Donnerstag, den 30.03.2017, 11:55 -0400 schrieb Alan Stern: > > I'm pretty sure that usb-storage does not do this, at least, not when > operating in its normal Bulk-Only-Transport mode. It never tries to > read the results of an earlier transfer after carrying out a later > transfer to any part of the same buffer. The storage driver takes buffers as the block layer (or sg) provide them, does it not? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Alan,
On Thursday 30 Mar 2017 11:55:18 Alan Stern wrote:
> On Thu, 30 Mar 2017, Mauro Carvalho Chehab wrote:
> > Em Thu, 30 Mar 2017 10:26:32 -0400 (EDT) Alan Stern escreveu:
> >> On Thu, 30 Mar 2017, Oliver Neukum wrote:
> 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.
> >>>
> >>> They do assume that.
> >>
> >> Wait a minute. Where does that assumption occur?
> >>
> >> And exactly what is the assumption? Mauro wrote "the buffer can be
> >> continuous", but that is certainly not what he meant.
> >
> > What I meant to say is that drivers like the uvcdriver (and maybe network
> > and usb-storage drivers) may allocate a big buffer and get data there on
> > some random order, e. g.:
> >
> > int get_from_buf_pos(char *buf, int pos, int size)
> > {
> > /* or an equivalent call to usb_submit_urb() */
> > usb_control_msg(..., buf + pos, size, ...);
> > }
> >
> > some_function ()
> > {
> > ...
> >
> > chr *buf = kzalloc(4, GFP_KERNEL);
> >
> > /*
> > * Access the bytes at the array on a random order, with random size,
> > * Like:
> > */
> > get_from_buf_pos(buf, 2, 2);/* should read 0x56, 0x78 */
> > get_from_buf_pos(buf, 0, 2);/* should read 0x12, 0x34 */
> >
> > /*
> > * the expected value for the buffer would be:
> > * { 0x12, 0x34, 0x56, 0x78 }
> > */
> >
> > E. g. they assume that the transfer URB can work with any arbitrary
> > pointer and size, without needing of pre-align them.
> >
> > This doesn't work with HCD drivers like dwc2, as each USB_IN operation
> > will actually write 4 bytes to the buffer.
> >
> > So, what happens, instead, is that each data transfer will get four
> > bytes. Due to a hack inside dwc2, with checks if the transfer_buffer
> > is DWORD aligned. So, the first transfer will do what's expected: it will
> > read 4 bytes to a temporary buffer, allocated inside the driver,
> > copying just two bytes to buf. So, after the first read, the
> >
> > buffer content will be:
> > buf = { 0x00, x00, 0x56, 0x78 }
> >
> > But, on the second read, it won't be using any temporary
> > buffer. So, instead of reading a 16-bits word (0x5678),
> > it will actually read 32 bits, with 16-bits with some random value,
> >
> > causing a buffer overflow. E. g. buffer content will now be:
> > buf = { 0x12, x34, 0xde, 0xad }
> >
> > In other words, the second transfer corrupted the data from the
> > first transfer.
>
> I'm pretty sure that usb-storage does not do this, at least, not when
> operating in its normal Bulk-Only-Transport mode. It never tries to
> read the results of an earlier transfer after carrying out a later
> transfer to any part of the same buffer.
The uvcvideo driver does something similar. Given the size of the transfer a
bounce buffer shouldn't affect performances. Handling this in the USB core
sounds like the best solution to me.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Mauro Carvalho Chehab wrote:
> Em Thu, 30 Mar 2017 10:26:32 -0400 (EDT)
> Alan Stern escreveu:
>
> > On Thu, 30 Mar 2017, Oliver Neukum wrote:
> >
> > > > 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.
> >
> > >
> > > They do assume that.
> >
> > Wait a minute. Where does that assumption occur?
> >
> > And exactly what is the assumption? Mauro wrote "the buffer can be
> > continuous", but that is certainly not what he meant.
>
> What I meant to say is that drivers like the uvcdriver (and maybe network and
> usb-storage drivers) may allocate a big buffer and get data there on some
> random order, e. g.:
>
> int get_from_buf_pos(char *buf, int pos, int size)
> {
> /* or an equivalent call to usb_submit_urb() */
> usb_control_msg(..., buf + pos, size, ...);
> }
>
> some_function ()
> {
> ...
>
> chr *buf = kzalloc(4, GFP_KERNEL);
>
> /*
>* Access the bytes at the array on a random order, with random size,
>* Like:
>*/
> get_from_buf_pos(buf, 2, 2);/* should read 0x56, 0x78 */
> get_from_buf_pos(buf, 0, 2);/* should read 0x12, 0x34 */
>
> /*
>* the expected value for the buffer would be:
>* { 0x12, 0x34, 0x56, 0x78 }
>*/
>
> E. g. they assume that the transfer URB can work with any arbitrary
> pointer and size, without needing of pre-align them.
>
> This doesn't work with HCD drivers like dwc2, as each USB_IN operation will
> actually write 4 bytes to the buffer.
>
> So, what happens, instead, is that each data transfer will get four
> bytes. Due to a hack inside dwc2, with checks if the transfer_buffer
> is DWORD aligned. So, the first transfer will do what's expected: it will
> read 4 bytes to a temporary buffer, allocated inside the driver,
> copying just two bytes to buf. So, after the first read, the
> buffer content will be:
>
> buf = { 0x00, x00, 0x56, 0x78 }
>
> But, on the second read, it won't be using any temporary
> buffer. So, instead of reading a 16-bits word (0x5678),
> it will actually read 32 bits, with 16-bits with some random value,
> causing a buffer overflow. E. g. buffer content will now be:
>
> buf = { 0x12, x34, 0xde, 0xad }
>
> In other words, the second transfer corrupted the data from the
> first transfer.
I'm pretty sure that usb-storage does not do this, at least, not when
operating in its normal Bulk-Only-Transport mode. It never tries to
read the results of an earlier transfer after carrying out a later
transfer to any part of the same buffer.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Em Thu, 30 Mar 2017 10:26:32 -0400 (EDT)
Alan Stern escreveu:
> On Thu, 30 Mar 2017, Oliver Neukum wrote:
>
> > > 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.
>
> >
> > They do assume that.
>
> Wait a minute. Where does that assumption occur?
>
> And exactly what is the assumption? Mauro wrote "the buffer can be
> continuous", but that is certainly not what he meant.
What I meant to say is that drivers like the uvcdriver (and maybe network and
usb-storage drivers) may allocate a big buffer and get data there on some
random order, e. g.:
int get_from_buf_pos(char *buf, int pos, int size)
{
/* or an equivalent call to usb_submit_urb() */
usb_control_msg(..., buf + pos, size, ...);
}
some_function ()
{
...
chr *buf = kzalloc(4, GFP_KERNEL);
/*
* Access the bytes at the array on a random order, with random size,
* Like:
*/
get_from_buf_pos(buf, 2, 2);/* should read 0x56, 0x78 */
get_from_buf_pos(buf, 0, 2);/* should read 0x12, 0x34 */
/*
* the expected value for the buffer would be:
* { 0x12, 0x34, 0x56, 0x78 }
*/
E. g. they assume that the transfer URB can work with any arbitrary
pointer and size, without needing of pre-align them.
This doesn't work with HCD drivers like dwc2, as each USB_IN operation will
actually write 4 bytes to the buffer.
So, what happens, instead, is that each data transfer will get four
bytes. Due to a hack inside dwc2, with checks if the transfer_buffer
is DWORD aligned. So, the first transfer will do what's expected: it will
read 4 bytes to a temporary buffer, allocated inside the driver,
copying just two bytes to buf. So, after the first read, the
buffer content will be:
buf = { 0x00, x00, 0x56, 0x78 }
But, on the second read, it won't be using any temporary
buffer. So, instead of reading a 16-bits word (0x5678),
it will actually read 32 bits, with 16-bits with some random value,
causing a buffer overflow. E. g. buffer content will now be:
buf = { 0x12, x34, 0xde, 0xad }
In other words, the second transfer corrupted the data from the
first transfer.
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Oliver Neukum wrote: > > 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. > > They do assume that. Wait a minute. Where does that assumption occur? And exactly what is the assumption? Mauro wrote "the buffer can be continuous", but that is certainly not what he meant. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Am Donnerstag, den 30.03.2017, 07:28 -0300 schrieb Mauro Carvalho Chehab: > Em Thu, 30 Mar 2017 12:34:32 +0300 > Laurent Pinchart escreveu: > > > Hi, > > > 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 ;) That I am afraid was a mistake in a certain sense. This belongs into usbcore. It does not belong into controller drivers or device drivers. > 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 If you need it, you need it. Doing this in the device driver isn't any less problematic in terms of performance. The only thing we can do is do it in a central place to avoid code duplication. > adding this constraint to the API, pushing the task of getting > aligned buffers to the USB drivers, but you're right: that means a lot > of work, as all USB drivers should be reviewed. And it is wrong. To be blunt: The important drivers are EHCI and XHCI. We must not degrade performance with them for the sake of controllers with less capabilities. > 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. They do assume that. > I'll rework on this patch to put more emphasis about this issue. For now that is the best. But even in the medium term this sucks. At a minimum controller drivers need to export what they can do. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Em Thu, 30 Mar 2017 15:05:30 +0300 Laurent Pinchart escreveu: > 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. No, it won't degrade performance out there, except if the driver would need to do double buffering due to such constraint. It will just waste memory. > > > 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. Yeah, with such constraint, either the HCD drivers or the USB core should complain. There is another option: to add a field, filled by te USB driver, telling the core that the buffer is aligned, e. g. drivers would be doing something like: urb->transfer_buffer_align = 4; Something similar could be filled by the HCD driver: hc_driver->transfer_buffer_align = 4; The core will then check if the alignment required by the HCD driver is compatible with the buffer alignment ensured by the USB driver. If it doesn't, then the core would create a temporary buffer for the transfers. No idea about how easy/hard would be to implement something like that. In such case, it could make sense to generate a warning that the driver should be fixed, or that the performance would decrease due to double-buffering. Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
From: Mauro Carvalho Chehab > Sent: 30 March 2017 11:28 ... > 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. Buffers from the network stack will almost always be 4n+2 aligned. Receive data being fed into the network stack really needs to be 4n=2 aligned. The USB stack almost certainly has to live with that. If the USB ethernet device doesn't have two bytes of 'pad' before the frame data (destination MAC address) then you have to solve the problem within the USB stack. For transmits it might be possible to send an initial 2 byte fragment from a separate buffer - but only if arbitrary fragment sizes are allowed. A normal USB3 controller should allow this - but you have to be very careful about what happens at the end of the ring. David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Em Thu, 30 Mar 2017 12:38:42 +0300 Laurent Pinchart 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 > > > > --- > > > > > > > > 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/messa
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Em Thu, 30 Mar 2017 12:34:32 +0300 Laurent Pinchart escreveu: > Hi Oliver, > > 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, but you're right: that means a lot of work, as all USB drivers should be reviewed. 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. > Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
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 > > > --- > > > > > > 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. > > > + 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 > >
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Hi Oliver, 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
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. So we need to include the full story here. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
Em Thu, 30 Mar 2017 01:15:27 +0300 Laurent Pinchart 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 > > --- > > 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
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
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 > --- > 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. > + 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) ? > + 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. > + 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
[PATCH 22/22] usb: document that URB transfer_buffer should be aligned
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 --- 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). + 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 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. + * * Initialization: * * All URBs submitted must initialize the dev, pipe, transfer_flags (may be -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
