Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-28 Thread Jussi Kivilinna
On 16.06.2013 13:35, Jussi Kivilinna wrote:
 On 16.06.2013 11:21, Oliver Neukum wrote:
 On Saturday 15 June 2013 16:22:30 Jussi Kivilinna wrote:

 Hm.. rethink this a bit.

 Transfer buffer might be dma aligned but shorter than cacheline and end of 
 cacheline
 used as something else. Manual alignment by host driver does not catch that
 or fix that.
 So, yes.. dma mapping should work with unaligned buffers, but maybe the 
 actual
 problem is multiple buffers from same cacheline.

 The buffers kmalloc() returns are OK in that regard. A driver that uses
 a buffer for anything but buffering is buggy.
 
 Ok, I'll look at that direction. Thanks.
 

So if I understood correctly, drivers that allocate these as part of larger 
structures (struct *_device etc) are doing wrong thing and are potentially 
buggy. And this is because cachelines of buffers can be DMA mapped after 
usb_submit_urb() and editing same cacheline while URB is in-flight can 
therefore be hazardous.

I checked setup_packet and transfer_buffer usage of some drivers in 3.9.8 and 
made some observations. Should these be fixed?

URB setup_packet and transfer_buffer part of same structure (might share same 
cacheline for same URB):
 * iforce:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce-usb.c#L173
  - http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce.h#L101
 * usbvision:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/media/usb/usbvision/usbvision-core.c#L1445
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/media/usb/usbvision/usbvision.h#L366
 * catc:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L499
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L500
  - ctrl_buf, ctrl_dr: 
http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L162
 * rtl8150:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/rtl8150.c#L200
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/rtl8150.c#L128
 * rt2x000usb:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2x00usb.c#L212
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2x00usb.c#L169
 * rtl8187:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/dev.c#L156
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/dev.c#L130
 * uss720:
  - http://lxr.linux.no/linux+v3.9.8/drivers/usb/misc/uss720.c#L176
  - http://lxr.linux.no/linux+v3.9.8/drivers/usb/misc/uss720.c#L72

URB transfer_buffer array (transfer buffers preloaded as array, element size 
less than cacheline):
 * rtlwifi:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/usb.c#L152
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/wifi.h#L1859
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/usb.c#L980
 * catc:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L371
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L162

URB setup_packet part of larger structure:
 * gigaset:
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/gigaset/bas-gigaset.c#L90
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/gigaset/bas-gigaset.c#L581
 * mISDN/hfcsusb:
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.c#L1732
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.c#L74
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.h#L270
 * hisax/hfc_usb:
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L1185
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L227
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L193
 * hisax/st5481:
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/st5481_usb.c#L42
  - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/st5481.h#L326
 * hso:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/hso.c#L1812
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/hso.c#L1846
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/hso.c#L220
 * pegasus:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.c#L169
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.h#L111
 * brcmfmac:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/brcm80211/brcmfmac/usb.c#L209
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/brcm80211/brcmfmac/usb.c#L71
 * staging/vt6656:
  - http://lxr.linux.no/linux+v3.9.8/drivers/staging/vt6656/usbpipe.c#L142
  - http://lxr.linux.no/linux+v3.9.8/drivers/staging/vt6656/device.h#L390

URB transfer_buffer part of larger structure:
 * iforce:
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce-usb.c#L170
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce-usb.c#L167
  - http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce.h#L101
  - 
http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce-usb.c#L147
 * ks959:
  - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/ks959-sir.c#L278
  - 

Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-28 Thread Oliver Neukum
On Friday 28 June 2013 15:39:27 Jussi Kivilinna wrote:
 On 16.06.2013 13:35, Jussi Kivilinna wrote:
  On 16.06.2013 11:21, Oliver Neukum wrote:
  On Saturday 15 June 2013 16:22:30 Jussi Kivilinna wrote:
 
  Hm.. rethink this a bit.
 
  Transfer buffer might be dma aligned but shorter than cacheline and end 
  of cacheline
  used as something else. Manual alignment by host driver does not catch 
  that
  or fix that.
  So, yes.. dma mapping should work with unaligned buffers, but maybe the 
  actual
  problem is multiple buffers from same cacheline.
 
  The buffers kmalloc() returns are OK in that regard. A driver that uses
  a buffer for anything but buffering is buggy.
  
  Ok, I'll look at that direction. Thanks.
  
 
 So if I understood correctly, drivers that allocate these as part of larger 
 structures (struct *_device etc) are doing wrong thing and are potentially 
 buggy. And this is because cachelines of buffers can be DMA mapped after 
 usb_submit_urb() and editing same cacheline while URB is in-flight can 
 therefore be hazardous.
 
 I checked setup_packet and transfer_buffer usage of some drivers in 3.9.8 and 
 made some observations. Should these be fixed?
 
 URB setup_packet and transfer_buffer part of same structure (might share same 
 cacheline for same URB):
  * iforce:
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce-usb.c#L173
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/input/joystick/iforce/iforce.h#L101

