[PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Daniel Axtens]

Currently, disconnecting a USB webcam while it is in use prints out a
number of warnings, such as:

WARNING: CPU: 2 PID: 3118 at /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237 sysfs_remove_group+0x8b/0x90
sysfs group ffffffffa7cd0780 not found for kobject 'event13'

This has been noticed before. [0]

This is because of the order in which things are torn down.

If there are no streams active during a USB disconnect:

 - uvc_disconnect() is invoked via device_del() through the bus
   notifier mechanism.

 - this calls uvc_unregister_video().

 - uvc_unregister_video() unregisters the video device for each
   stream,

 - because there are no streams open, it calls uvc_delete()

 - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
   input device.

 - uvc_delete() calls media_device_unregister(), which cleans up the
   media device

 - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
   return, and we end up back in device_del().

 - device_del() then cleans up the sysfs folder for the camera with
   dpm_sysfs_remove(). Because uvc_status_cleanup() and
   media_device_unregister() have already been called, this all works
   nicely.

If, on the other hand, there *are* streams active during a USB disconnect:

 - uvc_disconnect() is invoked

 - this calls uvc_unregister_video()

 - uvc_unregister_video() unregisters the video device for each
   stream,

 - uvc_unregister_video() and uvc_disconnect() return, and we end up
   back in device_del().

 - device_del() then cleans up the sysfs folder for the camera with
   dpm_sysfs_remove(). Because the status input device and the media
   device are children of the USB device, this also deletes their
   sysfs folders.

 - Sometime later, the final stream is closed, invoking uvc_release().

 - uvc_release() calls uvc_delete()

 - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
   input device. Because the sysfs directory has already been removed,
   this causes a WARNing.

 - uvc_delete() calls media_device_unregister(), which cleans up the
   media device. Because the sysfs directory has already been removed,
   this causes another WARNing.

To fix this, we need to make sure the devices are always unregistered
before the end of uvc_disconnect(). To this, move the unregistration
into the disconnect path:

 - split uvc_status_cleanup() into two parts, one on disconnect that
   unregisters and one on delete that frees.

 - move media_device_unregister() into the disconnect path.

[0]: https://lkml.org/lkml/2016/12/8/657

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Dave Stevenson <linux-media@destevenson.freeserve.co.uk>
Cc: Greg KH <greg@kroah.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>

---

Tested with cheese and yavta.
---
 drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
 drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
 drivers/media/usb/uvc/uvcvideo.h   | 1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 46d6be0bb316..2390592f78e0 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev)
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(dev->mdev.devnode))
-		media_device_unregister(&dev->mdev);
 	media_device_cleanup(&dev->mdev);
 #endif
 
@@ -1884,6 +1882,12 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		uvc_debugfs_cleanup_stream(stream);
 	}
 
+	uvc_status_unregister(dev);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (media_devnode_is_registered(dev->mdev.devnode))
+		media_device_unregister(&dev->mdev);
+#endif
+
 	/* Decrement the stream count and call uvc_delete explicitly if there
 	 * are no stream left.
 	 */
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index f552ab997956..95709b23d3b4 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev)
 	return 0;
 }
 
-void uvc_status_cleanup(struct uvc_device *dev)
+void uvc_status_unregister(struct uvc_device *dev)
 {
 	usb_kill_urb(dev->int_urb);
+	uvc_input_cleanup(dev);
+}
+
+void uvc_status_cleanup(struct uvc_device *dev)
+{
 	usb_free_urb(dev->int_urb);
 	kfree(dev->status);
-	uvc_input_cleanup(dev);
 }
 
 int uvc_status_start(struct uvc_device *dev, gfp_t flags)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 15e415e32c7f..4b4814df35cd 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 
 /* Status */
 extern int uvc_status_init(struct uvc_device *dev);
+extern void uvc_status_unregister(struct uvc_device *dev);
 extern void uvc_status_cleanup(struct uvc_device *dev);
 extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
 extern void uvc_status_stop(struct uvc_device *dev);
