Message ID | 20141205200357.GA1586@linutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: > Consider the following scenario: > - plugin a webcam > - play the stream via gst-launch-0.10 v4l2src device=/dev/video0… > - remove the USB-HCD during playback via "rmmod $HCD" > > and now wait for the crash Which you deserve, why did you ever remove a kernel module? That's racy and _never_ recommended, which is why it never happens automatically and only root can do it. > |musb-hdrc musb-hdrc.2.auto: remove, state 1 > |usb usb2: USB disconnect, device number 1 > |usb 2-1: USB disconnect, device number 3 > |uvcvideo: Failed to resubmit video URB (-19). > |musb-hdrc musb-hdrc.2.auto: USB bus 2 deregistered > |musb-hdrc musb-hdrc.1.auto: remove, state 4 > |usb usb1: USB disconnect, device number 1 > |musb-hdrc musb-hdrc.1.auto: USB bus 1 deregistered > |Unable to handle kernel paging request at virtual address 6b6b6b6f > |pgd = c0004000 > |[6b6b6b6f] *pgd=00000000 > |Internal error: Oops: 5 [#1] ARM > |Modules linked in: uvcvideo] > |CPU: 0 PID: 2613 Comm: gst-launch-0.10 Tainted: G W 3.14.25+ #40 > |task: ec2b8100 ti: ec38e000 task.ti: ec38e000 > |PC is at hcd_buffer_free+0x64/0xc0 > |LR is at uvc_free_urb_buffers+0x34/0x50 [uvcvideo] > |Process gst-launch-0.10 (pid: 2613, stack limit = 0xec38e240) > |[<c03a07e8>] (hcd_buffer_free) > |[<bf2f0c78>] (uvc_free_urb_buffers [uvcvideo]) > |[<bf2f33dc>] (uvc_video_enable [uvcvideo]) > |[<bf2ef150>] (uvc_v4l2_release [uvcvideo]) > |[<bf2ac318>] (v4l2_release [videodev]) > |[<c0103334>] (__fput) > |[<c005b618>] (task_work_run) > |[<c00417a0>] (do_exit) > |[<c0041ebc>] (do_group_exit) > > as part of the device-removal the HCD removes its dma-buffers, the HCD > structure itself and even the struct device is gone. That means if UVC > removes its URBs after its last user (/dev/videoX) is gone and not from > the ->disconnect() callback then it is too late because the HCD might > gone. > > First, I fixed by in the UVC driver by ignoring the UVC_SC_VIDEOSTREAMING > in its ->disconnect callback and calling uvc_video_enable(, 0) in > uvc_unregister_video(). But then I though that it might not be clever to > release that memory if there is userspace using it. > > So instead, I hold the device struct in the HCD and the HCD struct on > every USB-buf-alloc. That means after a disconnect we still have a > refcount on usb_hcd and device and it will be cleaned "later" once the > last USB-buffer is released. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > With this applied, I only see this three times (which is not new) > > | ------------[ cut here ]------------ > | WARNING: CPU: 0 PID: 1755 at fs/sysfs/group.c:219 sysfs_remove_group+0x88/0x98() > | sysfs group c08a70d4 not found for kobject 'event4' > | Modules linked in: uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core v4l2_common videodev ipv6 musb_hdrc udc_core us] > | CPU: 0 PID: 1755 Comm: gst-launch-0.10 Not tainted 3.18.0-rc7-00065-gabcefb342fbf-dirty #1813 > | [<c00152a8>] (unwind_backtrace) from [<c0011a48>] (show_stack+0x10/0x14) > | [<c0011a48>] (show_stack) from [<c056450c>] (dump_stack+0x80/0x9c) > | [<c056450c>] (dump_stack) from [<c00401a0>] (warn_slowpath_common+0x68/0x8c) > | [<c00401a0>] (warn_slowpath_common) from [<c0040258>] (warn_slowpath_fmt+0x30/0x40) > | [<c0040258>] (warn_slowpath_fmt) from [<c01af2a4>] (sysfs_remove_group+0x88/0x98) > | [<c01af2a4>] (sysfs_remove_group) from [<c039420c>] (device_del+0x34/0x198) > | [<c039420c>] (device_del) from [<c0428208>] (evdev_disconnect+0x18/0x44) > | [<c0428208>] (evdev_disconnect) from [<c04258c8>] (__input_unregister_device+0xa4/0x148) > | [<c04258c8>] (__input_unregister_device) from [<c04259ac>] (input_unregister_device+0x40/0x74) > | [<c04259ac>] (input_unregister_device) from [<bf0c6294>] (uvc_delete+0x20/0x10c [uvcvideo]) > | [<bf0c6294>] (uvc_delete [uvcvideo]) from [<bf08a68c>] (v4l2_device_release+0x9c/0xc4 [videodev]) > | [<bf08a68c>] (v4l2_device_release [videodev]) from [<c03934f0>] (device_release+0x2c/0x90) > | [<c03934f0>] (device_release) from [<c030f7bc>] (kobject_release+0x48/0x7c) > | [<c030f7bc>] (kobject_release) from [<bf089330>] (v4l2_release+0x50/0x78 [videodev]) > | [<bf089330>] (v4l2_release [videodev]) from [<c0147e34>] (__fput+0x80/0x1c4) > | [<c0147e34>] (__fput) from [<c0058a18>] (task_work_run+0xb4/0xe4) > | [<c0058a18>] (task_work_run) from [<c00425ec>] (do_exit+0x2dc/0x92c) > | [<c00425ec>] (do_exit) from [<c0042ca4>] (do_group_exit+0x3c/0xb0) > | [<c0042ca4>] (do_group_exit) from [<c0042d28>] (__wake_up_parent+0x0/0x18) > | ---[ end trace b54a8f3c8129180e ]--- > anyone an idea? > > drivers/usb/core/buffer.c | 30 +++++++++++++++++++++++++----- > drivers/usb/core/hcd.c | 2 ++ > 2 files changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c > index 506b969ea7fd..01e080a61519 100644 > --- a/drivers/usb/core/buffer.c > +++ b/drivers/usb/core/buffer.c > @@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd) > * better sharing and to leverage mm/slab.c intelligence. > */ > > -void *hcd_buffer_alloc( > +static void *_hcd_buffer_alloc( Looks like this isn't really needed here, right? > struct usb_bus *bus, > size_t size, > gfp_t mem_flags, > @@ -131,7 +131,19 @@ void *hcd_buffer_alloc( > return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags); > } > > -void hcd_buffer_free( > +void *hcd_buffer_alloc(struct usb_bus *bus, size_t size, gfp_t mem_flags, > + dma_addr_t *dma) > +{ > + struct usb_hcd *hcd = bus_to_hcd(bus); > + void *ret; > + > + ret = _hcd_buffer_alloc(bus, size, mem_flags, dma); > + if (ret) > + usb_get_hcd(hcd); I'm all for some good reference counting, but this is going to cause a _lot_ of churn on this reference count, what is the performance issue with doing this for every buffer? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 5 Dec 2014, Sebastian Andrzej Siewior wrote: > Consider the following scenario: > - plugin a webcam > - play the stream via gst-launch-0.10 v4l2src device=/dev/video0… > - remove the USB-HCD during playback via "rmmod $HCD" > > and now wait for the crash > > |musb-hdrc musb-hdrc.2.auto: remove, state 1 > |usb usb2: USB disconnect, device number 1 > |usb 2-1: USB disconnect, device number 3 > |uvcvideo: Failed to resubmit video URB (-19). > |musb-hdrc musb-hdrc.2.auto: USB bus 2 deregistered > |musb-hdrc musb-hdrc.1.auto: remove, state 4 > |usb usb1: USB disconnect, device number 1 > |musb-hdrc musb-hdrc.1.auto: USB bus 1 deregistered > |Unable to handle kernel paging request at virtual address 6b6b6b6f > |pgd = c0004000 > |[6b6b6b6f] *pgd=00000000 > |Internal error: Oops: 5 [#1] ARM > |Modules linked in: uvcvideo] > |CPU: 0 PID: 2613 Comm: gst-launch-0.10 Tainted: G W 3.14.25+ #40 > |task: ec2b8100 ti: ec38e000 task.ti: ec38e000 > |PC is at hcd_buffer_free+0x64/0xc0 > |LR is at uvc_free_urb_buffers+0x34/0x50 [uvcvideo] > |Process gst-launch-0.10 (pid: 2613, stack limit = 0xec38e240) > |[<c03a07e8>] (hcd_buffer_free) > |[<bf2f0c78>] (uvc_free_urb_buffers [uvcvideo]) > |[<bf2f33dc>] (uvc_video_enable [uvcvideo]) > |[<bf2ef150>] (uvc_v4l2_release [uvcvideo]) > |[<bf2ac318>] (v4l2_release [videodev]) > |[<c0103334>] (__fput) > |[<c005b618>] (task_work_run) > |[<c00417a0>] (do_exit) > |[<c0041ebc>] (do_group_exit) > > as part of the device-removal the HCD removes its dma-buffers, the HCD > structure itself and even the struct device is gone. That means if UVC > removes its URBs after its last user (/dev/videoX) is gone and not from > the ->disconnect() callback then it is too late because the HCD might > gone. > > First, I fixed by in the UVC driver by ignoring the UVC_SC_VIDEOSTREAMING > in its ->disconnect callback and calling uvc_video_enable(, 0) in > uvc_unregister_video(). But then I though that it might not be clever to > release that memory if there is userspace using it. > > So instead, I hold the device struct in the HCD and the HCD struct on > every USB-buf-alloc. That means after a disconnect we still have a > refcount on usb_hcd and device and it will be cleaned "later" once the > last USB-buffer is released. This is not a valid solution. Notice that your _hcd_buffer_free still dereferences hcd->driver; that will not point to anything useful if you rmmod the HCD. Also, you neglected to move the calls to hcd_buffer_destroy from usb_remove_hcd to hcd_release. On the whole, it would be easier if the UVC driver could release its coherent DMA buffers during the disconnect callback. If that's not feasible we'll have to find some other solution. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Greg Kroah-Hartman | 2014-12-05 13:19:32 [-0800]: >On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: >> Consider the following scenario: >> - plugin a webcam >> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0… >> - remove the USB-HCD during playback via "rmmod $HCD" >> >> and now wait for the crash > >Which you deserve, why did you ever remove a kernel module? That's racy its been found by the testing team and looks legitimate. >and _never_ recommended, which is why it never happens automatically and >only root can do it. I beg your pardon. So it is okay to remove the UVC-driver / plug the cable and expect that things continue to work but removing the HCD is a no no? I always assumed that kernel should BUG() no matter what the user does unless he really begs for it. If there is a race then it is a bug that deserves to be fixed, right? >> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c >> index 506b969ea7fd..01e080a61519 100644 >> --- a/drivers/usb/core/buffer.c >> +++ b/drivers/usb/core/buffer.c >> @@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd) >> * better sharing and to leverage mm/slab.c intelligence. >> */ >> >> -void *hcd_buffer_alloc( >> +static void *_hcd_buffer_alloc( > >Looks like this isn't really needed here, right? either this or I would have the tree callers if the allocation succeded or not in order not to take a reference if the allocation failed. >> struct usb_bus *bus, >> size_t size, >> gfp_t mem_flags, >> @@ -131,7 +131,19 @@ void *hcd_buffer_alloc( >> return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags); >> } >> >> -void hcd_buffer_free( >> +void *hcd_buffer_alloc(struct usb_bus *bus, size_t size, gfp_t mem_flags, >> + dma_addr_t *dma) >> +{ >> + struct usb_hcd *hcd = bus_to_hcd(bus); >> + void *ret; >> + >> + ret = _hcd_buffer_alloc(bus, size, mem_flags, dma); >> + if (ret) >> + usb_get_hcd(hcd); > >I'm all for some good reference counting, but this is going to cause a >_lot_ of churn on this reference count, what is the performance issue >with doing this for every buffer? The UVC allocates the buffers once and reuses them. If a driver does any kind of high-performance transfers and allocates new buffers on each transfer then I would expect this kref_get() is in the noise area. But if you want real numbers I would have to go ahead and test it. A single get() on first allocation and its counter part on cleanup would be enough if you are too concerned about it on every allocation (it would be transparent to the user). >thanks, > >greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alan Stern | 2014-12-05 16:21:02 [-0500]: >On Fri, 5 Dec 2014, Sebastian Andrzej Siewior wrote: >> So instead, I hold the device struct in the HCD and the HCD struct on >> every USB-buf-alloc. That means after a disconnect we still have a >> refcount on usb_hcd and device and it will be cleaned "later" once the >> last USB-buffer is released. > >This is not a valid solution. Notice that your _hcd_buffer_free still >dereferences hcd->driver; that will not point to anything useful if you >rmmod the HCD. Hmm. You're right, that one is gone. >Also, you neglected to move the calls to hcd_buffer_destroy from >usb_remove_hcd to hcd_release. I add them, I didn't move them. >On the whole, it would be easier if the UVC driver could release its >coherent DMA buffers during the disconnect callback. If that's not >feasible we'll have to find some other solution. I had one patch doing that. Let me grab it out on Monday. >Alan Stern > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 06, 2014 at 12:13:13AM +0100, Sebastian Andrzej Siewior wrote: > * Greg Kroah-Hartman | 2014-12-05 13:19:32 [-0800]: > > >On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: > >> Consider the following scenario: > >> - plugin a webcam > >> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0… > >> - remove the USB-HCD during playback via "rmmod $HCD" > >> > >> and now wait for the crash > > > >Which you deserve, why did you ever remove a kernel module? That's racy > its been found by the testing team and looks legitimate. > > >and _never_ recommended, which is why it never happens automatically and > >only root can do it. > I beg your pardon. So it is okay to remove the UVC-driver / plug the > cable and expect that things continue to work but removing the HCD is a > no no? If you hot unplug the HCD and this is an issue, yes, that's something to fix. If you can only trigger this by unloading a kernel module, no, it's not a big issue at all. It's pretty trivial to cause kernel oopses by unloading kernel modules if you know what you are doing. > >> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c > >> index 506b969ea7fd..01e080a61519 100644 > >> --- a/drivers/usb/core/buffer.c > >> +++ b/drivers/usb/core/buffer.c > >> @@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd) > >> * better sharing and to leverage mm/slab.c intelligence. > >> */ > >> > >> -void *hcd_buffer_alloc( > >> +static void *_hcd_buffer_alloc( > > > >Looks like this isn't really needed here, right? > > either this or I would have the tree callers if the allocation succeded > or not in order not to take a reference if the allocation failed. My point is this isn't needed for this patch. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 08, 2014 at 09:44:05AM +0000, David Laight wrote: > From: Greg Kroah-Hartman > > On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: > > > Consider the following scenario: > > > - plugin a webcam > > > - play the stream via gst-launch-0.10 v4l2src device=/dev/video0 > > > - remove the USB-HCD during playback via "rmmod $HCD" > > > > > > and now wait for the crash > > > > Which you deserve, why did you ever remove a kernel module? That's racy > > and _never_ recommended, which is why it never happens automatically and > > only root can do it. > > Really drivers and subsystems should have the required locking (etc) to > ensure that kernel modules can either be unloaded, or that the unload > request itself fails if the device is busy. > > It shouldn't be considered a 'shoot self in foot' operation. > OTOH there are likely to be bugs. This is not always the case, sorry, removing a kernel module is a known racy condition, and sometimes adding all of the locking required to try to make it "safe" just isn't worth it overall, as this is something that _only_ a developer does. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/09/2014 04:24 PM, 'Greg Kroah-Hartman' wrote: > On Mon, Dec 08, 2014 at 09:44:05AM +0000, David Laight wrote: >> From: Greg Kroah-Hartman >>> On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: >>>> Consider the following scenario: >>>> - plugin a webcam >>>> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0 >>>> - remove the USB-HCD during playback via "rmmod $HCD" >>>> >>>> and now wait for the crash >>> >>> Which you deserve, why did you ever remove a kernel module? That's racy >>> and _never_ recommended, which is why it never happens automatically and >>> only root can do it. >> >> Really drivers and subsystems should have the required locking (etc) to >> ensure that kernel modules can either be unloaded, or that the unload >> request itself fails if the device is busy. >> >> It shouldn't be considered a 'shoot self in foot' operation. >> OTOH there are likely to be bugs. > > This is not always the case, sorry, removing a kernel module is a known > racy condition, and sometimes adding all of the locking required to try > to make it "safe" just isn't worth it overall, as this is something that > _only_ a developer does. I wasn't are of that. rmmod does not mention this. Kconfig does not mention this and suggest y as default (for MODULE_UNLOAD) . rmmod -f likely causes problems but this is not the case here. If you want to avoid rmmod why not mark a driver that it is not safe to remove it? And why not make it work? You can unbind the HCD driver from the PCI-device via sysfs and this is not something not only a developer does. This "unbind" calls the remove function of the driver and the only difference between unbind and rmmod is that the module remains inserted (but this is no news for you). Now, this unbind happens if you choose to pass a PCI-device to a qemu guest. This is a fairly common use-case for a non-developer since it is quite easy to setup in virt-manager for instance. All you need is a hardware with IOMMU support. I used this to get the usb.org testsuite running in my Windows guest which needs access to EHCI registers). I could also mention hacking on XHCI and not crashing the physical machine if something goes south but then I would rise the developer card again. I even rmmod & modprobe my mmc controller on my notebook because for some reason it does not work otherwise after a suspend + resume cycle (and my motivation to look after this is quite low since I barely use my notebook at all). I am really surprised that you as a core developer and maintainer of the drivers infrastructure say that one should not remove a driver. > greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 09, 2014 at 05:01:35PM +0100, Sebastian Andrzej Siewior wrote: > On 12/09/2014 04:24 PM, 'Greg Kroah-Hartman' wrote: > > On Mon, Dec 08, 2014 at 09:44:05AM +0000, David Laight wrote: > >> From: Greg Kroah-Hartman > >>> On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote: > >>>> Consider the following scenario: > >>>> - plugin a webcam > >>>> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0 > >>>> - remove the USB-HCD during playback via "rmmod $HCD" > >>>> > >>>> and now wait for the crash > >>> > >>> Which you deserve, why did you ever remove a kernel module? That's racy > >>> and _never_ recommended, which is why it never happens automatically and > >>> only root can do it. > >> > >> Really drivers and subsystems should have the required locking (etc) to > >> ensure that kernel modules can either be unloaded, or that the unload > >> request itself fails if the device is busy. > >> > >> It shouldn't be considered a 'shoot self in foot' operation. > >> OTOH there are likely to be bugs. > > > > This is not always the case, sorry, removing a kernel module is a known > > racy condition, and sometimes adding all of the locking required to try > > to make it "safe" just isn't worth it overall, as this is something that > > _only_ a developer does. > > I wasn't are of that. rmmod does not mention this. Kconfig does not > mention this and suggest y as default (for MODULE_UNLOAD) . rmmod -f > likely causes problems but this is not the case here. If you want to > avoid rmmod why not mark a driver that it is not safe to remove it? And > why not make it work? Because sometimes fixes to make rmmod work "properly" entail slowing down the whole "normal" path. There is inherit problems with the core of the module unload code for all modules that are known, and are not going to be fixed because this isn't something that really matters. > You can unbind the HCD driver from the PCI-device via sysfs and this is > not something not only a developer does. This "unbind" calls the remove > function of the driver and the only difference between unbind and rmmod > is that the module remains inserted (but this is no news for you). If unbind causes a problem, it's the same problem that could happen if the hardware is hot-unplugged (like on a PCI card.) Stuff like that _does_ need to be fixed, and if your test shows this is a problem, I am all for fixing that. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 'Greg Kroah-Hartman' | 2014-12-09 11:54:15 [-0500]: >> You can unbind the HCD driver from the PCI-device via sysfs and this is >> not something not only a developer does. This "unbind" calls the remove >> function of the driver and the only difference between unbind and rmmod >> is that the module remains inserted (but this is no news for you). > >If unbind causes a problem, it's the same problem that could happen if >the hardware is hot-unplugged (like on a PCI card.) Stuff like that >_does_ need to be fixed, and if your test shows this is a problem, I am >all for fixing that. so I tried this with unbind and it didn't explode as assumed. On musb, for some reason the hcd struct never gets cleaned up. The driver free(s) URB memory after the hcd_pools are gone but since its size is 32KiB it uses dma_free_coherent() and this seems to work fine (sice the device is still there). So tried the same thing with EHCI. EHCI-hcd cleans up its HCD-struct as expected so I would have to poke at musb and figure out why it does not happen. Also, there is another difference: with EHCI I see first removal of buffers followed by removal of the pools. So it happens after disconnect but before the HCD pools are gone. I'm not sure why this happens with EHCI but not with MUSB. It seems that for some reason unbind triggers an error on /dev/video0 which makes gst-launch close that file. Once all users are gone, that hcd struct is cleaned up. Again, it works as I would expect it with EHCI but not with MUSB. So maybe, once I learned how MUSB dafeated the userspace notification about a gone device I might gain the same behavior that EHCI has. Also not freed hcd struct of musb looks also important :) >thanks, > >greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c index 506b969ea7fd..01e080a61519 100644 --- a/drivers/usb/core/buffer.c +++ b/drivers/usb/core/buffer.c @@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd) * better sharing and to leverage mm/slab.c intelligence. */ -void *hcd_buffer_alloc( +static void *_hcd_buffer_alloc( struct usb_bus *bus, size_t size, gfp_t mem_flags, @@ -131,7 +131,19 @@ void *hcd_buffer_alloc( return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags); } -void hcd_buffer_free( +void *hcd_buffer_alloc(struct usb_bus *bus, size_t size, gfp_t mem_flags, + dma_addr_t *dma) +{ + struct usb_hcd *hcd = bus_to_hcd(bus); + void *ret; + + ret = _hcd_buffer_alloc(bus, size, mem_flags, dma); + if (ret) + usb_get_hcd(hcd); + return ret; +} + +static void _hcd_buffer_free( struct usb_bus *bus, size_t size, void *addr, @@ -141,9 +153,6 @@ void hcd_buffer_free( struct usb_hcd *hcd = bus_to_hcd(bus); int i; - if (!addr) - return; - if (!bus->controller->dma_mask && !(hcd->driver->flags & HCD_LOCAL_MEM)) { kfree(addr); @@ -158,3 +167,14 @@ void hcd_buffer_free( } dma_free_coherent(hcd->self.controller, size, addr, dma); } + +void hcd_buffer_free(struct usb_bus *bus, size_t size, void *addr, + dma_addr_t dma) +{ + struct usb_hcd *hcd = bus_to_hcd(bus); + + if (!addr) + return; + _hcd_buffer_free(bus, size, addr, dma); + usb_put_hcd(hcd); +} diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index a6efb4184f2b..536da1639ac5 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -2469,6 +2469,7 @@ struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, kref_init(&hcd->kref); usb_bus_init(&hcd->self); + get_device(dev); hcd->self.controller = dev; hcd->self.bus_name = bus_name; hcd->self.uses_dma = (dev->dma_mask != NULL); @@ -2534,6 +2535,7 @@ static void hcd_release(struct kref *kref) peer->primary_hcd = NULL; } mutex_unlock(&usb_port_peer_mutex); + put_device(hcd->self.controller); kfree(hcd); }
Consider the following scenario: - plugin a webcam - play the stream via gst-launch-0.10 v4l2src device=/dev/video0… - remove the USB-HCD during playback via "rmmod $HCD" and now wait for the crash |musb-hdrc musb-hdrc.2.auto: remove, state 1 |usb usb2: USB disconnect, device number 1 |usb 2-1: USB disconnect, device number 3 |uvcvideo: Failed to resubmit video URB (-19). |musb-hdrc musb-hdrc.2.auto: USB bus 2 deregistered |musb-hdrc musb-hdrc.1.auto: remove, state 4 |usb usb1: USB disconnect, device number 1 |musb-hdrc musb-hdrc.1.auto: USB bus 1 deregistered |Unable to handle kernel paging request at virtual address 6b6b6b6f |pgd = c0004000 |[6b6b6b6f] *pgd=00000000 |Internal error: Oops: 5 [#1] ARM |Modules linked in: uvcvideo] |CPU: 0 PID: 2613 Comm: gst-launch-0.10 Tainted: G W 3.14.25+ #40 |task: ec2b8100 ti: ec38e000 task.ti: ec38e000 |PC is at hcd_buffer_free+0x64/0xc0 |LR is at uvc_free_urb_buffers+0x34/0x50 [uvcvideo] |Process gst-launch-0.10 (pid: 2613, stack limit = 0xec38e240) |[<c03a07e8>] (hcd_buffer_free) |[<bf2f0c78>] (uvc_free_urb_buffers [uvcvideo]) |[<bf2f33dc>] (uvc_video_enable [uvcvideo]) |[<bf2ef150>] (uvc_v4l2_release [uvcvideo]) |[<bf2ac318>] (v4l2_release [videodev]) |[<c0103334>] (__fput) |[<c005b618>] (task_work_run) |[<c00417a0>] (do_exit) |[<c0041ebc>] (do_group_exit) as part of the device-removal the HCD removes its dma-buffers, the HCD structure itself and even the struct device is gone. That means if UVC removes its URBs after its last user (/dev/videoX) is gone and not from the ->disconnect() callback then it is too late because the HCD might gone. First, I fixed by in the UVC driver by ignoring the UVC_SC_VIDEOSTREAMING in its ->disconnect callback and calling uvc_video_enable(, 0) in uvc_unregister_video(). But then I though that it might not be clever to release that memory if there is userspace using it. So instead, I hold the device struct in the HCD and the HCD struct on every USB-buf-alloc. That means after a disconnect we still have a refcount on usb_hcd and device and it will be cleaned "later" once the last USB-buffer is released. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- With this applied, I only see this three times (which is not new) | ------------[ cut here ]------------ | WARNING: CPU: 0 PID: 1755 at fs/sysfs/group.c:219 sysfs_remove_group+0x88/0x98() | sysfs group c08a70d4 not found for kobject 'event4' | Modules linked in: uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core v4l2_common videodev ipv6 musb_hdrc udc_core us] | CPU: 0 PID: 1755 Comm: gst-launch-0.10 Not tainted 3.18.0-rc7-00065-gabcefb342fbf-dirty #1813 | [<c00152a8>] (unwind_backtrace) from [<c0011a48>] (show_stack+0x10/0x14) | [<c0011a48>] (show_stack) from [<c056450c>] (dump_stack+0x80/0x9c) | [<c056450c>] (dump_stack) from [<c00401a0>] (warn_slowpath_common+0x68/0x8c) | [<c00401a0>] (warn_slowpath_common) from [<c0040258>] (warn_slowpath_fmt+0x30/0x40) | [<c0040258>] (warn_slowpath_fmt) from [<c01af2a4>] (sysfs_remove_group+0x88/0x98) | [<c01af2a4>] (sysfs_remove_group) from [<c039420c>] (device_del+0x34/0x198) | [<c039420c>] (device_del) from [<c0428208>] (evdev_disconnect+0x18/0x44) | [<c0428208>] (evdev_disconnect) from [<c04258c8>] (__input_unregister_device+0xa4/0x148) | [<c04258c8>] (__input_unregister_device) from [<c04259ac>] (input_unregister_device+0x40/0x74) | [<c04259ac>] (input_unregister_device) from [<bf0c6294>] (uvc_delete+0x20/0x10c [uvcvideo]) | [<bf0c6294>] (uvc_delete [uvcvideo]) from [<bf08a68c>] (v4l2_device_release+0x9c/0xc4 [videodev]) | [<bf08a68c>] (v4l2_device_release [videodev]) from [<c03934f0>] (device_release+0x2c/0x90) | [<c03934f0>] (device_release) from [<c030f7bc>] (kobject_release+0x48/0x7c) | [<c030f7bc>] (kobject_release) from [<bf089330>] (v4l2_release+0x50/0x78 [videodev]) | [<bf089330>] (v4l2_release [videodev]) from [<c0147e34>] (__fput+0x80/0x1c4) | [<c0147e34>] (__fput) from [<c0058a18>] (task_work_run+0xb4/0xe4) | [<c0058a18>] (task_work_run) from [<c00425ec>] (do_exit+0x2dc/0x92c) | [<c00425ec>] (do_exit) from [<c0042ca4>] (do_group_exit+0x3c/0xb0) | [<c0042ca4>] (do_group_exit) from [<c0042d28>] (__wake_up_parent+0x0/0x18) | ---[ end trace b54a8f3c8129180e ]--- anyone an idea? drivers/usb/core/buffer.c | 30 +++++++++++++++++++++++++----- drivers/usb/core/hcd.c | 2 ++ 2 files changed, 27 insertions(+), 5 deletions(-)