Buggy

  * usbvision:
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/media/usb/usbvision/usbvision-core.c#L1445
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/media/usb/usbvision/usbvision.h#L366

Buggy

  * catc:
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L499
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L500
   - ctrl_buf, ctrl_dr: 
 http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L162
  * rtl8150:
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/rtl8150.c#L200
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/rtl8150.c#L128
  * rt2x000usb:
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2x00usb.c#L212
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2x00usb.c#L169
  * rtl8187:
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/dev.c#L156
   - 
 http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/dev.c#L130
  * uss720:
   - http://lxr.linux.no/linux+v3.9.8/drivers/usb/misc/uss720.c#L176
   - http://lxr.linux.no/linux+v3.9.8/drivers/usb/misc/uss720.c#L72

Well, I didn't look through them all, but we must assume that they are buggy.

 URB transfer_buffer array (transfer buffers preloaded as array, element size 
 less than cacheline):
  * rtlwifi:
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/usb.c#L152
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/wifi.h#L1859
   - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtlwifi/usb.c#L980

Good catch.

This is a very large number. I suggest you split it up.

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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-16 Thread Oliver Neukum
On Saturday 15 June 2013 16:22:30 Jussi Kivilinna wrote:

 Hm.. rethink this a bit.
 
 Transfer buffer might be dma aligned but shorter than cacheline and end of 
 cacheline
 used as something else. Manual alignment by host driver does not catch that
 or fix that.
 So, yes.. dma mapping should work with unaligned buffers, but maybe the actual
 problem is multiple buffers from same cacheline.

The buffers kmalloc() returns are OK in that regard. A driver that uses
a buffer for anything but buffering is buggy.

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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-16 Thread Jussi Kivilinna
On 15.06.2013 16:47, Ming Lei wrote:
 On Sat, Jun 15, 2013 at 9:10 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 On 15.06.2013 15:07, Ming Lei wrote:
 On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 On 15.06.2013 10:41, Ming Lei wrote:
 Cc: netdev

 On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA 
 alignment for

 Looks these host drivers have to face the fact that the transfer buffer 
 is often
 DMA non-aligned from network device drivers(in fact, the buffer is from
 network protocol stack), if you run usbnet, then you will get the added 
 warning
 immediately.


 Yes, getting warning immediately, but once, and blaming host driver seems 
 ok.

 We do know the fact of non-aligned transfer buffer from network, which has 
 been
 for long time, so does it make sense to print warning and annoy people?

 It's only printed if host controller driver is not behaving correctly.
 
 If you make sure the warning is only printed on broken controller,
 that is fine.
 

 I have changed the message to be printed for v2-patch, and it is now:
 dev_WARN_ONCE(hcd-self.controller, 1,
 broken USB host controller driver; does not correctly handle DMA 
 alignment for urb-transfer_buffer (offset: %d).\n,
 dma_offset);

 I sent the patch as RFC since I'm not sure.. maybe annoying warnings make
 
 That is fine.
 
 people aware of issues that they don't yet know of and things get fixed?
 
 I mean it isn't good to annoy people who are using good host controller, :-)
 

