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

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

[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.

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[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.

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Alan Stern]

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Sebastian Andrzej Siewior]

* 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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Sebastian Andrzej Siewior]

* 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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Sebastian Andrzej Siewior]

* 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.

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 7c8322d4fc63..d656c7de25ef 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -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);
-- 
2.1.3

RE: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[David Laight]

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.

	David

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

['Greg Kroah-Hartman']

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Sebastian Andrzej Siewior]

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

['Greg Kroah-Hartman']

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

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

[Sebastian Andrzej Siewior]

* '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