usb: hcd: get/put device and hcd for hcd_buffers()

PATCH In review Sebastian Andrzej Siewior 4 participants 12 messages Dec 5, 2014 – Dec 10, 2014

Subsystems (3)

Media (V4L/DVB) USB USB video class

lore

Share & tools

lore

Sebastian Andrzej Siewior

author

2014-12-05 20:04 UTC #0 lore raw

PATCH 1/1 usb: hcd: get/put device and hcd for hcd_buffers()

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 <[email protected]>

---
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(-)

drivers/usb/core/buffer.c +25 -5

index 506b969ea7fd..01e080a61519 100644
@@ -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);
+}

drivers/usb/core/hcd.c +2 -0

index a6efb4184f2b..536da1639ac5 100644
@@ -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);
 }

Signature · 1 line

2.1.3

Greg Kroah-Hartman

maintainer

2014-12-05 21:19 UTC #1 lore raw

Quoted 6 lines · Sebastian Andrzej Siewior

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.

Quoted 84 lines

|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)

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
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(-)

drivers/usb/core/buffer.c +0 -0

index 506b969ea7fd..01e080a61519 100644

drivers/usb/core/buffer.c void hcd_buffer_destroy(struct usb_hcd *hcd) · line 110

Quoted 5 lines

drivers/usb/core/buffer.c +1 -1

@@ -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?

Quoted 7 lines

drivers/usb/core/buffer.c +0 -0

 	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);
 }

drivers/usb/core/buffer.c void *hcd_buffer_alloc( · line 142

Quoted 10 lines

drivers/usb/core/buffer.c +9 -1

-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

Sebastian Andrzej Siewior

author

2014-12-05 23:13 UTC #2 lore raw

Quoted 9 lines · * Greg Kroah-Hartman | 2014-12-05 13:19:32 [-0800]:

Quoted 6 lines · Sebastian Andrzej Siewior
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.

Quoted 2 lines

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?

Quoted 8 lines

drivers/usb/core/buffer.c +0 -0

index 506b969ea7fd..01e080a61519 100644
@@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
  * better sharing and to leverage mm/slab.c intelligence.
  */

Quoted 4 lines

drivers/usb/core/buffer.c void hcd_buffer_destroy(struct usb_hcd *hcd) · line 110
Quoted 2 lines
drivers/usb/core/buffer.c +1 -1
-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.

Quoted 7 lines

drivers/usb/core/buffer.c +0 -0

 	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);
 }

Quoted 14 lines

drivers/usb/core/buffer.c void *hcd_buffer_alloc( · line 142
Quoted 10 lines
drivers/usb/core/buffer.c +9 -1
-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).

Quoted 3 lines

thanks,
greg k-h

Sebastian

Greg Kroah-Hartman

maintainer

2014-12-06 00:37 UTC #3 lore raw

Quoted 10 lines · Sebastian Andrzej Siewior

Quoted 8 lines · * Greg Kroah-Hartman | 2014-12-05 13:19:32 [-0800]:
Quoted 6 lines · Sebastian Andrzej Siewior
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

Quoted 8 lines

Quoted 1 line
Which you deserve, why did you ever remove a kernel module? That's racy
its been found by the testing team and looks legitimate.
Quoted 2 lines
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.

Quoted 8 lines

drivers/usb/core/buffer.c +0 -0

index 506b969ea7fd..01e080a61519 100644
@@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
  * better sharing and to leverage mm/slab.c intelligence.
  */

Quoted 7 lines

Quoted 4 lines
drivers/usb/core/buffer.c void hcd_buffer_destroy(struct usb_hcd *hcd) · line 110
Quoted 2 lines
drivers/usb/core/buffer.c +1 -1
-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

David Laight

2014-12-08 09:51 UTC #4 lore raw

In reply to Greg Kroah-Hartman

From: Greg Kroah-Hartman

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 6 lines
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.

David

'Greg Kroah-Hartman'

maintainer

2014-12-09 15:24 UTC #5 lore raw

Quoted 13 lines · David Laight

From: Greg Kroah-Hartman
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 6 lines
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.

Quoted 6 lines

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

Sebastian Andrzej Siewior

author

2014-12-09 16:01 UTC #6 lore raw

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · David Laight
From: Greg Kroah-Hartman
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 6 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 1 line

greg k-h

Sebastian

'Greg Kroah-Hartman'

maintainer

2014-12-09 16:54 UTC #7 lore raw

Quoted 27 lines · Sebastian Andrzej Siewior

Quoted 18 lines · Greg Kroah-Hartman
Quoted 17 lines · David Laight
From: Greg Kroah-Hartman
Quoted 11 lines · Sebastian Andrzej Siewior
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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.

Quoted 5 lines

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.

Quoted 4 lines

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

Sebastian Andrzej Siewior

author

2014-12-10 18:25 UTC #8 lore raw

Quoted 9 lines · * 'Greg Kroah-Hartman' | 2014-12-09 11:54:15 [-0500]:

Quoted 4 lines
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 :)

Quoted 3 lines

thanks,
greg k-h

Sebastian

Alan Stern

2014-12-05 21:21 UTC #9 lore raw

In reply to Sebastian Andrzej Siewior

Quoted 46 lines · Sebastian Andrzej Siewior

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.

Quoted 4 lines

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

Sebastian Andrzej Siewior

author

2014-12-05 23:23 UTC #10 lore raw

Quoted 9 lines · * Alan Stern | 2014-12-05 16:21:02 [-0500]:

Quoted 4 lines · Sebastian Andrzej Siewior
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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

Alan Stern

Sebastian

Sebastian Andrzej Siewior

author

2014-12-08 08:43 UTC #11 lore raw

Quoted 1 line · * Sebastian Andrzej Siewior | 2014-12-06 00:23:27 [+0100]:

I had one patch doing that. Let me grab it out on Monday.

okay, this is it. Laurent, any idea why this could not fly? I haven't seen anything odd so far.

drivers/media/usb/uvc/uvc_driver.c +1 -4

index 7c8322d4fc63..d656c7de25ef 100644
@@ -1703,6 +1703,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		stream->vdev = NULL;
 
 		uvc_debugfs_cleanup_stream(stream);
+		uvc_video_enable(stream, 0);
 	}
 
 	/* Decrement the stream count and call uvc_delete explicitly if there

@@ -1950,10 +1951,6 @@ static void uvc_disconnect(struct usb_interface *intf)
 	 */
 	usb_set_intfdata(intf, NULL);
 
-	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
-	    UVC_SC_VIDEOSTREAMING)
-		return;
-
 	dev->state |= UVC_DEV_DISCONNECTED;
 
 	uvc_unregister_video(dev);

Signature · 1 line

2.1.3