Yes, and now it seems that this might very well be the case with this patch. It
would cause false warnings.



 URB buffers and let core/hcd.c to do the mapping on architectures that 
 have
 minimum DMA alignment requirements. This leads to random memory 
 corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

 Maybe you should check the dma mapping/unmapping implementation of
 the arch, non-aligned buffer should have be covered by the API easily.

 Also USB Host controller should have supported non-aligned DMA buffer.

 From what I found, there was some discussion about these issues around 
 2010:
  
 http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022983.html

 From the discussion,  people think that HCD should handle the unaligned 
 buffer,
 right?

 Yes, that's how I understood it.



 To me, it seems that non-aligned buffers cannot be easily handled by all 
 archs
 at dma mapping/unmapping phase and that HCD driver should do the alignment 
 on

 If the memory which shares cache line with transfer buffer can't be
 accessed during
 DMA transfer(between URB submit and complete), dma mapping/unmapping
 should have handled it.

 About the network transfer buffer case, I think it should be true,
 otherwise there
 should have lots of memory corruption reports about usb network drivers.
 Fortunately, there are seldom such reports.


 Another reason why rtl8192cu is so hard, is that it uses pre-allocated array
 for buffers of multiple URBs, and more than one transfer buffer can reside on
 same cache line.
 
 If so, that should be bug inside rtl8192, and more than one transfer
 buffer shares
 one cache line should be avoided, I understand the buffer isn't from
 network stack,
 don't I?
 

Buffer is not from network stack, but driver allocated array used for control 
message.


 archs that set ARCH_DMA_MINALIGN. For example, ehci_tegra does copy 
 unaligned
 transfer buffers to temporary aligned buffers before letting them to USB 
 core.

 Yes, if host controller can't handle this, the HCD has to work around
 the problem. Anyway, most of host controllers can deal with the it,
 can't they?

 Can they? Maybe they can handle most cases of unaligned buffers, but not some
 corner cases, like transfer buffers on same cache line.
 
 Of course, most of in-tree host controller can handle non-aligned buffer.
 
 If transfer buffers share one same cache line, it should be bug in driver,
 not fault of host controller.
 

Ok.






 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Not only rtl8192cu driver, all USB network device drivers have the 
 problem.


 Patch makes this issue more visible at core level, and hopefully gives 
 hint
 for future hcd driver implementors about this problem.

 So please find the root cause first, and don't add the noise now.

 I think the root cause is that host driver is letting pass non-aligned 
 buffers
 to core on archs that have ARCH_DMA_MINALIGN set.

 No, I don't think so, about the problem, the dma alignment requirement 
 should
 be from your host controller.

 As I said above, dma mapping/unmapping should be capable of dealing with
 the unaligned buffer if no one 

Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Ming Lei
Cc: netdev

On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment for

Looks these host drivers have to face the fact that the transfer buffer is often
DMA non-aligned from network device drivers(in fact, the buffer is from
network protocol stack), if you run usbnet, then you will get the added warning
immediately.

 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

Maybe you should check the dma mapping/unmapping implementation of
the arch, non-aligned buffer should have be covered by the API easily.

Also USB Host controller should have supported non-aligned DMA buffer.


 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

Not only rtl8192cu driver, all USB network device drivers have the problem.


 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.

So please find the root cause first, and don't add the noise now.


Thanks,
-- 
Ming Lei
--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Jussi Kivilinna
On 14.06.2013 23:20, Alan Stern wrote:
 On Fri, 14 Jun 2013, Jussi Kivilinna wrote:
 
 Appearently some out-of-tree USB host drivers do not handle DMA alignment for
 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.

 Signed-off-by: Jussi Kivilinna jussi.kivili...@iki.fi
 
 It's probably okay to leave out the test for the setup buffer.  That 
 thing is only 8 bytes long and is unlikely to be misaligned.  The 
 transfer buffer is the one that matters.

The rtl8192cu driver used to allocate the transfer buffer and the setup buffer 
with
same kmalloc, so that setup and transfer buffers for same URB shared cache-line.
This was causing pretty hard crashes with ARM/sunxi device I have. Before I 
realized
that problem really was in the host controller driver, I made patch to rtl8192cu
that changed code to use two separate kmallocs.

Old code looked like:
  http://lxr.linux.no/linux+v3.7/drivers/net/wireless/rtlwifi/usb.c#L58

 
 Also, the error message should include the name of the host controller 
 driver.  How about using dev_WARN_ONCE(hcd-self.controller, ...)?

Ah, thanks, didn't know about dev_WARN*.

-Jussi

 
 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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Jussi Kivilinna
