Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1624609 > unrolled thread

[PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect Daniel Axtens <dja@axtens.net> - 2017-04-17 11:00 +0200
    Re: [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-04-17 14:50 +0200
      Re: [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect Daniel Axtens <dja@axtens.net> - 2017-04-23 02:50 +0200

#1624609 — [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect

When an in-use webcam is disconnected, I noticed the following
messages:

  uvcvideo: Failed to resubmit video URB (-19).

-19 is -ENODEV, which does make sense given that the device has
disappeared.

We could put a case for -ENODEV like we have with -ENOENT, -ECONNRESET
and -ESHUTDOWN, but the usb_unlink_urb() API documentation says that
'The disconnect function should synchronize with a driver's I/O
routines to insure that all URB-related activity has completed before
it returns.' So we should make an effort to proactively kill URBs in
the disconnect path instead.

Call uvc_enable_video() (specifying 0 to disable) in the disconnect
path, which kills and frees URBs.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>

---

Before this patch, yavta -c hangs when a camera is disconnected, but
with this patch it exits immediately after the camera is
disconnected. I'm not sure if this is acceptable - Laurent?

---
 drivers/media/usb/uvc/uvc_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 2390592f78e0..647e3d8a1256 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1877,6 +1877,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		uvc_video_enable(stream, 0);
+
 		video_unregister_device(&stream->vdev);
 
 		uvc_debugfs_cleanup_stream(stream);
-- 
2.9.3

[toc] | [next] | [standalone]

#1624664

Hi Daniel,

Thank you for the patch.

On Monday 17 Apr 2017 18:52:40 Daniel Axtens wrote:
> When an in-use webcam is disconnected, I noticed the following
> messages:
> 
>   uvcvideo: Failed to resubmit video URB (-19).
> 
> -19 is -ENODEV, which does make sense given that the device has
> disappeared.
> 
> We could put a case for -ENODEV like we have with -ENOENT, -ECONNRESET
> and -ESHUTDOWN, but the usb_unlink_urb() API documentation says that
> 'The disconnect function should synchronize with a driver's I/O
> routines to insure that all URB-related activity has completed before
> it returns.' So we should make an effort to proactively kill URBs in
> the disconnect path instead.
> 
> Call uvc_enable_video() (specifying 0 to disable) in the disconnect
> path, which kills and frees URBs.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> ---
> 
> Before this patch, yavta -c hangs when a camera is disconnected, but
> with this patch it exits immediately after the camera is
> disconnected. I'm not sure if this is acceptable - Laurent?

I assume that the error message is caused by a race between disconnection and 
URB handling. When the device is disconnected I believe the USB core will 
cancel all in-progress URBs (I'm not very familiar with that part of the USB 
core anymore, so please don't consider this or any further related statement 
as true without double-checking), resulting in the URB completion handler 
being called with an error status. The completion handler should then avoid 
resubmitting the URB. However, if the completion handler is in progress when 
the device is disconnected, it won't notice that the device got disconnected, 
and will try to resubmit the URB.

I'm not sure to see how this patch will fix the problem. If the URB completion 
handler is in progress when the device is being disconnected, won't it call 
usb_submit_urb() regardless of whether you call usb_kill_urb() in the 
disconnect handler, resulting in an error message being printed ?

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 2390592f78e0..647e3d8a1256
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1877,6 +1877,8 @@ static void uvc_unregister_video(struct uvc_device
> *dev) if (!video_is_registered(&stream->vdev))
>  			continue;
> 
> +		uvc_video_enable(stream, 0);
> +
>  		video_unregister_device(&stream->vdev);
> 
>  		uvc_debugfs_cleanup_stream(stream);

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1628895

Hi Laurent,

Apologies for takig so long to get back to you.

> I assume that the error message is caused by a race between disconnection and 
> URB handling. When the device is disconnected I believe the USB core will 
> cancel all in-progress URBs (I'm not very familiar with that part of the USB 
> core anymore, so please don't consider this or any further related statement 
> as true without double-checking), resulting in the URB completion handler 
> being called with an error status. The completion handler should then avoid 
> resubmitting the URB. However, if the completion handler is in progress when 
> the device is disconnected, it won't notice that the device got disconnected, 
> and will try to resubmit the URB.
>
> I'm not sure to see how this patch will fix the problem. If the URB completion 
> handler is in progress when the device is being disconnected, won't it call 
> usb_submit_urb() regardless of whether you call usb_kill_urb() in the 
> disconnect handler, resulting in an error message being printed ?

You're completely right - I misunderstood the flow of the
function. Fixing this would require a different approach and would be
more invasive. Given that this does not cause anything more annoying
than some log messages, I will leave it for now.

Regards,
Daniel

>
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>> b/drivers/media/usb/uvc/uvc_driver.c index 2390592f78e0..647e3d8a1256
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1877,6 +1877,8 @@ static void uvc_unregister_video(struct uvc_device
>> *dev) if (!video_is_registered(&stream->vdev))
>>  			continue;
>> 
>> +		uvc_video_enable(stream, 0);
>> +
>>  		video_unregister_device(&stream->vdev);
>> 
>>  		uvc_debugfs_cleanup_stream(stream);
>
> -- 
> Regards,
>
> Laurent Pinchart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web