* [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context
@ 2018-08-04 4:01 Guenter Roeck
2018-08-04 14:50 ` Alan Stern
2018-08-06 8:37 ` Christoph Hellwig
0 siblings, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2018-08-04 4:01 UTC (permalink / raw)
To: Alan Stern; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Guenter Roeck
Testing an USB drive connected to ohci-sm501 results in a large number
of runtime warnings.
WARNING: CPU: 0 PID: 0 at ./include/linux/dma-mapping.h:541
hcd_buffer_free+0x148/0x178
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc7-00014-g7ec386e4c991-dirty
PC is at hcd_buffer_free+0x148/0x178
PR is at hcd_buffer_free+0x66/0x178
PC : 8c26cbb0 SP : 8c481da8 SR : 400080f1
TEA : c00c8fe0
R0 : 000000f0 R1 : 000000f0 R2 : 8f9bb890 R3 : 00000000
R4 : 8f9c8800 R5 : 00001004 R6 : b07c6000 R7 : 007c6000
R8 : 00001004 R9 : 8f9bb814 R10 : 8c388104 R11 : 007c6000
R12 : b07c6000 R13 : 8f875680 R14 : 00000000
MACH: 000002fe MACL: 0000017c GBR : 00000000 PR : 8c26cace
Call trace:
[<(ptrval)>] usb_hcd_unmap_urb_for_dma+0xf4/0x13c
[<(ptrval)>] arch_local_save_flags+0x0/0x8
[<(ptrval)>] __usb_hcd_giveback_urb+0x2e/0xdc
[<(ptrval)>] arch_local_save_flags+0x0/0x8
[<(ptrval)>] finish_urb+0x8a/0x164
[<(ptrval)>] arch_local_save_flags+0x0/0x8
[<(ptrval)>] printk+0x0/0x48
[<(ptrval)>] ohci_work.part.11+0x150/0x41c
[<(ptrval)>] td_done.isra.4+0x0/0x11c
[<(ptrval)>] vprintk_default+0x14/0x20
[<(ptrval)>] arch_local_save_flags+0x0/0x8
[<(ptrval)>] ohci_irq+0x20c/0x314
[<(ptrval)>] usb_hcd_irq+0x16/0x28
Code analysis shows that interrupts are indeed disabled in ohci_irq().
Handle the situation by setting the HCD_BH flag in the ohci-sm501 driver.
With this flag set, urbs are released in a tasklet and not by the
interrupt handler.
Fixes: f54aab6ebcecd ("usb: ohci-sm501 driver")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/usb/host/ohci-sm501.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index c9233cddf9a2..eeb5b3137cf2 100644
--- a/drivers/usb/host/ohci-sm501.c
+++ b/drivers/usb/host/ohci-sm501.c
@@ -49,7 +49,7 @@ static const struct hc_driver ohci_sm501_hc_driver = {
* generic hardware linkage
*/
.irq = ohci_irq,
- .flags = HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM,
+ .flags = HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM | HCD_BH,
/*
* basic lifecycle operations
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-04 4:01 [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context Guenter Roeck @ 2018-08-04 14:50 ` Alan Stern 2018-08-04 19:19 ` Guenter Roeck 2018-08-06 8:37 ` Christoph Hellwig 1 sibling, 1 reply; 10+ messages in thread From: Alan Stern @ 2018-08-04 14:50 UTC (permalink / raw) To: Guenter Roeck; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel On Fri, 3 Aug 2018, Guenter Roeck wrote: > Testing an USB drive connected to ohci-sm501 results in a large number > of runtime warnings. > > WARNING: CPU: 0 PID: 0 at ./include/linux/dma-mapping.h:541 > hcd_buffer_free+0x148/0x178 > Modules linked in: > > CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc7-00014-g7ec386e4c991-dirty > PC is at hcd_buffer_free+0x148/0x178 > PR is at hcd_buffer_free+0x66/0x178 > PC : 8c26cbb0 SP : 8c481da8 SR : 400080f1 > TEA : c00c8fe0 > R0 : 000000f0 R1 : 000000f0 R2 : 8f9bb890 R3 : 00000000 > R4 : 8f9c8800 R5 : 00001004 R6 : b07c6000 R7 : 007c6000 > R8 : 00001004 R9 : 8f9bb814 R10 : 8c388104 R11 : 007c6000 > R12 : b07c6000 R13 : 8f875680 R14 : 00000000 > MACH: 000002fe MACL: 0000017c GBR : 00000000 PR : 8c26cace > > Call trace: > [<(ptrval)>] usb_hcd_unmap_urb_for_dma+0xf4/0x13c > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > [<(ptrval)>] __usb_hcd_giveback_urb+0x2e/0xdc > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > [<(ptrval)>] finish_urb+0x8a/0x164 > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > [<(ptrval)>] printk+0x0/0x48 > [<(ptrval)>] ohci_work.part.11+0x150/0x41c > [<(ptrval)>] td_done.isra.4+0x0/0x11c > [<(ptrval)>] vprintk_default+0x14/0x20 > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > [<(ptrval)>] ohci_irq+0x20c/0x314 > [<(ptrval)>] usb_hcd_irq+0x16/0x28 > > Code analysis shows that interrupts are indeed disabled in ohci_irq(). > Handle the situation by setting the HCD_BH flag in the ohci-sm501 driver. > With this flag set, urbs are released in a tasklet and not by the > interrupt handler. > > Fixes: f54aab6ebcecd ("usb: ohci-sm501 driver") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Unfortunately, one must not simply turn on this flag. Doing so will violate some of the documented requirements for scheduling of periodic transfers. Significantly deeper changes to the OHCI driver are necessary before the flag is set. Alan Stern > --- > drivers/usb/host/ohci-sm501.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c > index c9233cddf9a2..eeb5b3137cf2 100644 > --- a/drivers/usb/host/ohci-sm501.c > +++ b/drivers/usb/host/ohci-sm501.c > @@ -49,7 +49,7 @@ static const struct hc_driver ohci_sm501_hc_driver = { > * generic hardware linkage > */ > .irq = ohci_irq, > - .flags = HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM, > + .flags = HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM | HCD_BH, > > /* > * basic lifecycle operations > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-04 14:50 ` Alan Stern @ 2018-08-04 19:19 ` Guenter Roeck 2018-08-05 18:31 ` Alan Stern 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-08-04 19:19 UTC (permalink / raw) To: Alan Stern; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel On Sat, Aug 04, 2018 at 10:50:15AM -0400, Alan Stern wrote: > On Fri, 3 Aug 2018, Guenter Roeck wrote: > > > Testing an USB drive connected to ohci-sm501 results in a large number > > of runtime warnings. > > > > WARNING: CPU: 0 PID: 0 at ./include/linux/dma-mapping.h:541 > > hcd_buffer_free+0x148/0x178 > > Modules linked in: > > > > CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc7-00014-g7ec386e4c991-dirty > > PC is at hcd_buffer_free+0x148/0x178 > > PR is at hcd_buffer_free+0x66/0x178 > > PC : 8c26cbb0 SP : 8c481da8 SR : 400080f1 > > TEA : c00c8fe0 > > R0 : 000000f0 R1 : 000000f0 R2 : 8f9bb890 R3 : 00000000 > > R4 : 8f9c8800 R5 : 00001004 R6 : b07c6000 R7 : 007c6000 > > R8 : 00001004 R9 : 8f9bb814 R10 : 8c388104 R11 : 007c6000 > > R12 : b07c6000 R13 : 8f875680 R14 : 00000000 > > MACH: 000002fe MACL: 0000017c GBR : 00000000 PR : 8c26cace > > > > Call trace: > > [<(ptrval)>] usb_hcd_unmap_urb_for_dma+0xf4/0x13c > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > [<(ptrval)>] __usb_hcd_giveback_urb+0x2e/0xdc > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > [<(ptrval)>] finish_urb+0x8a/0x164 > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > [<(ptrval)>] printk+0x0/0x48 > > [<(ptrval)>] ohci_work.part.11+0x150/0x41c > > [<(ptrval)>] td_done.isra.4+0x0/0x11c > > [<(ptrval)>] vprintk_default+0x14/0x20 > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > [<(ptrval)>] ohci_irq+0x20c/0x314 > > [<(ptrval)>] usb_hcd_irq+0x16/0x28 > > > > Code analysis shows that interrupts are indeed disabled in ohci_irq(). > > Handle the situation by setting the HCD_BH flag in the ohci-sm501 driver. > > With this flag set, urbs are released in a tasklet and not by the > > interrupt handler. > > > > Fixes: f54aab6ebcecd ("usb: ohci-sm501 driver") > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > Unfortunately, one must not simply turn on this flag. Doing so will > violate some of the documented requirements for scheduling of periodic > transfers. Significantly deeper changes to the OHCI driver are > necessary before the flag is set. > Well, it was worth a try. Note that I did try to figure out if there are any pitfalls when setting HCD_BH, but I didn't find anything. Reminds me of Hitchhiker through the Galaxy for some reason. Cheers, Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-04 19:19 ` Guenter Roeck @ 2018-08-05 18:31 ` Alan Stern 2018-08-05 21:38 ` Guenter Roeck 0 siblings, 1 reply; 10+ messages in thread From: Alan Stern @ 2018-08-05 18:31 UTC (permalink / raw) To: Guenter Roeck; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel On Sat, 4 Aug 2018, Guenter Roeck wrote: > On Sat, Aug 04, 2018 at 10:50:15AM -0400, Alan Stern wrote: > > On Fri, 3 Aug 2018, Guenter Roeck wrote: > > > > > Testing an USB drive connected to ohci-sm501 results in a large number > > > of runtime warnings. > > > > > > WARNING: CPU: 0 PID: 0 at ./include/linux/dma-mapping.h:541 > > > hcd_buffer_free+0x148/0x178 > > > Modules linked in: > > > > > > CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc7-00014-g7ec386e4c991-dirty > > > PC is at hcd_buffer_free+0x148/0x178 > > > PR is at hcd_buffer_free+0x66/0x178 > > > PC : 8c26cbb0 SP : 8c481da8 SR : 400080f1 > > > TEA : c00c8fe0 > > > R0 : 000000f0 R1 : 000000f0 R2 : 8f9bb890 R3 : 00000000 > > > R4 : 8f9c8800 R5 : 00001004 R6 : b07c6000 R7 : 007c6000 > > > R8 : 00001004 R9 : 8f9bb814 R10 : 8c388104 R11 : 007c6000 > > > R12 : b07c6000 R13 : 8f875680 R14 : 00000000 > > > MACH: 000002fe MACL: 0000017c GBR : 00000000 PR : 8c26cace > > > > > > Call trace: > > > [<(ptrval)>] usb_hcd_unmap_urb_for_dma+0xf4/0x13c > > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > > [<(ptrval)>] __usb_hcd_giveback_urb+0x2e/0xdc > > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > > [<(ptrval)>] finish_urb+0x8a/0x164 > > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > > [<(ptrval)>] printk+0x0/0x48 > > > [<(ptrval)>] ohci_work.part.11+0x150/0x41c > > > [<(ptrval)>] td_done.isra.4+0x0/0x11c > > > [<(ptrval)>] vprintk_default+0x14/0x20 > > > [<(ptrval)>] arch_local_save_flags+0x0/0x8 > > > [<(ptrval)>] ohci_irq+0x20c/0x314 > > > [<(ptrval)>] usb_hcd_irq+0x16/0x28 > > > > > > Code analysis shows that interrupts are indeed disabled in ohci_irq(). > > > Handle the situation by setting the HCD_BH flag in the ohci-sm501 driver. > > > With this flag set, urbs are released in a tasklet and not by the > > > interrupt handler. > > > > > > Fixes: f54aab6ebcecd ("usb: ohci-sm501 driver") > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > Unfortunately, one must not simply turn on this flag. Doing so will > > violate some of the documented requirements for scheduling of periodic > > transfers. Significantly deeper changes to the OHCI driver are > > necessary before the flag is set. > > > > Well, it was worth a try. Note that I did try to figure out if there are > any pitfalls when setting HCD_BH, but I didn't find anything. Reminds me > of Hitchhiker through the Galaxy for some reason. The interactions are pretty subtle, and the descriptions are hidden in the kerneldoc for usb_submit_urb and usb_unlink_urb. ehci-hcd, for example, required a number of non-obvious changes when it was converted to use HCD_BH. If anyone wants to write something similar for ohci-hcd I'll be happy to review it, but I don't want to write it myself. (Besides, speeding up the time spent in interrupt handling seems less urgent for OHCI, which runs much slower than EHCI and is not getting used very much in new hardware.) Another way to attack this problem would be to allow DMA unmapping in interrupt context. I have never understood why DMA mapping is okay with this but unmapping isn't. Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-05 18:31 ` Alan Stern @ 2018-08-05 21:38 ` Guenter Roeck 2018-08-06 8:33 ` Christoph Hellwig 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-08-05 21:38 UTC (permalink / raw) To: Alan Stern; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Christoph Hellwig On 08/05/2018 11:31 AM, Alan Stern wrote: > On Sat, 4 Aug 2018, Guenter Roeck wrote: > >> On Sat, Aug 04, 2018 at 10:50:15AM -0400, Alan Stern wrote: >>> On Fri, 3 Aug 2018, Guenter Roeck wrote: >>> >>>> Testing an USB drive connected to ohci-sm501 results in a large number >>>> of runtime warnings. >>>> >>>> WARNING: CPU: 0 PID: 0 at ./include/linux/dma-mapping.h:541 >>>> hcd_buffer_free+0x148/0x178 >>>> Modules linked in: >>>> >>>> CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc7-00014-g7ec386e4c991-dirty >>>> PC is at hcd_buffer_free+0x148/0x178 >>>> PR is at hcd_buffer_free+0x66/0x178 >>>> PC : 8c26cbb0 SP : 8c481da8 SR : 400080f1 >>>> TEA : c00c8fe0 >>>> R0 : 000000f0 R1 : 000000f0 R2 : 8f9bb890 R3 : 00000000 >>>> R4 : 8f9c8800 R5 : 00001004 R6 : b07c6000 R7 : 007c6000 >>>> R8 : 00001004 R9 : 8f9bb814 R10 : 8c388104 R11 : 007c6000 >>>> R12 : b07c6000 R13 : 8f875680 R14 : 00000000 >>>> MACH: 000002fe MACL: 0000017c GBR : 00000000 PR : 8c26cace >>>> >>>> Call trace: >>>> [<(ptrval)>] usb_hcd_unmap_urb_for_dma+0xf4/0x13c >>>> [<(ptrval)>] arch_local_save_flags+0x0/0x8 >>>> [<(ptrval)>] __usb_hcd_giveback_urb+0x2e/0xdc >>>> [<(ptrval)>] arch_local_save_flags+0x0/0x8 >>>> [<(ptrval)>] finish_urb+0x8a/0x164 >>>> [<(ptrval)>] arch_local_save_flags+0x0/0x8 >>>> [<(ptrval)>] printk+0x0/0x48 >>>> [<(ptrval)>] ohci_work.part.11+0x150/0x41c >>>> [<(ptrval)>] td_done.isra.4+0x0/0x11c >>>> [<(ptrval)>] vprintk_default+0x14/0x20 >>>> [<(ptrval)>] arch_local_save_flags+0x0/0x8 >>>> [<(ptrval)>] ohci_irq+0x20c/0x314 >>>> [<(ptrval)>] usb_hcd_irq+0x16/0x28 >>>> >>>> Code analysis shows that interrupts are indeed disabled in ohci_irq(). >>>> Handle the situation by setting the HCD_BH flag in the ohci-sm501 driver. >>>> With this flag set, urbs are released in a tasklet and not by the >>>> interrupt handler. >>>> >>>> Fixes: f54aab6ebcecd ("usb: ohci-sm501 driver") >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> >>> Unfortunately, one must not simply turn on this flag. Doing so will >>> violate some of the documented requirements for scheduling of periodic >>> transfers. Significantly deeper changes to the OHCI driver are >>> necessary before the flag is set. >>> >> >> Well, it was worth a try. Note that I did try to figure out if there are >> any pitfalls when setting HCD_BH, but I didn't find anything. Reminds me >> of Hitchhiker through the Galaxy for some reason. > > The interactions are pretty subtle, and the descriptions are hidden in > the kerneldoc for usb_submit_urb and usb_unlink_urb. ehci-hcd, for > example, required a number of non-obvious changes when it was converted > to use HCD_BH. If anyone wants to write something similar for > ohci-hcd I'll be happy to review it, but I don't want to write it > myself. (Besides, speeding up the time spent in interrupt handling Not me. I stumbled over the problem while enhancing my qemu boot tests, after attaching a USB drive to the sm501-ohci controller on a virtual sh4 system. Who knows if that system is still used in the real world and, if it does, if it runs a recent kernel. Worth a quick fix, but not a major code overhaul - even more so without access to real hardware. > seems less urgent for OHCI, which runs much slower than EHCI and is > not getting used very much in new hardware.) > > Another way to attack this problem would be to allow DMA unmapping in > interrupt context. I have never understood why DMA mapping is okay > with this but unmapping isn't. > AFAICS it used to be interrupt tolerant for all but x86 up to commit 6894258eda ("dma-mapping: consolidate dma_{alloc,free}_{attrs,coherent}"). A quick test shows that the warning is indeed not seen if I run my test on v3.18.y. You would have to ask Christoph why it is now interrupt-intolerant for all architectures. Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-05 21:38 ` Guenter Roeck @ 2018-08-06 8:33 ` Christoph Hellwig 2018-08-06 15:58 ` Guenter Roeck 0 siblings, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2018-08-06 8:33 UTC (permalink / raw) To: Guenter Roeck Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel, Christoph Hellwig On Sun, Aug 05, 2018 at 02:38:22PM -0700, Guenter Roeck wrote: > AFAICS it used to be interrupt tolerant for all but x86 up to commit > 6894258eda ("dma-mapping: consolidate dma_{alloc,free}_{attrs,coherent}"). > A quick test shows that the warning is indeed not seen if I run my test > on v3.18.y. > > You would have to ask Christoph why it is now interrupt-intolerant for all > architectures. interrupt-tolerant actually is a very odd wording. x86 itself (at least unless using an iommu) actually is fine with calling dma_free_* with interrupts disabled, but many other architectures are not. For many architectures that are not cache coherent we'll have to manipulale the kernel page tables when freeing coherent memory, which we absolutely can't do from irq context. And as usual in the kernel we try to enforce the same rules on everyone to allow for portable code. Also except for the odd USB use cases that abuses dma_alloc_coherent for the bounce buffers in device local memory you are doing something wrong if you call dma_alloc*/dma_free* anywhere but during device initialization and removal. These are slow path interfaces for allocating memory for things like descriptors and similar. Your fast path should be using dma_map*/dma_unmap*. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-06 8:33 ` Christoph Hellwig @ 2018-08-06 15:58 ` Guenter Roeck 0 siblings, 0 replies; 10+ messages in thread From: Guenter Roeck @ 2018-08-06 15:58 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel On 08/06/2018 01:33 AM, Christoph Hellwig wrote: > On Sun, Aug 05, 2018 at 02:38:22PM -0700, Guenter Roeck wrote: >> AFAICS it used to be interrupt tolerant for all but x86 up to commit >> 6894258eda ("dma-mapping: consolidate dma_{alloc,free}_{attrs,coherent}"). >> A quick test shows that the warning is indeed not seen if I run my test >> on v3.18.y. >> >> You would have to ask Christoph why it is now interrupt-intolerant for all >> architectures. > > interrupt-tolerant actually is a very odd wording. x86 itself (at least > unless using an iommu) actually is fine with calling dma_free_* with > interrupts disabled, but many other architectures are not. For many > architectures that are not cache coherent we'll have to manipulale > the kernel page tables when freeing coherent memory, which we absolutely > can't do from irq context. And as usual in the kernel we try to enforce > the same rules on everyone to allow for portable code. > > Also except for the odd USB use cases that abuses dma_alloc_coherent > for the bounce buffers in device local memory you are doing something > wrong if you call dma_alloc*/dma_free* anywhere but during device > initialization and removal. These are slow path interfaces for > allocating memory for things like descriptors and similar. Your fast > path should be using dma_map*/dma_unmap*. > FWIW, that isn't "my" code. I just had the impunity to test it. Cheers, Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-04 4:01 [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context Guenter Roeck 2018-08-04 14:50 ` Alan Stern @ 2018-08-06 8:37 ` Christoph Hellwig 2018-08-06 16:03 ` Guenter Roeck 1 sibling, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2018-08-06 8:37 UTC (permalink / raw) To: Guenter Roeck; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel On Fri, Aug 03, 2018 at 09:01:19PM -0700, Guenter Roeck wrote: > Testing an USB drive connected to ohci-sm501 results in a large number > of runtime warnings. As far as I can tell this driver uses the HCD_LOCAL_MEM feature flag for memory declared using dma_declare_coherent_memory. Unlike the mormal dma mapping interfaces this special case can actually be freed from interrupt context, and we have a fix for this warning queued up in linux-next: http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/d27fb99f62af7b79c542d161aa5155ed57271ddc That being said I'm generally very unhappy how dma_init_coherent_memory turned out. The idea was to allow device local memory to be hidden behind the DMA API, but in general we use it either as a way to declare special uncache system memory (which would really be the plaform codes job), or as a magic bounce buffer like in the USB code. I plan to eventually untangle this, but it is going to take some time. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-06 8:37 ` Christoph Hellwig @ 2018-08-06 16:03 ` Guenter Roeck 2018-08-07 7:25 ` Christoph Hellwig 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-08-06 16:03 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel On 08/06/2018 01:37 AM, Christoph Hellwig wrote: > On Fri, Aug 03, 2018 at 09:01:19PM -0700, Guenter Roeck wrote: >> Testing an USB drive connected to ohci-sm501 results in a large number >> of runtime warnings. > > As far as I can tell this driver uses the HCD_LOCAL_MEM feature flag > for memory declared using dma_declare_coherent_memory. Unlike the > mormal dma mapping interfaces this special case can actually be freed > from interrupt context, and we have a fix for this warning queued > up in linux-next: > > http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/d27fb99f62af7b79c542d161aa5155ed57271ddc > > That being said I'm generally very unhappy how dma_init_coherent_memory > turned out. The idea was to allow device local memory to be hidden > behind the DMA API, but in general we use it either as a way to declare > special uncache system memory (which would really be the plaform codes > job), or as a magic bounce buffer like in the USB code. I plan to > eventually untangle this, but it is going to take some time. > Yes, I can confirm that the warning is gone in -next. Problem solved, except of course there are still the warnings about the missing coherent_dma_mask. sm501-usb sm501-usb: SM501 OHCI sm501-usb sm501-usb: new USB bus registered, assigned bus number 1 WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 ohci_init+0x194/0x2d8 Is that warning also not warranted for the given use case, or is the missing mask indeed necessary ? It is easy to add - see https://lore.kernel.org/patchwork/patch/971411/ - but I do wonder if that change is appropriate. Thanks, Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context 2018-08-06 16:03 ` Guenter Roeck @ 2018-08-07 7:25 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2018-08-07 7:25 UTC (permalink / raw) To: Guenter Roeck Cc: Christoph Hellwig, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, Aug 06, 2018 at 09:03:35AM -0700, Guenter Roeck wrote: > Yes, I can confirm that the warning is gone in -next. Problem solved, > except of course there are still the warnings about the missing > coherent_dma_mask. > > sm501-usb sm501-usb: SM501 OHCI > sm501-usb sm501-usb: new USB bus registered, assigned bus number 1 > WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 ohci_init+0x194/0x2d8 > > Is that warning also not warranted for the given use case, or is > the missing mask indeed necessary ? It is easy to add - see > https://lore.kernel.org/patchwork/patch/971411/ - but I do wonder > if that change is appropriate. Yes, every driver using dma coherent functions should set a coherent mask. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-08-07 7:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-04 4:01 [PATCH] USB: OHCI: ohci-sm501: complete URBs in BH context Guenter Roeck 2018-08-04 14:50 ` Alan Stern 2018-08-04 19:19 ` Guenter Roeck 2018-08-05 18:31 ` Alan Stern 2018-08-05 21:38 ` Guenter Roeck 2018-08-06 8:33 ` Christoph Hellwig 2018-08-06 15:58 ` Guenter Roeck 2018-08-06 8:37 ` Christoph Hellwig 2018-08-06 16:03 ` Guenter Roeck 2018-08-07 7:25 ` Christoph Hellwig
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).