On 15.06.2013 10:41, Ming Lei wrote:
 Cc: netdev
 
 On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment for
 
 Looks these host drivers have to face the fact that the transfer buffer is 
 often
 DMA non-aligned from network device drivers(in fact, the buffer is from
 network protocol stack), if you run usbnet, then you will get the added 
 warning
 immediately.
 

Yes, getting warning immediately, but once, and blaming host driver seems ok.

 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.
 
 Maybe you should check the dma mapping/unmapping implementation of
 the arch, non-aligned buffer should have be covered by the API easily.
 
 Also USB Host controller should have supported non-aligned DMA buffer.

From what I found, there was some discussion about these issues around 2010:
 http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022983.html

To me, it seems that non-aligned buffers cannot be easily handled by all archs
at dma mapping/unmapping phase and that HCD driver should do the alignment on
archs that set ARCH_DMA_MINALIGN. For example, ehci_tegra does copy unaligned
transfer buffers to temporary aligned buffers before letting them to USB core.

 

 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).
 
 Not only rtl8192cu driver, all USB network device drivers have the problem.
 

 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.
 
 So please find the root cause first, and don't add the noise now.

I think the root cause is that host driver is letting pass non-aligned buffers
to core on archs that have ARCH_DMA_MINALIGN set.

The warning given just before such unaligned buffer is passed to dma_map_single,
which requires ARCH_DMA_MINALIGN alignment. This seems reasonable to me.

-Jussi

 
 
 Thanks,
 

--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Ming Lei
On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna jussi.kivili...@iki.fi wrote:
 On 15.06.2013 10:41, Ming Lei wrote:
 Cc: netdev

 On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment 
 for

 Looks these host drivers have to face the fact that the transfer buffer is 
 often
 DMA non-aligned from network device drivers(in fact, the buffer is from
 network protocol stack), if you run usbnet, then you will get the added 
 warning
 immediately.


 Yes, getting warning immediately, but once, and blaming host driver seems ok.

We do know the fact of non-aligned transfer buffer from network, which has been
for long time, so does it make sense to print warning and annoy people?


 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

 Maybe you should check the dma mapping/unmapping implementation of
 the arch, non-aligned buffer should have be covered by the API easily.

 Also USB Host controller should have supported non-aligned DMA buffer.

 From what I found, there was some discussion about these issues around 2010:
  http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022983.html

From the discussion,  people think that HCD should handle the unaligned buffer,
right?


 To me, it seems that non-aligned buffers cannot be easily handled by all archs
 at dma mapping/unmapping phase and that HCD driver should do the alignment on

If the memory which shares cache line with transfer buffer can't be
accessed during
DMA transfer(between URB submit and complete), dma mapping/unmapping
should have handled it.

About the network transfer buffer case, I think it should be true,
otherwise there
should have lots of memory corruption reports about usb network drivers.
Fortunately, there are seldom such reports.

 archs that set ARCH_DMA_MINALIGN. For example, ehci_tegra does copy unaligned
 transfer buffers to temporary aligned buffers before letting them to USB core.

Yes, if host controller can't handle this, the HCD has to work around
the problem. Anyway, most of host controllers can deal with the it,
can't they?




 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Not only rtl8192cu driver, all USB network device drivers have the problem.


 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.

 So please find the root cause first, and don't add the noise now.

 I think the root cause is that host driver is letting pass non-aligned buffers
 to core on archs that have ARCH_DMA_MINALIGN set.

No, I don't think so, about the problem, the dma alignment requirement should
be from your host controller.

As I said above, dma mapping/unmapping should be capable of dealing with
the unaligned buffer if no one touches memory which shares cacheline with
URB-transfer_buffer during URB transfer.

Looks you need to know why the memory corruption happens. Is it caused
by non-aligned arch mapping/unmapping? or by host controller hardware when
dealing with non-aligned transfer buffer?

 The warning given just before such unaligned buffer is passed to 
 dma_map_single,
 which requires ARCH_DMA_MINALIGN alignment. This seems reasonable to me.

ARCH_DMA_MINALIGN means that kmalloc() should return aligned dma buffer.

Again, you have to accept the fact in which transfer buffer from
network stack is
non-aligned.