-- 
2.9.3

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

[Daniel Axtens]

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

Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch (and the investigation).

On Monday 17 Apr 2017 18:52:39 Daniel Axtens wrote:
> Currently, disconnecting a USB webcam while it is in use prints out a
> number of warnings, such as:
> 
> WARNING: CPU: 2 PID: 3118 at
> /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237
> sysfs_remove_group+0x8b/0x90 sysfs group ffffffffa7cd0780 not found for
> kobject 'event13'
> 
> This has been noticed before. [0]
> 
> This is because of the order in which things are torn down.
> 
> If there are no streams active during a USB disconnect:
> 
>  - uvc_disconnect() is invoked via device_del() through the bus
>    notifier mechanism.
> 
>  - this calls uvc_unregister_video().
> 
>  - uvc_unregister_video() unregisters the video device for each
>    stream,
> 
>  - because there are no streams open, it calls uvc_delete()
> 
>  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
>    input device.
> 
>  - uvc_delete() calls media_device_unregister(), which cleans up the
>    media device
> 
>  - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
>    return, and we end up back in device_del().
> 
>  - device_del() then cleans up the sysfs folder for the camera with
>    dpm_sysfs_remove(). Because uvc_status_cleanup() and
>    media_device_unregister() have already been called, this all works
>    nicely.
> 
> If, on the other hand, there *are* streams active during a USB disconnect:
> 
>  - uvc_disconnect() is invoked
> 
>  - this calls uvc_unregister_video()
> 
>  - uvc_unregister_video() unregisters the video device for each
>    stream,
> 
>  - uvc_unregister_video() and uvc_disconnect() return, and we end up
>    back in device_del().
> 
>  - device_del() then cleans up the sysfs folder for the camera with
>    dpm_sysfs_remove(). Because the status input device and the media
>    device are children of the USB device, this also deletes their
>    sysfs folders.
> 
>  - Sometime later, the final stream is closed, invoking uvc_release().
> 
>  - uvc_release() calls uvc_delete()
> 
>  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
>    input device. Because the sysfs directory has already been removed,
>    this causes a WARNing.
> 
>  - uvc_delete() calls media_device_unregister(), which cleans up the
>    media device. Because the sysfs directory has already been removed,
>    this causes another WARNing.
> 
> To fix this, we need to make sure the devices are always unregistered
> before the end of uvc_disconnect(). To this, move the unregistration
> into the disconnect path:
>
>  - split uvc_status_cleanup() into two parts, one on disconnect that
>    unregisters and one on delete that frees.
> 
>  - move media_device_unregister() into the disconnect path.

While the patch looks reasonable to me (with one comment below though), isn't 
this an issue with the USB core, or possibly the device core ? It's a common 
practice to create device nodes as children of physical devices. Does the 
device core really require all device nodes to be unregistered synchronously 
with physical device hot-unplug ? If so, shouldn't it warn somehow when a 
device is deleted and still has children, instead of accepting that silently 
and later complaining due to sysfs issues ?

> [0]: https://lkml.org/lkml/2016/12/8/657
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Dave Stevenson <linux-media@destevenson.freeserve.co.uk>
> Cc: Greg KH <greg@kroah.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> ---
> 
> Tested with cheese and yavta.
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
>  drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
>  drivers/media/usb/uvc/uvcvideo.h   | 1 +
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 46d6be0bb316..2390592f78e0
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev)
>  	if (dev->vdev.dev)
>  		v4l2_device_unregister(&dev->vdev);
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	if (media_devnode_is_registered(dev->mdev.devnode))
> -		media_device_unregister(&dev->mdev);

media_device_unregister() will now be called before v4l2_device_unregister() 
which, unless I'm mistaken, will now result in 
media_device_unregister_entity() being called twice for every entity, the 
first time by media_device_unregister(), and the second time by 
v4l2_device_unregister_subdev() through v4l2_device_unregister(). Looking at 
media_device_unregister() I don't think that's safe.

