* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped [not found] ` <20130614133803.25747.98705.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org> @ 2013-06-15 7:41 ` Ming Lei 2013-06-15 10:19 ` Jussi Kivilinna 0 siblings, 1 reply; 11+ messages in thread From: Ming Lei @ 2013-06-15 7:41 UTC (permalink / raw) To: Jussi Kivilinna Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman, Network Development Cc: netdev On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna <jussi.kivilinna-X3B1VOXEql0@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-15 7:41 ` [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped Ming Lei @ 2013-06-15 10:19 ` Jussi Kivilinna [not found] ` <51BC3F9E.3010605-X3B1VOXEql0@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-15 10:19 UTC (permalink / raw) To: Ming Lei; +Cc: linux-usb, Greg Kroah-Hartman, Network Development On 15.06.2013 10:41, Ming Lei wrote: > Cc: netdev > > On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna <jussi.kivilinna@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, > ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <51BC3F9E.3010605-X3B1VOXEql0@public.gmane.org>]
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped [not found] ` <51BC3F9E.3010605-X3B1VOXEql0@public.gmane.org> @ 2013-06-15 12:07 ` Ming Lei [not found] ` <CACVXFVMe9fgdiDTRC0rWvwZJM8aT7AZY8Q1MwiOTc4ks0PQPOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Ming Lei @ 2013-06-15 12:07 UTC (permalink / raw) To: Jussi Kivilinna Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman, Network Development On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna <jussi.kivilinna-X3B1VOXEql0@public.gmane.org> wrote: > On 15.06.2013 10:41, Ming Lei wrote: >> Cc: netdev >> >> On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna <jussi.kivilinna-X3B1VOXEql0@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CACVXFVMe9fgdiDTRC0rWvwZJM8aT7AZY8Q1MwiOTc4ks0PQPOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped [not found] ` <CACVXFVMe9fgdiDTRC0rWvwZJM8aT7AZY8Q1MwiOTc4ks0PQPOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2013-06-15 13:10 ` Jussi Kivilinna 2013-06-15 13:22 ` Jussi Kivilinna 2013-06-15 13:47 ` Ming Lei 0 siblings, 2 replies; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-15 13:10 UTC (permalink / raw) To: Ming Lei Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman, Network Development On 15.06.2013 15:07, Ming Lei wrote: > On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna <jussi.kivilinna-X3B1VOXEql0@public.gmane.org> wrote: >> On 15.06.2013 10:41, Ming Lei wrote: >>> Cc: netdev >>> >>> On Fri, Jun 14, 2013 at 9:38 PM, Jussi Kivilinna <jussi.kivilinna-X3B1VOXEql0@public.gmane.org> 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. > > 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. > Yes, that is the message I'm trying to make visible so that host drivers, that don't handle such, get fixed. -Jussi > > Thanks, > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-15 13:10 ` Jussi Kivilinna @ 2013-06-15 13:22 ` Jussi Kivilinna [not found] ` <51BC6A96.7030707-X3B1VOXEql0@public.gmane.org> 2013-06-15 13:47 ` Ming Lei 1 sibling, 1 reply; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-15 13:22 UTC (permalink / raw) To: Ming Lei; +Cc: linux-usb, Greg Kroah-Hartman, Network Development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <51BC6A96.7030707-X3B1VOXEql0@public.gmane.org>]
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped [not found] ` <51BC6A96.7030707-X3B1VOXEql0@public.gmane.org> @ 2013-06-16 8:21 ` Oliver Neukum 2013-06-16 10:35 ` Jussi Kivilinna 0 siblings, 1 reply; 11+ messages in thread From: Oliver Neukum @ 2013-06-16 8:21 UTC (permalink / raw) To: Jussi Kivilinna Cc: Ming Lei, linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman, Network Development 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-16 8:21 ` Oliver Neukum @ 2013-06-16 10:35 ` Jussi Kivilinna 2013-06-28 12:39 ` Jussi Kivilinna 0 siblings, 1 reply; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-16 10:35 UTC (permalink / raw) To: Oliver Neukum Cc: Ming Lei, linux-usb, Greg Kroah-Hartman, Network Development 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. -Jussi > > Regards > Oliver > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-16 10:35 ` Jussi Kivilinna @ 2013-06-28 12:39 ` Jussi Kivilinna 2013-06-28 12:58 ` Oliver Neukum 0 siblings, 1 reply; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-28 12:39 UTC (permalink / raw) To: linux-usb Cc: Oliver Neukum, Ming Lei, Greg Kroah-Hartman, Network Development 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 - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/ks959-sir.c#L172 * ksdazzle-sir: - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/ksdazzle-sir.c#L199 - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/ksdazzle-sir.c#L138 * wusbcore: - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/security.c#L568 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/wusbhc.h#L288 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/wa-xfer.c#L378 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/wa-xfer.c#L337 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/wa-xfer.c#L661 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/wusbcore/wa-xfer.c#L115 * mISDN/hfcsusb: - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.c#L1515 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.h#L247 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.c#L1071 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hardware/mISDN/hfcsusb.h#L236 * hisax/hfc_usb: - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L924 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L148 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L591 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/hisax/hfc_usb.c#L144 * rc/imon: - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L480 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L504 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L2148 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L2232 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L2452 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L2462 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/imon.c#L90 * catc: - tx_buf, rx_buf, irq_buf: http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/catc.c#L162 * hso: - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/hso.c#L1464 - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/hso.c#L208 * pegasus: - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.c#L1030 - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.h#L111 - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.c#L886 - http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/pegasus.h#L107 * brcmfmac: - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/brcm80211/brcmfmac/usb.c#L631 - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/brcm80211/brcmfmac/usb.c#L71 * staging/bcm: - http://lxr.linux.no/linux+v3.9.8/drivers/staging/bcm/InterfaceIsr.c#L125 - http://lxr.linux.no/linux+v3.9.8/drivers/staging/bcm/InterfaceAdapter.h#L63 * speedtch: - http://lxr.linux.no/linux+v3.9.8/drivers/usb/atm/speedtch.c#L888 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/atm/speedtch.c#L137 * gigaset: - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/gigaset/usb-gigaset.c#L467 - http://lxr.linux.no/linux+v3.9.8/drivers/isdn/gigaset/gigaset.h#L521 * ttusbir: - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/ttusbir.c#L300 - http://lxr.linux.no/linux+v3.9.8/drivers/media/rc/ttusbir.c#L48 * vub300: - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L467 - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L346 - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L491 - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L345 - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L1526 - http://lxr.linux.no/linux+v3.9.8/drivers/mmc/host/vub300.c#L355 * mcs7780: - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/mcs7780.c#L519 - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/mcs7780.c#L844 - http://lxr.linux.no/linux+v3.9.8/drivers/net/irda/mcs7780.h#L103 * rt2x00usb: - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2500usb.c#L1194 - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rt2x00/rt2x00usb.h#L387 * rtl8187: - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/dev.c#L551 - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h#L142 * zd1201: - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/zd1201.c#L817 - http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/zd1201.h#L42 * quatech2: - http://lxr.linux.no/linux+v3.9.8/drivers/usb/serial/quatech2.c#L769 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/serial/quatech2.c#L111 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/serial/quatech2.c#L841 - http://lxr.linux.no/linux+v3.9.8/drivers/usb/serial/quatech2.c#L118 * caiaq: - http://lxr.linux.no/linux+v3.9.8/sound/usb/caiaq/device.c#L425 - http://lxr.linux.no/linux+v3.9.8/sound/usb/caiaq/device.c#L430 - http://lxr.linux.no/linux+v3.9.8/sound/usb/caiaq/device.h#L77 - http://lxr.linux.no/linux+v3.9.8/sound/usb/caiaq/device.h#L79 URB transfer buffer in stack: * alauda: - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L226 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L210 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L291 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L275 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L342 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L325 - command: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L165 - oob: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L230 - oob: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L377 - oob: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L401 - oob: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L467 - oob: http://lxr.linux.no/linux+v3.9.8/drivers/mtd/nand/alauda.c#L295 * pn533: - ack: http://lxr.linux.no/linux+v3.9.8/drivers/nfc/pn533.c#L630 * i2c-tiny-usb: - func: http://lxr.linux.no/linux+v3.9.8/drivers/i2c/busses/i2c-tiny-usb.c#L116 - func: http://lxr.linux.no/linux+v3.9.8/drivers/i2c/busses/i2c-tiny-usb.c#L159 * zd1201: - ret: http://lxr.linux.no/linux+v3.9.8/drivers/net/wireless/zd1201.c#L100 * rndis_host: - notification: http://lxr.linux.no/linux+v3.9.8/drivers/net/usb/rndis_host.c#L143 * 6fire: - buffer: http://lxr.linux.no/linux+v3.9.8/sound/usb/6fire/comm.c#L113 - buffer: http://lxr.linux.no/linux+v3.9.8/sound/usb/6fire/comm.c#L122 - send_buffer: http://lxr.linux.no/linux+v3.9.8/sound/usb/6fire/comm.c#L96 -Jussi ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-28 12:39 ` Jussi Kivilinna @ 2013-06-28 12:58 ` Oliver Neukum 0 siblings, 0 replies; 11+ messages in thread From: Oliver Neukum @ 2013-06-28 12:58 UTC (permalink / raw) To: Jussi Kivilinna Cc: linux-usb, Ming Lei, Greg Kroah-Hartman, Network Development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-15 13:10 ` Jussi Kivilinna 2013-06-15 13:22 ` Jussi Kivilinna @ 2013-06-15 13:47 ` Ming Lei 2013-06-16 10:34 ` Jussi Kivilinna 1 sibling, 1 reply; 11+ messages in thread From: Ming Lei @ 2013-06-15 13:47 UTC (permalink / raw) To: Jussi Kivilinna; +Cc: linux-usb, Greg Kroah-Hartman, Network Development On Sat, Jun 15, 2013 at 9:10 PM, Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote: > On 15.06.2013 15:07, Ming Lei wrote: >> On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna <jussi.kivilinna@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.kivilinna@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 be accessed during transfer. > > You can have the buffer as part of some larger structure and send out async URB. That is bug in the driver, and not the situation I mentioned. I mean if the non-aligned buffer is skb->data, it should be OK. But if the buffer is allocated inside driver itself and not skb buffer, it is better to keep aligned since driver can do it. > 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. >> >> 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. >> > > Yes, that is the message I'm trying to make visible so that host drivers, > that don't handle such, get fixed. Please only print the warning on the host controller which can't deal with non-aligned buffer. > 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. That is bug in driver. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped 2013-06-15 13:47 ` Ming Lei @ 2013-06-16 10:34 ` Jussi Kivilinna 0 siblings, 0 replies; 11+ messages in thread From: Jussi Kivilinna @ 2013-06-16 10:34 UTC (permalink / raw) To: Ming Lei; +Cc: linux-usb, Greg Kroah-Hartman, Network Development On 15.06.2013 16:47, Ming Lei wrote: > On Sat, Jun 15, 2013 at 9:10 PM, Jussi Kivilinna <jussi.kivilinna@iki.fi> wrote: >> On 15.06.2013 15:07, Ming Lei wrote: >>> On Sat, Jun 15, 2013 at 6:19 PM, Jussi Kivilinna <jussi.kivilinna@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.kivilinna@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 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 be accessed during transfer. > >> >> You can have the buffer as part of some larger structure and send out async URB. > > That is bug in the driver, and not the situation I mentioned. > > I mean if the non-aligned buffer is skb->data, it should be OK. But if > the buffer is allocated inside driver itself and not skb buffer, it is better > to keep aligned since driver can do it. > >> 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. >>> >>> 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. >>> >> >> Yes, that is the message I'm trying to make visible so that host drivers, >> that don't handle such, get fixed. > > Please only print the warning on the host controller which can't deal with > non-aligned buffer. If non-aligned buffers work even if arch has ARCH_DMA_MINALIGN, then the patch would give false warnings. > >> 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. > > That is bug in driver. I'll have to look at the device driver more closely. Thanks. -Jussi > > > Thanks, > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-06-28 12:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20130614133803.25747.98705.stgit@localhost6.localdomain6>
[not found] ` <20130614133803.25747.98705.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
2013-06-15 7:41 ` [RFC PATCH] usb: hcd: warn about URB buffers that are not DMA aligned and are about to be DMA mapped Ming Lei
2013-06-15 10:19 ` Jussi Kivilinna
[not found] ` <51BC3F9E.3010605-X3B1VOXEql0@public.gmane.org>
2013-06-15 12:07 ` Ming Lei
[not found] ` <CACVXFVMe9fgdiDTRC0rWvwZJM8aT7AZY8Q1MwiOTc4ks0PQPOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-06-15 13:10 ` Jussi Kivilinna
2013-06-15 13:22 ` Jussi Kivilinna
[not found] ` <51BC6A96.7030707-X3B1VOXEql0@public.gmane.org>
2013-06-16 8:21 ` Oliver Neukum
2013-06-16 10:35 ` Jussi Kivilinna
2013-06-28 12:39 ` Jussi Kivilinna
2013-06-28 12:58 ` Oliver Neukum
2013-06-15 13:47 ` Ming Lei
2013-06-16 10:34 ` Jussi Kivilinna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).