Thanks,
-- 
Ming Lei
--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Jussi Kivilinna
On 15.06.2013 15:07, Ming Lei wrote:
 On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 On 15.06.2013 10:41, Ming Lei wrote:
 Cc: netdev

 On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment 
 for

 Looks these host drivers have to face the fact that the transfer buffer is 
 often
 DMA non-aligned from network device drivers(in fact, the buffer is from
 network protocol stack), if you run usbnet, then you will get the added 
 warning
 immediately.


 Yes, getting warning immediately, but once, and blaming host driver seems ok.
 
 We do know the fact of non-aligned transfer buffer from network, which has 
 been
 for long time, so does it make sense to print warning and annoy people?

It's only printed if host controller driver is not behaving correctly.

I have changed the message to be printed for v2-patch, and it is now:
dev_WARN_ONCE(hcd-self.controller, 1,
broken USB host controller driver; does not correctly handle DMA 
alignment for urb-transfer_buffer (offset: %d).\n,
dma_offset);

I sent the patch as RFC since I'm not sure.. maybe annoying warnings make
people aware of issues that they don't yet know of and things get fixed?

 

 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

 Maybe you should check the dma mapping/unmapping implementation of
 the arch, non-aligned buffer should have be covered by the API easily.

 Also USB Host controller should have supported non-aligned DMA buffer.

 From what I found, there was some discussion about these issues around 2010:
  
 http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022983.html
 
 From the discussion,  people think that HCD should handle the unaligned 
 buffer,
 right?

Yes, that's how I understood it.

 

 To me, it seems that non-aligned buffers cannot be easily handled by all 
 archs
 at dma mapping/unmapping phase and that HCD driver should do the alignment on
 
 If the memory which shares cache line with transfer buffer can't be
 accessed during
 DMA transfer(between URB submit and complete), dma mapping/unmapping
 should have handled it.
 
 About the network transfer buffer case, I think it should be true,
 otherwise there
 should have lots of memory corruption reports about usb network drivers.
 Fortunately, there are seldom such reports.
 

Another reason why rtl8192cu is so hard, is that it uses pre-allocated array
for buffers of multiple URBs, and more than one transfer buffer can reside on
same cache line.

 archs that set ARCH_DMA_MINALIGN. For example, ehci_tegra does copy unaligned
 transfer buffers to temporary aligned buffers before letting them to USB 
 core.
 
 Yes, if host controller can't handle this, the HCD has to work around
 the problem. Anyway, most of host controllers can deal with the it,
 can't they?

Can they? Maybe they can handle most cases of unaligned buffers, but not some
corner cases, like transfer buffers on same cache line.

 



 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Not only rtl8192cu driver, all USB network device drivers have the problem.


 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.

 So please find the root cause first, and don't add the noise now.

 I think the root cause is that host driver is letting pass non-aligned 
 buffers
 to core on archs that have ARCH_DMA_MINALIGN set.
 
 No, I don't think so, about the problem, the dma alignment requirement should
 be from your host controller.
 
 As I said above, dma mapping/unmapping should be capable of dealing with
 the unaligned buffer if no one touches memory which shares cacheline with
 URB-transfer_buffer during URB transfer.

How can you guarantee that when you allow unaligned URB buffers?

You can have the buffer as part of some larger structure and send out async URB.
Then while buffer is DMA mapped and send async to hw, you use other parts of
that structure even if it shares cacheline with the buffer. You might issue
multiple URBs with transfer buffers within same cacheline. I would expect that
to be acceptable or URB documentation should say something against such.

 
 Looks you need to know why the memory corruption happens. Is it caused
 by non-aligned arch mapping/unmapping? or by host controller hardware when
 dealing with non-aligned transfer buffer?
 
 The warning given just before such unaligned buffer is passed to 
 dma_map_single,
 which requires ARCH_DMA_MINALIGN alignment. This seems reasonable to me.
 
 

Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Jussi Kivilinna
On 15.06.2013 16:10, Jussi Kivilinna wrote:
[...]




 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Not only rtl8192cu driver, all USB network device drivers have the problem.


 Patch makes this issue more visible at core level, and hopefully gives 
 hint
 for future hcd driver implementors about this problem.

 So please find the root cause first, and don't add the noise now.

 I think the root cause is that host driver is letting pass non-aligned 
 buffers
 to core on archs that have ARCH_DMA_MINALIGN set.

 No, I don't think so, about the problem, the dma alignment requirement should
 be from your host controller.

 As I said above, dma mapping/unmapping should be capable of dealing with
 the unaligned buffer if no one touches memory which shares cacheline with
 URB-transfer_buffer during URB transfer.
 
 How can you guarantee that when you allow unaligned URB buffers?
 
 You can have the buffer as part of some larger structure and send out async 
 URB.
 Then while buffer is DMA mapped and send async to hw, you use other parts of
 that structure even if it shares cacheline with the buffer. You might issue
 multiple URBs with transfer buffers within same cacheline. I would expect that
 to be acceptable or URB documentation should say something against such.
 