We could move to v4l2_device_unregister() call to uvc_unregister_video(), but 
that worries me (perhaps unnecessarily though) due to the race conditions it 
could introduce. Would you still be able to give it a try ?

Note that your patch isn't really at fault here, the media controller and V4L2 
core code have been broken for a long time when it comes to entity lifetime 
management. That might be fixed some day, but I won't hold my breath given the 
bad track record of the previous year and a half.

>  	media_device_cleanup(&dev->mdev);
>  #endif
> 
> @@ -1884,6 +1882,12 @@ static void uvc_unregister_video(struct uvc_device
> *dev) uvc_debugfs_cleanup_stream(stream);
>  	}
> 
> +	uvc_status_unregister(dev);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	if (media_devnode_is_registered(dev->mdev.devnode))
> +		media_device_unregister(&dev->mdev);
> +#endif
> +
>  	/* Decrement the stream count and call uvc_delete explicitly if there
>  	 * are no stream left.
>  	 */
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c index f552ab997956..95709b23d3b4
> 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev)
>  	return 0;
>  }
> 
> -void uvc_status_cleanup(struct uvc_device *dev)
> +void uvc_status_unregister(struct uvc_device *dev)
>  {
>  	usb_kill_urb(dev->int_urb);
> +	uvc_input_cleanup(dev);
> +}
> +
> +void uvc_status_cleanup(struct uvc_device *dev)
> +{
>  	usb_free_urb(dev->int_urb);
>  	kfree(dev->status);
> -	uvc_input_cleanup(dev);
>  }
> 
>  int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 15e415e32c7f..4b4814df35cd 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream,
> 
>  /* Status */
>  extern int uvc_status_init(struct uvc_device *dev);
> +extern void uvc_status_unregister(struct uvc_device *dev);
>  extern void uvc_status_cleanup(struct uvc_device *dev);
>  extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
>  extern void uvc_status_stop(struct uvc_device *dev);

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

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

[Daniel Axtens]

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

Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Daniel Axtens]

Hi Laurent,

>> To fix this, we need to make sure the devices are always unregistered
>> before the end of uvc_disconnect(). To this, move the unregistration
>> into the disconnect path:
>>
>>  - split uvc_status_cleanup() into two parts, one on disconnect that
>>    unregisters and one on delete that frees.
>> 
>>  - move media_device_unregister() into the disconnect path.
>
> While the patch looks reasonable to me (with one comment below though), isn't 
> this an issue with the USB core, or possibly the device core ? It's a common 
> practice to create device nodes as children of physical devices. Does the 
> device core really require all device nodes to be unregistered synchronously 
> with physical device hot-unplug ? If so, shouldn't it warn somehow when a 
> device is deleted and still has children, instead of accepting that silently 
> and later complaining due to sysfs issues ?

Probably! I might have a look at this in a bit - I was initially drawn
into this area because of a misbehaving USB3 dock + webcam combo; once I
sort out my more pressing issues there I will have a look at putting
that extra sanity checking in the device core.

>
>> [0]: https://lkml.org/lkml/2016/12/8/657
>> 
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Dave Stevenson <linux-media@destevenson.freeserve.co.uk>
>> Cc: Greg KH <greg@kroah.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> 
>> ---
>> 
>> Tested with cheese and yavta.
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
>>  drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
>>  drivers/media/usb/uvc/uvcvideo.h   | 1 +
>>  3 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>> b/drivers/media/usb/uvc/uvc_driver.c index 46d6be0bb316..2390592f78e0
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev)
>>  	if (dev->vdev.dev)
>>  		v4l2_device_unregister(&dev->vdev);
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>> -	if (media_devnode_is_registered(dev->mdev.devnode))
>> -		media_device_unregister(&dev->mdev);
>
> media_device_unregister() will now be called before v4l2_device_unregister() 
> which, unless I'm mistaken, will now result in 
> media_device_unregister_entity() being called twice for every entity, the 
> first time by media_device_unregister(), and the second time by 
> v4l2_device_unregister_subdev() through v4l2_device_unregister(). Looking at 
> media_device_unregister() I don't think that's safe.
>
> We could move to v4l2_device_unregister() call to uvc_unregister_video(), but 
> that worries me (perhaps unnecessarily though) due to the race conditions it 
> could introduce. Would you still be able to give it a try ?
>

