Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect
From: Laurent Pinchart
Date: Mon Apr 17 2017 - 08:23:18 EST
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@xxxxxxxxxxxxxxxx>
> Cc: Dave Stevenson <linux-media@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Greg KH <greg@xxxxxxxxx>
> Signed-off-by: Daniel Axtens <dja@xxxxxxxxxx>
>
> ---
>
> 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