Hm.. rethink this a bit.

Transfer buffer might be dma aligned but shorter than cacheline and end of 
cacheline
used as something else. Manual alignment by host driver does not catch that
or fix that.
So, yes.. dma mapping should work with unaligned buffers, but maybe the actual
problem is multiple buffers from same cacheline.

-Jussi


--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-15 Thread Ming Lei
On Sat, Jun 15, 2013 at 9:10 PM, Jussi Kivilinna jussi.kivili...@iki.fi wrote:
 On 15.06.2013 15:07, Ming Lei wrote:
 On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 On 15.06.2013 10:41, Ming Lei wrote:
 Cc: netdev

 On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna jussi.kivili...@iki.fi 
 wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment 
 for

 Looks these host drivers have to face the fact that the transfer buffer is 
 often
 DMA non-aligned from network device drivers(in fact, the buffer is from
 network protocol stack), if you run usbnet, then you will get the added 
 warning
 immediately.


 Yes, getting warning immediately, but once, and blaming host driver seems 
 ok.

 We do know the fact of non-aligned transfer buffer from network, which has 
 been
 for long time, so does it make sense to print warning and annoy people?

 It's only printed if host controller driver is not behaving correctly.

If you make sure the warning is only printed on broken controller,
that is fine.


 I have changed the message to be printed for v2-patch, and it is now:
 dev_WARN_ONCE(hcd-self.controller, 1,
 broken USB host controller driver; does not correctly handle DMA 
 alignment for urb-transfer_buffer (offset: %d).\n,
 dma_offset);

 I sent the patch as RFC since I'm not sure.. maybe annoying warnings make

That is fine.

 people aware of issues that they don't yet know of and things get fixed?

I mean it isn't good to annoy people who are using good host controller, :-)



 URB buffers and let core/hcd.c to do the mapping on architectures that 
 have
 minimum DMA alignment requirements. This leads to random memory 
 corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.

 Maybe you should check the dma mapping/unmapping implementation of
 the arch, non-aligned buffer should have be covered by the API easily.

 Also USB Host controller should have supported non-aligned DMA buffer.

 From what I found, there was some discussion about these issues around 2010:
  
 http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022983.html

 From the discussion,  people think that HCD should handle the unaligned 
 buffer,
 right?

 Yes, that's how I understood it.



 To me, it seems that non-aligned buffers cannot be easily handled by all 
 archs
 at dma mapping/unmapping phase and that HCD driver should do the alignment 
 on

 If the memory which shares cache line with transfer buffer can't be
 accessed during
 DMA transfer(between URB submit and complete), dma mapping/unmapping
 should have handled it.

 About the network transfer buffer case, I think it should be true,
 otherwise there
 should have lots of memory corruption reports about usb network drivers.
 Fortunately, there are seldom such reports.


 Another reason why rtl8192cu is so hard, is that it uses pre-allocated array
 for buffers of multiple URBs, and more than one transfer buffer can reside on
 same cache line.

If so, that should be bug inside rtl8192, and more than one transfer
buffer shares
one cache line should be avoided, I understand the buffer isn't from
network stack,
don't I?


 archs that set ARCH_DMA_MINALIGN. For example, ehci_tegra does copy 
 unaligned
 transfer buffers to temporary aligned buffers before letting them to USB 
 core.

 Yes, if host controller can't handle this, the HCD has to work around
 the problem. Anyway, most of host controllers can deal with the it,
 can't they?

 Can they? Maybe they can handle most cases of unaligned buffers, but not some
 corner cases, like transfer buffers on same cache line.