That's a good catch. I have moved v4l2_device_unregister() into the
unregister path, and turned on a bunch of debugging, including KASan. It
looks good so far, but I will plug and unplug the webcam a few more
times before sending v2 :)

> Note that your patch isn't really at fault here, the media controller and V4L2 
> core code have been broken for a long time when it comes to entity lifetime 
> management. That might be fixed some day, but I won't hold my breath given the 
> bad track record of the previous year and a half.

This is my first real foray into lifecycle managment in Linux. I've
found it quite confusing, so it's comforting to know that it's not just me!

Regards,
Daniel

>
>>  	media_device_cleanup(&dev->mdev);
>>  #endif
>> 
>> @@ -1884,6 +1882,12 @@ static void uvc_unregister_video(struct uvc_device
>> *dev) uvc_debugfs_cleanup_stream(stream);
>>  	}
>> 
>> +	uvc_status_unregister(dev);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +	if (media_devnode_is_registered(dev->mdev.devnode))
>> +		media_device_unregister(&dev->mdev);
>> +#endif
>> +
>>  	/* Decrement the stream count and call uvc_delete explicitly if there
>>  	 * are no stream left.
>>  	 */
>> diff --git a/drivers/media/usb/uvc/uvc_status.c
>> b/drivers/media/usb/uvc/uvc_status.c index f552ab997956..95709b23d3b4
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_status.c
>> +++ b/drivers/media/usb/uvc/uvc_status.c
>> @@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev)
>>  	return 0;
>>  }
>> 
>> -void uvc_status_cleanup(struct uvc_device *dev)
>> +void uvc_status_unregister(struct uvc_device *dev)
>>  {
>>  	usb_kill_urb(dev->int_urb);
>> +	uvc_input_cleanup(dev);
>> +}
>> +
>> +void uvc_status_cleanup(struct uvc_device *dev)
>> +{
>>  	usb_free_urb(dev->int_urb);
>>  	kfree(dev->status);
>> -	uvc_input_cleanup(dev);
>>  }
>> 
>>  int uvc_status_start(struct uvc_device *dev, gfp_t flags)
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
>> b/drivers/media/usb/uvc/uvcvideo.h index 15e415e32c7f..4b4814df35cd 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming
>> *stream,
>> 
>>  /* Status */
>>  extern int uvc_status_init(struct uvc_device *dev);
>> +extern void uvc_status_unregister(struct uvc_device *dev);
>>  extern void uvc_status_cleanup(struct uvc_device *dev);
>>  extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
>>  extern void uvc_status_stop(struct uvc_device *dev);
>
> -- 
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Greg KH]