Of course, most of in-tree host controller can handle non-aligned buffer.

If transfer buffers share one same cache line, it should be bug in driver,
not fault of host controller.






 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).

 Not only rtl8192cu driver, all USB network device drivers have the problem.


 Patch makes this issue more visible at core level, and hopefully gives 
 hint
 for future hcd driver implementors about this problem.

 So please find the root cause first, and don't add the noise now.

 I think the root cause is that host driver is letting pass non-aligned 
 buffers
 to core on archs that have ARCH_DMA_MINALIGN set.

 No, I don't think so, about the problem, the dma alignment requirement should
 be from your host controller.

 As I said above, dma mapping/unmapping should be capable of dealing with
 the unaligned buffer if no one touches memory which shares cacheline with
 URB-transfer_buffer during URB transfer.

 How can you guarantee that when you allow unaligned URB buffers?

As far as the network driver is concerned, the network stack should guarantee
memory shared cacheline with skb-data won't 

Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-14 Thread Greg Kroah-Hartman
On Fri, Jun 14, 2013 at 04:38:03PM +0300, Jussi Kivilinna wrote:
 Appearently some out-of-tree USB host drivers do not handle DMA alignment for
 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.
 
 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).
 
 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.

You should be more forceful in your warning messages to blame the host
controller, as it is, if I was a user and I saw:

 + WARN_ONCE(1, urb-setup_packet not DMA 
 aligned.\n);

I would think that the device driver was still the problem, not the host
controller.

thanks,

greg k-h
--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-14 Thread Greg Kroah-Hartman
On Fri, Jun 14, 2013 at 08:24:24PM +0300, Jussi Kivilinna wrote:
 On 14.06.2013 19:05, Greg Kroah-Hartman wrote:
  On Fri, Jun 14, 2013 at 04:38:03PM +0300, Jussi Kivilinna wrote:
  Appearently some out-of-tree USB host drivers do not handle DMA alignment 
  for
  URB buffers and let core/hcd.c to do the mapping on architectures that have
  minimum DMA alignment requirements. This leads to random memory corruptions
  and crashes when using USB device drivers that use unaligned URB buffers.
 
  Instead of fixing host drivers, users end up posting bug reports against
  those USB device drivers that use unaligned buffers for URB; such as with
  rtl8192cu 
  (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).
 
  Patch makes this issue more visible at core level, and hopefully gives hint
  for future hcd driver implementors about this problem.
  
  You should be more forceful in your warning messages to blame the host
  controller, as it is, if I was a user and I saw:
  
  +  WARN_ONCE(1, urb-setup_packet not DMA 
  aligned.\n);
  
  I would think that the device driver was still the problem, not the host
  controller.
 
 Ok. Would this be better?
 
 Possibly broken USB host controller driver; does not correctly handle DMA 
 alignment for urb-blabla.\n
 
 ... or should that 'Possibly' be dropped and just say broken USB 

Yes, just say broken, and give the device id of the broken USB
controller (I think you have a pointer to it somewhere at that time,
right?)

thanks,

greg k-h
--
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: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped

2013-06-14 Thread Alan Stern
On Fri, 14 Jun 2013, Jussi Kivilinna wrote:

 Appearently some out-of-tree USB host drivers do not handle DMA alignment for
 URB buffers and let core/hcd.c to do the mapping on architectures that have
 minimum DMA alignment requirements. This leads to random memory corruptions
 and crashes when using USB device drivers that use unaligned URB buffers.
 
 Instead of fixing host drivers, users end up posting bug reports against
 those USB device drivers that use unaligned buffers for URB; such as with
 rtl8192cu 
 (http://thread.gmane.org/gmane.linux.kernel.wireless.general/105631).
 
 Patch makes this issue more visible at core level, and hopefully gives hint
 for future hcd driver implementors about this problem.
 
 Signed-off-by: Jussi Kivilinna jussi.kivili...@iki.fi

It's probably okay to leave out the test for the setup buffer.  That 
thing is only 8 bytes long and is unlikely to be misaligned.  The 
transfer buffer is the one that matters.

Also, the error message should include the name of the host controller 
driver.  How about using dev_WARN_ONCE(hcd-self.controller, ...)?

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