On Mon, Apr 17, 2017 at 03:23:55PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch (and the investigation).
> 
> On Monday 17 Apr 2017 18:52:39 Daniel Axtens wrote:
> > Currently, disconnecting a USB webcam while it is in use prints out a
> > number of warnings, such as:
> > 
> > WARNING: CPU: 2 PID: 3118 at
> > /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237
> > sysfs_remove_group+0x8b/0x90 sysfs group ffffffffa7cd0780 not found for
> > kobject 'event13'
> > 
> > This has been noticed before. [0]
> > 
> > This is because of the order in which things are torn down.
> > 
> > If there are no streams active during a USB disconnect:
> > 
> >  - uvc_disconnect() is invoked via device_del() through the bus
> >    notifier mechanism.
> > 
> >  - this calls uvc_unregister_video().
> > 
> >  - uvc_unregister_video() unregisters the video device for each
> >    stream,
> > 
> >  - because there are no streams open, it calls uvc_delete()
> > 
> >  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> >    input device.
> > 
> >  - uvc_delete() calls media_device_unregister(), which cleans up the
> >    media device
> > 
> >  - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
> >    return, and we end up back in device_del().
> > 
> >  - device_del() then cleans up the sysfs folder for the camera with
> >    dpm_sysfs_remove(). Because uvc_status_cleanup() and
> >    media_device_unregister() have already been called, this all works
> >    nicely.
> > 
> > If, on the other hand, there *are* streams active during a USB disconnect:
> > 
> >  - uvc_disconnect() is invoked
> > 
> >  - this calls uvc_unregister_video()
> > 
> >  - uvc_unregister_video() unregisters the video device for each
> >    stream,
> > 
> >  - uvc_unregister_video() and uvc_disconnect() return, and we end up
> >    back in device_del().
> > 
> >  - device_del() then cleans up the sysfs folder for the camera with
> >    dpm_sysfs_remove(). Because the status input device and the media
> >    device are children of the USB device, this also deletes their
> >    sysfs folders.
> > 
> >  - Sometime later, the final stream is closed, invoking uvc_release().
> > 
> >  - uvc_release() calls uvc_delete()
> > 
> >  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> >    input device. Because the sysfs directory has already been removed,
> >    this causes a WARNing.
> > 
> >  - uvc_delete() calls media_device_unregister(), which cleans up the
> >    media device. Because the sysfs directory has already been removed,
> >    this causes another WARNing.
> > 
> > To fix this, we need to make sure the devices are always unregistered
> > before the end of uvc_disconnect(). To this, move the unregistration
> > into the disconnect path:
> >
> >  - split uvc_status_cleanup() into two parts, one on disconnect that
> >    unregisters and one on delete that frees.
> > 
> >  - move media_device_unregister() into the disconnect path.
> 
> While the patch looks reasonable to me (with one comment below though), isn't 
> this an issue with the USB core, or possibly the device core ? It's a common 
> practice to create device nodes as children of physical devices. Does the 
> device core really require all device nodes to be unregistered synchronously 
> with physical device hot-unplug ? If so, shouldn't it warn somehow when a 
> device is deleted and still has children, instead of accepting that silently 
> and later complaining due to sysfs issues ?

When a physical device, or any other device, is removed, the children
should all also be removed.  You can't leave them around to be cleaned
up later, this has always been the case.

Yes, userspace can still hold references, but that should be fine as the
device will still be unregistered, just not freed yet, right?

> > [0]: https://lkml.org/lkml/2016/12/8/657
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Dave Stevenson <linux-media@destevenson.freeserve.co.uk>
> > Cc: Greg KH <greg@kroah.com>
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > 
> > ---
> > 
> > Tested with cheese and yavta.
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 8 ++++++--
> >  drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
> >  drivers/media/usb/uvc/uvcvideo.h   | 1 +
> >  3 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index 46d6be0bb316..2390592f78e0
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1815,8 +1815,6 @@ static void uvc_delete(struct uvc_device *dev)
> >  	if (dev->vdev.dev)
> >  		v4l2_device_unregister(&dev->vdev);
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> > -	if (media_devnode_is_registered(dev->mdev.devnode))
> > -		media_device_unregister(&dev->mdev);
> 
> media_device_unregister() will now be called before v4l2_device_unregister() 
> which, unless I'm mistaken, will now result in 
> media_device_unregister_entity() being called twice for every entity, the 
> first time by media_device_unregister(), and the second time by 
> v4l2_device_unregister_subdev() through v4l2_device_unregister(). Looking at 
> media_device_unregister() I don't think that's safe.
> 
> We could move to v4l2_device_unregister() call to uvc_unregister_video(), but 
> that worries me (perhaps unnecessarily though) due to the race conditions it 
> could introduce. Would you still be able to give it a try ?
> 
> Note that your patch isn't really at fault here, the media controller and V4L2 
> core code have been broken for a long time when it comes to entity lifetime 
> management. That might be fixed some day, but I won't hold my breath given the 
> bad track record of the previous year and a half.

Ugh, what a mess, I'll trust you that this is correct :)

thanks,

greg k-h