[PATCH v6 0/4] uvcvideo: Attempt N to land UVC race conditions fixes

[PATCH v6 0/4] uvcvideo: Attempt N to land UVC race conditions fixes

[Ricardo Ribalda]

Back in 2020 Guenter published a set of patches to fix some race
conditions in UVC:
https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/

That kind of race conditions are not only seen in UVC, but are a common
seen in almost all the kernel, so this is what it was decided back then
that we should try to fix them at higher levels.

After that. A lot of video_is_registered() were added to the core:

```
ribalda@alco:~/work/linux$ git grep is_registered drivers/media/v4l2-core/
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:  if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (vdev == NULL || !video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!vdev || !video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-ioctl.c:   if (!video_is_registered(vfd)) {
drivers/media/v4l2-core/v4l2-subdev.c:  if (video_is_registered(vdev)) {
```

And recently Sakari is trying to land:
https://lore.kernel.org/linux-media/20230201214535.347075-1-sakari.ailus@linux.intel.com/

Which will make obsolete a lot off (all?) of the video_is_registered() checks in
Guenter's patches.

Besides those checks, there were some other valid races fixed in his
patches.

This patchset tries to fix the races still present in our code.

Thanks!

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v6: Thanks Hans
- s/uvc_queue_streamoff/uvc_queue_release/
- Link to v5: https://lore.kernel.org/r/20240611-guenter-mini-v5-0-047b6fe5d08b@chromium.org

Changes in v5: Thanks Hans!
- Refactor unregister as vb2_video_unregister_device
- I have tested the first patch independently from the others, so it
  could be merged in two steps if needed.
- Link to v4: https://lore.kernel.org/r/20240327-guenter-mini-v4-0-49955c198eae@chromium.org

Changes in v4: Thanks Sergey and Guenter
- Fix typos
- Move location of mutex_init
- Split patch to make the suspend change explicit
- Link to v3: https://lore.kernel.org/r/20240325-guenter-mini-v3-0-c4bc61d84e03@chromium.org

Changes in v3: Thanks Hans!
- Stop streaming during uvc_unregister()
- Refactor the uvc_status code
- Link to v2: https://lore.kernel.org/r/20230309-guenter-mini-v2-0-e6410d590d43@chromium.org

Changes in v2:
- Actually send the series to the ML an not only to individuals.
- Link to v1: https://lore.kernel.org/r/20230309-guenter-mini-v1-0-627d10cf6e96@chromium.org

---
Ricardo Ribalda (4):
      media: uvcvideo: Stop stream during unregister
      media: uvcvideo: Refactor the status irq API
      media: uvcvideo: Avoid race condition during unregister
      media: uvcvideo: Exit early if there is not int_urb

 drivers/media/usb/uvc/uvc_driver.c | 45 +++++++++++++++++++--------
 drivers/media/usb/uvc/uvc_status.c | 62 +++++++++++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   | 22 ++++----------
 drivers/media/usb/uvc/uvcvideo.h   | 10 +++---
 4 files changed, 103 insertions(+), 36 deletions(-)
---
base-commit: b14257abe7057def6127f6fb2f14f9adc8acabdb
change-id: 20230309-guenter-mini-89861b084ef1

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Ricardo Ribalda]

uvc_unregister_video() can be called asynchronously from
uvc_disconnect(). If the device is still streaming when that happens, a
plethora of race conditions can occur.

Make sure that the device has stopped streaming before exiting this
function.

If the user still holds handles to the driver's file descriptors, any
ioctl will return -ENODEV from the v4l2 core.

This change makes uvc more consistent with the rest of the v4l2 drivers
using the vb2_fop_* and vb2_ioctl_* helpers.

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bbd90123a4e7..55132688e363 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
 	struct uvc_streaming *stream;
 
 	list_for_each_entry(stream, &dev->streams, list) {
+		/* Nothing to do here, continue. */
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		/*
+		 * For stream->vdev we follow the same logic as:
+		 * vb2_video_unregister_device().
+		 */
+
+		/* 1. Take a reference to vdev */
+		get_device(&stream->vdev.dev);
+
+		/* 2. Ensure that no new ioctls can be called. */
 		video_unregister_device(&stream->vdev);
-		video_unregister_device(&stream->meta.vdev);
+
+		/* 3. Wait for old ioctls to finish. */
+		mutex_lock(&stream->mutex);
+
+		/* 4. Stop streaming. */
+		uvc_queue_release(&stream->queue);
+
+		mutex_unlock(&stream->mutex);
+
+		put_device(&stream->vdev.dev);
+
+		/*
+		 * For stream->meta.vdev we can directly call:
+		 * vb2_video_unregister_device().
+		 */
+		vb2_video_unregister_device(&stream->meta.vdev);
+
+		/*
+		 * Now both vdevs are not streaming and all the ioctls will
+		 * return -ENODEV.
+		 */
 
 		uvc_debugfs_cleanup_stream(stream);
 	}

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v6 2/4] media: uvcvideo: Refactor the status irq API

[Ricardo Ribalda]

There are two different use-cases of uvc_status():
	- adding/removing a user when the camera is open/closed
	- stopping/starting when the camera is suspended/resumed

Make the API reflect these two use-cases and move all the refcounting
and locking logic to the uvc_status.c file.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++-------
 drivers/media/usb/uvc/uvc_status.c | 55 ++++++++++++++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c   | 22 +++++----------
 drivers/media/usb/uvc/uvcvideo.h   | 10 ++++---
 4 files changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 55132688e363..c8c0352af1e5 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2135,7 +2135,6 @@ static int uvc_probe(struct usb_interface *intf,
 	INIT_LIST_HEAD(&dev->streams);
 	kref_init(&dev->ref);
 	atomic_set(&dev->nmappings, 0);
-	mutex_init(&dev->lock);
 
 	dev->udev = usb_get_dev(udev);
 	dev->intf = usb_get_intf(intf);
@@ -2301,10 +2300,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
 	/* Controls are cached on the fly so they don't need to be saved. */
 	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
 	    UVC_SC_VIDEOCONTROL) {
-		mutex_lock(&dev->lock);
-		if (dev->users)
-			uvc_status_stop(dev);
-		mutex_unlock(&dev->lock);
+		uvc_status_suspend(dev);
 		return 0;
 	}
 
@@ -2335,12 +2331,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 				return ret;
 		}
 
-		mutex_lock(&dev->lock);
-		if (dev->users)
-			ret = uvc_status_start(dev, GFP_NOIO);
-		mutex_unlock(&dev->lock);
-
-		return ret;
+		return uvc_status_resume(dev);
 	}
 
 	list_for_each_entry(stream, &dev->streams, list) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e2..375a95dd3011 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -257,6 +257,8 @@ int uvc_status_init(struct uvc_device *dev)
 	unsigned int pipe;
 	int interval;
 
+	mutex_init(&dev->status_lock);
+
 	if (ep == NULL)
 		return 0;
 
@@ -302,18 +304,22 @@ void uvc_status_cleanup(struct uvc_device *dev)
 	kfree(dev->status);
 }
 
-int uvc_status_start(struct uvc_device *dev, gfp_t flags)
+static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
 {
+	lockdep_assert_held(&dev->status_lock);
+
 	if (dev->int_urb == NULL)
 		return 0;
 
 	return usb_submit_urb(dev->int_urb, flags);
 }
 
-void uvc_status_stop(struct uvc_device *dev)
+static void __uvc_status_stop(struct uvc_device *dev)
 {
 	struct uvc_ctrl_work *w = &dev->async_ctrl;
 
+	lockdep_assert_held(&dev->status_lock);
+
 	/*
 	 * Prevent the asynchronous control handler from requeing the URB. The
 	 * barrier is needed so the flush_status change is visible to other
@@ -350,3 +356,48 @@ void uvc_status_stop(struct uvc_device *dev)
 	 */
 	smp_store_release(&dev->flush_status, false);
 }
+
+int uvc_status_resume(struct uvc_device *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users)
+		ret = __uvc_status_start(dev, GFP_NOIO);
+	mutex_unlock(&dev->status_lock);
+
+	return ret;
+}
+
+void uvc_status_suspend(struct uvc_device *dev)
+{
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users)
+		__uvc_status_stop(dev);
+	mutex_unlock(&dev->status_lock);
+}
+
+int uvc_status_get(struct uvc_device *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->status_lock);
+	if (!dev->status_users)
+		ret = __uvc_status_start(dev, GFP_KERNEL);
+	if (!ret)
+		dev->status_users++;
+	mutex_unlock(&dev->status_lock);
+
+	return ret;
+}
+
+void uvc_status_put(struct uvc_device *dev)
+{
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users == 1)
+		__uvc_status_stop(dev);
+	WARN_ON(!dev->status_users);
+	if (dev->status_users)
+		dev->status_users--;
+	mutex_unlock(&dev->status_lock);
+}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4988f03640a..97c5407f6603 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -628,20 +628,13 @@ static int uvc_v4l2_open(struct file *file)
 		return -ENOMEM;
 	}
 
-	mutex_lock(&stream->dev->lock);
-	if (stream->dev->users == 0) {
-		ret = uvc_status_start(stream->dev, GFP_KERNEL);
-		if (ret < 0) {
-			mutex_unlock(&stream->dev->lock);
-			usb_autopm_put_interface(stream->dev->intf);
-			kfree(handle);
-			return ret;
-		}
+	ret = uvc_status_get(stream->dev);
+	if (ret) {
+		usb_autopm_put_interface(stream->dev->intf);
+		kfree(handle);
+		return ret;
 	}
 
-	stream->dev->users++;
-	mutex_unlock(&stream->dev->lock);
-
 	v4l2_fh_init(&handle->vfh, &stream->vdev);
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
@@ -670,10 +663,7 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	mutex_lock(&stream->dev->lock);
-	if (--stream->dev->users == 0)
-		uvc_status_stop(stream->dev);
-	mutex_unlock(&stream->dev->lock);
+	uvc_status_put(stream->dev);
 
 	usb_autopm_put_interface(stream->dev->intf);
 	return 0;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6fb0a78b1b00..00b600eb058c 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -555,8 +555,6 @@ struct uvc_device {
 
 	const struct uvc_device_info *info;
 
-	struct mutex lock;		/* Protects users */
-	unsigned int users;
 	atomic_t nmappings;
 
 	/* Video control interface */
@@ -578,6 +576,8 @@ struct uvc_device {
 	struct usb_host_endpoint *int_ep;
 	struct urb *int_urb;
 	struct uvc_status *status;
+	struct mutex status_lock; /* Protects status_users */
+	unsigned int status_users;
 	bool flush_status;
 
 	struct input_dev *input;
@@ -744,8 +744,10 @@ int uvc_register_video_device(struct uvc_device *dev,
 int uvc_status_init(struct uvc_device *dev);
 void uvc_status_unregister(struct uvc_device *dev);
 void uvc_status_cleanup(struct uvc_device *dev);
-int uvc_status_start(struct uvc_device *dev, gfp_t flags);
-void uvc_status_stop(struct uvc_device *dev);
+int uvc_status_resume(struct uvc_device *dev);
+void uvc_status_suspend(struct uvc_device *dev);
+int uvc_status_get(struct uvc_device *dev);
+void uvc_status_put(struct uvc_device *dev);
 
 /* Controls */
 extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v6 3/4] media: uvcvideo: Avoid race condition during unregister

[Ricardo Ribalda]

The control events are handled asynchronously by the driver. Once the
control event are handled, the urb is re-submitted.

If we simply kill the urb, there is a chance that a control event is
waiting to be processed, which will re-submit the urb after the device is
disconnected.

uvc_status_suspend() flushes the async controls and stops the urb in a
correct manner.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 375a95dd3011..8fd8250110e2 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -294,7 +294,7 @@ int uvc_status_init(struct uvc_device *dev)
 
 void uvc_status_unregister(struct uvc_device *dev)
 {
-	usb_kill_urb(dev->int_urb);
+	uvc_status_suspend(dev);
 	uvc_input_unregister(dev);
 }
 

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v6 4/4] media: uvcvideo: Exit early if there is not int_urb

[Ricardo Ribalda]

If there is no int_urb there is no need to do a clean stop.

Also we avoid calling usb_kill_urb(NULL). It is properly handled by the
usb framework, but it is not polite.

Now that we are at it, fix the code style in uvc_status_start() for
consistency.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_status.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 8fd8250110e2..9108522beea6 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -308,7 +308,7 @@ static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
 {
 	lockdep_assert_held(&dev->status_lock);
 
-	if (dev->int_urb == NULL)
+	if (!dev->int_urb)
 		return 0;
 
 	return usb_submit_urb(dev->int_urb, flags);
@@ -320,6 +320,9 @@ static void __uvc_status_stop(struct uvc_device *dev)
 
 	lockdep_assert_held(&dev->status_lock);
 
+	if (!dev->int_urb)
+		return;
+
 	/*
 	 * Prevent the asynchronous control handler from requeing the URB. The
 	 * barrier is needed so the flush_status change is visible to other

-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Jun 14, 2024 at 12:41:27PM +0000, Ricardo Ribalda wrote:
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can occur.
> 
> Make sure that the device has stopped streaming before exiting this
> function.
> 
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
> 
> This change makes uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.

As I've said many times before, this issue needs a fix in the V4L2 core,
ideally with support in the cdev core. It seems I'll have to do it
myself ?

> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e7..55132688e363 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	struct uvc_streaming *stream;
>  
>  	list_for_each_entry(stream, &dev->streams, list) {
> +		/* Nothing to do here, continue. */
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * For stream->vdev we follow the same logic as:
> +		 * vb2_video_unregister_device().
> +		 */
> +
> +		/* 1. Take a reference to vdev */
> +		get_device(&stream->vdev.dev);
> +
> +		/* 2. Ensure that no new ioctls can be called. */
>  		video_unregister_device(&stream->vdev);
> -		video_unregister_device(&stream->meta.vdev);
> +
> +		/* 3. Wait for old ioctls to finish. */
> +		mutex_lock(&stream->mutex);
> +
> +		/* 4. Stop streaming. */
> +		uvc_queue_release(&stream->queue);
> +
> +		mutex_unlock(&stream->mutex);
> +
> +		put_device(&stream->vdev.dev);
> +
> +		/*
> +		 * For stream->meta.vdev we can directly call:
> +		 * vb2_video_unregister_device().
> +		 */
> +		vb2_video_unregister_device(&stream->meta.vdev);
> +
> +		/*
> +		 * Now both vdevs are not streaming and all the ioctls will
> +		 * return -ENODEV.
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Ricardo Ribalda]

Hi Laurent

On Mon, 17 Jun 2024 at 01:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Jun 14, 2024 at 12:41:27PM +0000, Ricardo Ribalda wrote:
> > uvc_unregister_video() can be called asynchronously from
> > uvc_disconnect(). If the device is still streaming when that happens, a
> > plethora of race conditions can occur.
> >
> > Make sure that the device has stopped streaming before exiting this
> > function.
> >
> > If the user still holds handles to the driver's file descriptors, any
> > ioctl will return -ENODEV from the v4l2 core.
> >
> > This change makes uvc more consistent with the rest of the v4l2 drivers
> > using the vb2_fop_* and vb2_ioctl_* helpers.
>
> As I've said many times before, this issue needs a fix in the V4L2 core,
> ideally with support in the cdev core. It seems I'll have to do it
> myself ?

 vb2_video_unregister_device() already patched this issue. We are just
porting that solution to UVC, because uvc is not using the vb2
helpers.
I don't see why being more consistent with the rest of the v4l2 driver
makes it a bad thing.

I am reverting the ChromeOS solution for this race condition and
applying this patch instead:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5632459/1
That is going to test this patch in some million devices in some days.

This change is self-contained, fixes a real problem, makes the driver
consistent and will be tested by lots of users. We could land this
patch and help our users until there is a solution in cdev.
I would argue that even with a solution in cdev this is not a bad patch.

> ideally with support in the cdev core. It seems I'll have to do it
> myself ?

I can help reviewing and testing ;)

Regards!


>
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index bbd90123a4e7..55132688e363 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >       struct uvc_streaming *stream;
> >
> >       list_for_each_entry(stream, &dev->streams, list) {
> > +             /* Nothing to do here, continue. */
> >               if (!video_is_registered(&stream->vdev))
> >                       continue;
> >
> > +             /*
> > +              * For stream->vdev we follow the same logic as:
> > +              * vb2_video_unregister_device().
> > +              */
> > +
> > +             /* 1. Take a reference to vdev */
> > +             get_device(&stream->vdev.dev);
> > +
> > +             /* 2. Ensure that no new ioctls can be called. */
> >               video_unregister_device(&stream->vdev);
> > -             video_unregister_device(&stream->meta.vdev);
> > +
> > +             /* 3. Wait for old ioctls to finish. */
> > +             mutex_lock(&stream->mutex);
> > +
> > +             /* 4. Stop streaming. */
> > +             uvc_queue_release(&stream->queue);
> > +
> > +             mutex_unlock(&stream->mutex);
> > +
> > +             put_device(&stream->vdev.dev);
> > +
> > +             /*
> > +              * For stream->meta.vdev we can directly call:
> > +              * vb2_video_unregister_device().
> > +              */
> > +             vb2_video_unregister_device(&stream->meta.vdev);
> > +
> > +             /*
> > +              * Now both vdevs are not streaming and all the ioctls will
> > +              * return -ENODEV.
> > +              */
> >
> >               uvc_debugfs_cleanup_stream(stream);
> >       }
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

Hi Laurent,

We discussed this patch last week, and you thought that there was still
a race condition if the uvc device was unplugged while an application was
in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
op is called, which unlocks the serialization mutex).

I'll go through the code below, explaining why that isn't an issue.

On 14/06/2024 14:41, Ricardo Ribalda wrote:
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can occur.
> 
> Make sure that the device has stopped streaming before exiting this
> function.
> 
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
> 
> This change makes uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e7..55132688e363 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	struct uvc_streaming *stream;
>  
>  	list_for_each_entry(stream, &dev->streams, list) {
> +		/* Nothing to do here, continue. */
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * For stream->vdev we follow the same logic as:
> +		 * vb2_video_unregister_device().
> +		 */
> +
> +		/* 1. Take a reference to vdev */
> +		get_device(&stream->vdev.dev);

This ensures that the device refcount won't go to 0 if video_unregister_device
is called (which calls put_device).

But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
then that open filehandle also called get_device(). So while that application is
waiting, the device refcount will never go to 0.

> +
> +		/* 2. Ensure that no new ioctls can be called. */
>  		video_unregister_device(&stream->vdev);
> -		video_unregister_device(&stream->meta.vdev);
> +
> +		/* 3. Wait for old ioctls to finish. */
> +		mutex_lock(&stream->mutex);

If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
lock here. So in that case this won't wait for that specific ioctl to finish.

> +
> +		/* 4. Stop streaming. */
> +		uvc_queue_release(&stream->queue);

This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
waiting for the mutex to become available.

> +
> +		mutex_unlock(&stream->mutex);

At this point it can take the mutex again. But since q->streaming is now false,
(due to the __vb2_queue_cancel call) this will return an error which is returned
to userspace.

> +
> +		put_device(&stream->vdev.dev);

This releases the reference we took earlier. If the application has already closed
the filehandle, then this will release all memory. If the application still has the
fh open, then only when it closes that fh will the memory be released.

Conclusion: there is no race condition here, this is handled correctly by the core.

> +
> +		/*
> +		 * For stream->meta.vdev we can directly call:
> +		 * vb2_video_unregister_device().
> +		 */
> +		vb2_video_unregister_device(&stream->meta.vdev);

Perhaps a patch adding more comments to the vb2_video_unregister_device()
function might help document this sequence better.

Regards,

	Hans

> +
> +		/*
> +		 * Now both vdevs are not streaming and all the ioctls will
> +		 * return -ENODEV.
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
> 

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

On 25/09/2024 10:32, Hans Verkuil wrote:
> Hi Laurent,
> 
> We discussed this patch last week, and you thought that there was still
> a race condition if the uvc device was unplugged while an application was
> in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
> op is called, which unlocks the serialization mutex).
> 
> I'll go through the code below, explaining why that isn't an issue.

Update: I added an extra check for this scenario to the test-media script to make
sure we catch any potential regressions in how this is handled in the core.

Regards,

	Hans

> 
> On 14/06/2024 14:41, Ricardo Ribalda wrote:
>> uvc_unregister_video() can be called asynchronously from
>> uvc_disconnect(). If the device is still streaming when that happens, a
>> plethora of race conditions can occur.
>>
>> Make sure that the device has stopped streaming before exiting this
>> function.
>>
>> If the user still holds handles to the driver's file descriptors, any
>> ioctl will return -ENODEV from the v4l2 core.
>>
>> This change makes uvc more consistent with the rest of the v4l2 drivers
>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index bbd90123a4e7..55132688e363 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>>  	struct uvc_streaming *stream;
>>  
>>  	list_for_each_entry(stream, &dev->streams, list) {
>> +		/* Nothing to do here, continue. */
>>  		if (!video_is_registered(&stream->vdev))
>>  			continue;
>>  
>> +		/*
>> +		 * For stream->vdev we follow the same logic as:
>> +		 * vb2_video_unregister_device().
>> +		 */
>> +
>> +		/* 1. Take a reference to vdev */
>> +		get_device(&stream->vdev.dev);
> 
> This ensures that the device refcount won't go to 0 if video_unregister_device
> is called (which calls put_device).
> 
> But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
> then that open filehandle also called get_device(). So while that application is
> waiting, the device refcount will never go to 0.
> 
>> +
>> +		/* 2. Ensure that no new ioctls can be called. */
>>  		video_unregister_device(&stream->vdev);
>> -		video_unregister_device(&stream->meta.vdev);
>> +
>> +		/* 3. Wait for old ioctls to finish. */
>> +		mutex_lock(&stream->mutex);
> 
> If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
> lock here. So in that case this won't wait for that specific ioctl to finish.
> 
>> +
>> +		/* 4. Stop streaming. */
>> +		uvc_queue_release(&stream->queue);
> 
> This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
> buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
> waiting for the mutex to become available.
> 
>> +
>> +		mutex_unlock(&stream->mutex);
> 
> At this point it can take the mutex again. But since q->streaming is now false,
> (due to the __vb2_queue_cancel call) this will return an error which is returned
> to userspace.
> 
>> +
>> +		put_device(&stream->vdev.dev);
> 
> This releases the reference we took earlier. If the application has already closed
> the filehandle, then this will release all memory. If the application still has the
> fh open, then only when it closes that fh will the memory be released.
> 
> Conclusion: there is no race condition here, this is handled correctly by the core.
> 
>> +
>> +		/*
>> +		 * For stream->meta.vdev we can directly call:
>> +		 * vb2_video_unregister_device().
>> +		 */
>> +		vb2_video_unregister_device(&stream->meta.vdev);
> 
> Perhaps a patch adding more comments to the vb2_video_unregister_device()
> function might help document this sequence better.
> 
> Regards,
> 
> 	Hans
> 
>> +
>> +		/*
>> +		 * Now both vdevs are not streaming and all the ioctls will
>> +		 * return -ENODEV.
>> +		 */
>>  
>>  		uvc_debugfs_cleanup_stream(stream);
>>  	}
>>
> 
> 

Re: [PATCH v6 2/4] media: uvcvideo: Refactor the status irq API

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Jun 14, 2024 at 12:41:28PM +0000, Ricardo Ribalda wrote:
> There are two different use-cases of uvc_status():

I'd add a blank line here, and remove the leading tabs from the next two
lines.

> 	- adding/removing a user when the camera is open/closed
> 	- stopping/starting when the camera is suspended/resumed
> 
> Make the API reflect these two use-cases and move all the refcounting
> and locking logic to the uvc_status.c file.

If my understanding is correct, this doesn't introduce any functional
change, and is not a dependency of further patches in the series. I
don't have a strong opinion on the refactoring itself, so I'm fine with
it, but I'd like a mention in the commit message that no functional
change is introduced.

> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 13 ++-------
>  drivers/media/usb/uvc/uvc_status.c | 55 ++++++++++++++++++++++++++++++++++++--
>  drivers/media/usb/uvc/uvc_v4l2.c   | 22 +++++----------
>  drivers/media/usb/uvc/uvcvideo.h   | 10 ++++---
>  4 files changed, 67 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 55132688e363..c8c0352af1e5 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2135,7 +2135,6 @@ static int uvc_probe(struct usb_interface *intf,
>  	INIT_LIST_HEAD(&dev->streams);
>  	kref_init(&dev->ref);
>  	atomic_set(&dev->nmappings, 0);
> -	mutex_init(&dev->lock);
>  
>  	dev->udev = usb_get_dev(udev);
>  	dev->intf = usb_get_intf(intf);
> @@ -2301,10 +2300,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
>  	/* Controls are cached on the fly so they don't need to be saved. */
>  	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
>  	    UVC_SC_VIDEOCONTROL) {
> -		mutex_lock(&dev->lock);
> -		if (dev->users)
> -			uvc_status_stop(dev);
> -		mutex_unlock(&dev->lock);
> +		uvc_status_suspend(dev);
>  		return 0;
>  	}
>  
> @@ -2335,12 +2331,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
>  				return ret;
>  		}
>  
> -		mutex_lock(&dev->lock);
> -		if (dev->users)
> -			ret = uvc_status_start(dev, GFP_NOIO);
> -		mutex_unlock(&dev->lock);
> -
> -		return ret;
> +		return uvc_status_resume(dev);
>  	}
>  
>  	list_for_each_entry(stream, &dev->streams, list) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index a78a88c710e2..375a95dd3011 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -257,6 +257,8 @@ int uvc_status_init(struct uvc_device *dev)
>  	unsigned int pipe;
>  	int interval;
>  
> +	mutex_init(&dev->status_lock);
> +
>  	if (ep == NULL)
>  		return 0;
>  
> @@ -302,18 +304,22 @@ void uvc_status_cleanup(struct uvc_device *dev)
>  	kfree(dev->status);
>  }
>  
> -int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> +static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)

Do we need a double underscore prefix here and for the stop() function ?

>  {
> +	lockdep_assert_held(&dev->status_lock);
> +
>  	if (dev->int_urb == NULL)
>  		return 0;
>  
>  	return usb_submit_urb(dev->int_urb, flags);
>  }
>  
> -void uvc_status_stop(struct uvc_device *dev)
> +static void __uvc_status_stop(struct uvc_device *dev)
>  {
>  	struct uvc_ctrl_work *w = &dev->async_ctrl;
>  
> +	lockdep_assert_held(&dev->status_lock);
> +
>  	/*
>  	 * Prevent the asynchronous control handler from requeing the URB. The
>  	 * barrier is needed so the flush_status change is visible to other
> @@ -350,3 +356,48 @@ void uvc_status_stop(struct uvc_device *dev)
>  	 */
>  	smp_store_release(&dev->flush_status, false);
>  }
> +
> +int uvc_status_resume(struct uvc_device *dev)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&dev->status_lock);
> +	if (dev->status_users)
> +		ret = __uvc_status_start(dev, GFP_NOIO);
> +	mutex_unlock(&dev->status_lock);
> +
> +	return ret;

Now that we have scoped guards, this can be written as

	guard(mutex)(&dev->status_lock);

	if (!dev->status_users)
		return 0;

	return __uvc_status_start(dev, GFP_NOIO);

> +}
> +
> +void uvc_status_suspend(struct uvc_device *dev)
> +{
> +	mutex_lock(&dev->status_lock);
> +	if (dev->status_users)
> +		__uvc_status_stop(dev);
> +	mutex_unlock(&dev->status_lock);

	guard(mutex)(&dev->status_lock);

	if (dev->status_users)
		__uvc_status_stop(dev);

Same below.

> +}
> +
> +int uvc_status_get(struct uvc_device *dev)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&dev->status_lock);
> +	if (!dev->status_users)
> +		ret = __uvc_status_start(dev, GFP_KERNEL);
> +	if (!ret)
> +		dev->status_users++;
> +	mutex_unlock(&dev->status_lock);
> +
> +	return ret;
> +}
> +
> +void uvc_status_put(struct uvc_device *dev)
> +{
> +	mutex_lock(&dev->status_lock);
> +	if (dev->status_users == 1)
> +		__uvc_status_stop(dev);
> +	WARN_ON(!dev->status_users);

Is this needed, or could we keep the original code ?

	if (--dev->status_users == 0)
		__uvc_status_stop(dev);

All those comments are quite minor, the next version will have my R-b.

> +	if (dev->status_users)
> +		dev->status_users--;
> +	mutex_unlock(&dev->status_lock);
> +}
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4988f03640a..97c5407f6603 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -628,20 +628,13 @@ static int uvc_v4l2_open(struct file *file)
>  		return -ENOMEM;
>  	}
>  
> -	mutex_lock(&stream->dev->lock);
> -	if (stream->dev->users == 0) {
> -		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> -		if (ret < 0) {
> -			mutex_unlock(&stream->dev->lock);
> -			usb_autopm_put_interface(stream->dev->intf);
> -			kfree(handle);
> -			return ret;
> -		}
> +	ret = uvc_status_get(stream->dev);
> +	if (ret) {
> +		usb_autopm_put_interface(stream->dev->intf);
> +		kfree(handle);
> +		return ret;
>  	}
>  
> -	stream->dev->users++;
> -	mutex_unlock(&stream->dev->lock);
> -
>  	v4l2_fh_init(&handle->vfh, &stream->vdev);
>  	v4l2_fh_add(&handle->vfh);
>  	handle->chain = stream->chain;
> @@ -670,10 +663,7 @@ static int uvc_v4l2_release(struct file *file)
>  	kfree(handle);
>  	file->private_data = NULL;
>  
> -	mutex_lock(&stream->dev->lock);
> -	if (--stream->dev->users == 0)
> -		uvc_status_stop(stream->dev);
> -	mutex_unlock(&stream->dev->lock);
> +	uvc_status_put(stream->dev);
>  
>  	usb_autopm_put_interface(stream->dev->intf);
>  	return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 6fb0a78b1b00..00b600eb058c 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -555,8 +555,6 @@ struct uvc_device {
>  
>  	const struct uvc_device_info *info;
>  
> -	struct mutex lock;		/* Protects users */
> -	unsigned int users;
>  	atomic_t nmappings;
>  
>  	/* Video control interface */
> @@ -578,6 +576,8 @@ struct uvc_device {
>  	struct usb_host_endpoint *int_ep;
>  	struct urb *int_urb;
>  	struct uvc_status *status;
> +	struct mutex status_lock; /* Protects status_users */
> +	unsigned int status_users;
>  	bool flush_status;
>  
>  	struct input_dev *input;
> @@ -744,8 +744,10 @@ int uvc_register_video_device(struct uvc_device *dev,
>  int uvc_status_init(struct uvc_device *dev);
>  void uvc_status_unregister(struct uvc_device *dev);
>  void uvc_status_cleanup(struct uvc_device *dev);
> -int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> -void uvc_status_stop(struct uvc_device *dev);
> +int uvc_status_resume(struct uvc_device *dev);
> +void uvc_status_suspend(struct uvc_device *dev);
> +int uvc_status_get(struct uvc_device *dev);
> +void uvc_status_put(struct uvc_device *dev);
>  
>  /* Controls */
>  extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 3/4] media: uvcvideo: Avoid race condition during unregister

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

I'd write "during status unregister" in the subject line.

On Fri, Jun 14, 2024 at 12:41:29PM +0000, Ricardo Ribalda wrote:
> The control events are handled asynchronously by the driver. Once the
> control event are handled, the urb is re-submitted.
> 
> If we simply kill the urb, there is a chance that a control event is
> waiting to be processed, which will re-submit the urb after the device is
> disconnected.
> 
> uvc_status_suspend() flushes the async controls and stops the urb in a
> correct manner.

"Fix this by calling uvc_status_suspend(), which flushes the async
controls and kills the URB in a race-free manner."

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 375a95dd3011..8fd8250110e2 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -294,7 +294,7 @@ int uvc_status_init(struct uvc_device *dev)
>  
>  void uvc_status_unregister(struct uvc_device *dev)
>  {
> -	usb_kill_urb(dev->int_urb);
> +	uvc_status_suspend(dev);
>  	uvc_input_unregister(dev);
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 2/4] media: uvcvideo: Refactor the status irq API

[Laurent Pinchart]

On Wed, Sep 25, 2024 at 10:15:39PM +0300, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Fri, Jun 14, 2024 at 12:41:28PM +0000, Ricardo Ribalda wrote:
> > There are two different use-cases of uvc_status():
> 
> I'd add a blank line here, and remove the leading tabs from the next two
> lines.
> 
> > 	- adding/removing a user when the camera is open/closed
> > 	- stopping/starting when the camera is suspended/resumed
> > 
> > Make the API reflect these two use-cases and move all the refcounting
> > and locking logic to the uvc_status.c file.
> 
> If my understanding is correct, this doesn't introduce any functional
> change, and is not a dependency of further patches in the series. I
> don't have a strong opinion on the refactoring itself, so I'm fine with
> it, but I'd like a mention in the commit message that no functional
> change is introduced.
> 
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 13 ++-------
> >  drivers/media/usb/uvc/uvc_status.c | 55 ++++++++++++++++++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c   | 22 +++++----------
> >  drivers/media/usb/uvc/uvcvideo.h   | 10 ++++---
> >  4 files changed, 67 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 55132688e363..c8c0352af1e5 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2135,7 +2135,6 @@ static int uvc_probe(struct usb_interface *intf,
> >  	INIT_LIST_HEAD(&dev->streams);
> >  	kref_init(&dev->ref);
> >  	atomic_set(&dev->nmappings, 0);
> > -	mutex_init(&dev->lock);
> >  
> >  	dev->udev = usb_get_dev(udev);
> >  	dev->intf = usb_get_intf(intf);
> > @@ -2301,10 +2300,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
> >  	/* Controls are cached on the fly so they don't need to be saved. */
> >  	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> >  	    UVC_SC_VIDEOCONTROL) {
> > -		mutex_lock(&dev->lock);
> > -		if (dev->users)
> > -			uvc_status_stop(dev);
> > -		mutex_unlock(&dev->lock);
> > +		uvc_status_suspend(dev);
> >  		return 0;
> >  	}
> >  
> > @@ -2335,12 +2331,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
> >  				return ret;
> >  		}
> >  
> > -		mutex_lock(&dev->lock);
> > -		if (dev->users)
> > -			ret = uvc_status_start(dev, GFP_NOIO);
> > -		mutex_unlock(&dev->lock);
> > -
> > -		return ret;
> > +		return uvc_status_resume(dev);
> >  	}
> >  
> >  	list_for_each_entry(stream, &dev->streams, list) {
> > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> > index a78a88c710e2..375a95dd3011 100644
> > --- a/drivers/media/usb/uvc/uvc_status.c
> > +++ b/drivers/media/usb/uvc/uvc_status.c
> > @@ -257,6 +257,8 @@ int uvc_status_init(struct uvc_device *dev)
> >  	unsigned int pipe;
> >  	int interval;
> >  
> > +	mutex_init(&dev->status_lock);
> > +
> >  	if (ep == NULL)
> >  		return 0;
> >  
> > @@ -302,18 +304,22 @@ void uvc_status_cleanup(struct uvc_device *dev)
> >  	kfree(dev->status);
> >  }
> >  
> > -int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> > +static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
> 
> Do we need a double underscore prefix here and for the stop() function ?

The uvc_status_start() function is also referenced in comments in this
file. If you keep the double undescore prefix they need to be renamed.

> >  {
> > +	lockdep_assert_held(&dev->status_lock);
> > +
> >  	if (dev->int_urb == NULL)
> >  		return 0;
> >  
> >  	return usb_submit_urb(dev->int_urb, flags);
> >  }
> >  
> > -void uvc_status_stop(struct uvc_device *dev)
> > +static void __uvc_status_stop(struct uvc_device *dev)
> >  {
> >  	struct uvc_ctrl_work *w = &dev->async_ctrl;
> >  
> > +	lockdep_assert_held(&dev->status_lock);
> > +
> >  	/*
> >  	 * Prevent the asynchronous control handler from requeing the URB. The
> >  	 * barrier is needed so the flush_status change is visible to other
> > @@ -350,3 +356,48 @@ void uvc_status_stop(struct uvc_device *dev)
> >  	 */
> >  	smp_store_release(&dev->flush_status, false);
> >  }
> > +
> > +int uvc_status_resume(struct uvc_device *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&dev->status_lock);
> > +	if (dev->status_users)
> > +		ret = __uvc_status_start(dev, GFP_NOIO);
> > +	mutex_unlock(&dev->status_lock);
> > +
> > +	return ret;
> 
> Now that we have scoped guards, this can be written as
> 
> 	guard(mutex)(&dev->status_lock);
> 
> 	if (!dev->status_users)
> 		return 0;
> 
> 	return __uvc_status_start(dev, GFP_NOIO);
> 
> > +}
> > +
> > +void uvc_status_suspend(struct uvc_device *dev)
> > +{
> > +	mutex_lock(&dev->status_lock);
> > +	if (dev->status_users)
> > +		__uvc_status_stop(dev);
> > +	mutex_unlock(&dev->status_lock);
> 
> 	guard(mutex)(&dev->status_lock);
> 
> 	if (dev->status_users)
> 		__uvc_status_stop(dev);
> 
> Same below.
> 
> > +}
> > +
> > +int uvc_status_get(struct uvc_device *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&dev->status_lock);
> > +	if (!dev->status_users)
> > +		ret = __uvc_status_start(dev, GFP_KERNEL);
> > +	if (!ret)
> > +		dev->status_users++;
> > +	mutex_unlock(&dev->status_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +void uvc_status_put(struct uvc_device *dev)
> > +{
> > +	mutex_lock(&dev->status_lock);
> > +	if (dev->status_users == 1)
> > +		__uvc_status_stop(dev);
> > +	WARN_ON(!dev->status_users);
> 
> Is this needed, or could we keep the original code ?
> 
> 	if (--dev->status_users == 0)
> 		__uvc_status_stop(dev);
> 
> All those comments are quite minor, the next version will have my R-b.
> 
> > +	if (dev->status_users)
> > +		dev->status_users--;
> > +	mutex_unlock(&dev->status_lock);
> > +}
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index f4988f03640a..97c5407f6603 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -628,20 +628,13 @@ static int uvc_v4l2_open(struct file *file)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	mutex_lock(&stream->dev->lock);
> > -	if (stream->dev->users == 0) {
> > -		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> > -		if (ret < 0) {
> > -			mutex_unlock(&stream->dev->lock);
> > -			usb_autopm_put_interface(stream->dev->intf);
> > -			kfree(handle);
> > -			return ret;
> > -		}
> > +	ret = uvc_status_get(stream->dev);
> > +	if (ret) {
> > +		usb_autopm_put_interface(stream->dev->intf);
> > +		kfree(handle);
> > +		return ret;
> >  	}
> >  
> > -	stream->dev->users++;
> > -	mutex_unlock(&stream->dev->lock);
> > -
> >  	v4l2_fh_init(&handle->vfh, &stream->vdev);
> >  	v4l2_fh_add(&handle->vfh);
> >  	handle->chain = stream->chain;
> > @@ -670,10 +663,7 @@ static int uvc_v4l2_release(struct file *file)
> >  	kfree(handle);
> >  	file->private_data = NULL;
> >  
> > -	mutex_lock(&stream->dev->lock);
> > -	if (--stream->dev->users == 0)
> > -		uvc_status_stop(stream->dev);
> > -	mutex_unlock(&stream->dev->lock);
> > +	uvc_status_put(stream->dev);
> >  
> >  	usb_autopm_put_interface(stream->dev->intf);
> >  	return 0;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 6fb0a78b1b00..00b600eb058c 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -555,8 +555,6 @@ struct uvc_device {
> >  
> >  	const struct uvc_device_info *info;
> >  
> > -	struct mutex lock;		/* Protects users */
> > -	unsigned int users;
> >  	atomic_t nmappings;
> >  
> >  	/* Video control interface */
> > @@ -578,6 +576,8 @@ struct uvc_device {
> >  	struct usb_host_endpoint *int_ep;
> >  	struct urb *int_urb;
> >  	struct uvc_status *status;
> > +	struct mutex status_lock; /* Protects status_users */
> > +	unsigned int status_users;
> >  	bool flush_status;
> >  
> >  	struct input_dev *input;
> > @@ -744,8 +744,10 @@ int uvc_register_video_device(struct uvc_device *dev,
> >  int uvc_status_init(struct uvc_device *dev);
> >  void uvc_status_unregister(struct uvc_device *dev);
> >  void uvc_status_cleanup(struct uvc_device *dev);
> > -int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> > -void uvc_status_stop(struct uvc_device *dev);
> > +int uvc_status_resume(struct uvc_device *dev);
> > +void uvc_status_suspend(struct uvc_device *dev);
> > +int uvc_status_get(struct uvc_device *dev);
> > +void uvc_status_put(struct uvc_device *dev);
> >  
> >  /* Controls */
> >  extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/4] media: uvcvideo: Exit early if there is not int_urb

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Jun 14, 2024 at 12:41:30PM +0000, Ricardo Ribalda wrote:
> If there is no int_urb there is no need to do a clean stop.
> 
> Also we avoid calling usb_kill_urb(NULL). It is properly handled by the
> usb framework, but it is not polite.
> 
> Now that we are at it, fix the code style in uvc_status_start() for
> consistency.
> 
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_status.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 8fd8250110e2..9108522beea6 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -308,7 +308,7 @@ static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
>  {
>  	lockdep_assert_held(&dev->status_lock);
>  
> -	if (dev->int_urb == NULL)
> +	if (!dev->int_urb)
>  		return 0;
>  
>  	return usb_submit_urb(dev->int_urb, flags);
> @@ -320,6 +320,9 @@ static void __uvc_status_stop(struct uvc_device *dev)
>  
>  	lockdep_assert_held(&dev->status_lock);
>  
> +	if (!dev->int_urb)
> +		return;
> +
>  	/*
>  	 * Prevent the asynchronous control handler from requeing the URB. The
>  	 * barrier is needed so the flush_status change is visible to other
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Laurent Pinchart]

Hi Hans,

On Wed, Sep 25, 2024 at 11:50:41AM +0200, Hans Verkuil wrote:
> On 25/09/2024 10:32, Hans Verkuil wrote:
> > Hi Laurent,
> > 
> > We discussed this patch last week, and you thought that there was still
> > a race condition if the uvc device was unplugged while an application was
> > in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
> > op is called, which unlocks the serialization mutex).
> > 
> > I'll go through the code below, explaining why that isn't an issue.
> 
> Update: I added an extra check for this scenario to the test-media script to make
> sure we catch any potential regressions in how this is handled in the core.

I'm recovering from Vienna and I'll review your explanation towards the
end of the week.

Ricardo, patches 2/4 to 4/4 are not controversial. 2/4 needs a new
version to address small issues. As far as I understand, they don't
depend on 1/4. Would you submit a new version of them that I can merge
right away ?

> > On 14/06/2024 14:41, Ricardo Ribalda wrote:
> >> uvc_unregister_video() can be called asynchronously from
> >> uvc_disconnect(). If the device is still streaming when that happens, a
> >> plethora of race conditions can occur.
> >>
> >> Make sure that the device has stopped streaming before exiting this
> >> function.
> >>
> >> If the user still holds handles to the driver's file descriptors, any
> >> ioctl will return -ENODEV from the v4l2 core.
> >>
> >> This change makes uvc more consistent with the rest of the v4l2 drivers
> >> using the vb2_fop_* and vb2_ioctl_* helpers.
> >>
> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >> ---
> >>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
> >>  1 file changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >> index bbd90123a4e7..55132688e363 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >>  	struct uvc_streaming *stream;
> >>  
> >>  	list_for_each_entry(stream, &dev->streams, list) {
> >> +		/* Nothing to do here, continue. */
> >>  		if (!video_is_registered(&stream->vdev))
> >>  			continue;
> >>  
> >> +		/*
> >> +		 * For stream->vdev we follow the same logic as:
> >> +		 * vb2_video_unregister_device().
> >> +		 */
> >> +
> >> +		/* 1. Take a reference to vdev */
> >> +		get_device(&stream->vdev.dev);
> > 
> > This ensures that the device refcount won't go to 0 if video_unregister_device
> > is called (which calls put_device).
> > 
> > But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
> > then that open filehandle also called get_device(). So while that application is
> > waiting, the device refcount will never go to 0.
> > 
> >> +
> >> +		/* 2. Ensure that no new ioctls can be called. */
> >>  		video_unregister_device(&stream->vdev);
> >> -		video_unregister_device(&stream->meta.vdev);
> >> +
> >> +		/* 3. Wait for old ioctls to finish. */
> >> +		mutex_lock(&stream->mutex);
> > 
> > If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
> > lock here. So in that case this won't wait for that specific ioctl to finish.
> > 
> >> +
> >> +		/* 4. Stop streaming. */
> >> +		uvc_queue_release(&stream->queue);
> > 
> > This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
> > buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
> > waiting for the mutex to become available.
> > 
> >> +
> >> +		mutex_unlock(&stream->mutex);
> > 
> > At this point it can take the mutex again. But since q->streaming is now false,
> > (due to the __vb2_queue_cancel call) this will return an error which is returned
> > to userspace.
> > 
> >> +
> >> +		put_device(&stream->vdev.dev);
> > 
> > This releases the reference we took earlier. If the application has already closed
> > the filehandle, then this will release all memory. If the application still has the
> > fh open, then only when it closes that fh will the memory be released.
> > 
> > Conclusion: there is no race condition here, this is handled correctly by the core.
> > 
> >> +
> >> +		/*
> >> +		 * For stream->meta.vdev we can directly call:
> >> +		 * vb2_video_unregister_device().
> >> +		 */
> >> +		vb2_video_unregister_device(&stream->meta.vdev);
> > 
> > Perhaps a patch adding more comments to the vb2_video_unregister_device()
> > function might help document this sequence better.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> >> +
> >> +		/*
> >> +		 * Now both vdevs are not streaming and all the ioctls will
> >> +		 * return -ENODEV.
> >> +		 */
> >>  
> >>  		uvc_debugfs_cleanup_stream(stream);
> >>  	}
> >>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 2/4] media: uvcvideo: Refactor the status irq API

[Ricardo Ribalda]

Hi Laurent

On Wed, 25 Sept 2024 at 21:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Jun 14, 2024 at 12:41:28PM +0000, Ricardo Ribalda wrote:
> > There are two different use-cases of uvc_status():
>
> I'd add a blank line here, and remove the leading tabs from the next two
> lines.
>
> >       - adding/removing a user when the camera is open/closed
> >       - stopping/starting when the camera is suspended/resumed
> >
> > Make the API reflect these two use-cases and move all the refcounting
> > and locking logic to the uvc_status.c file.
>
> If my understanding is correct, this doesn't introduce any functional
> change, and is not a dependency of further patches in the series. I
> don't have a strong opinion on the refactoring itself, so I'm fine with
> it, but I'd like a mention in the commit message that no functional
> change is introduced.
>
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 13 ++-------
> >  drivers/media/usb/uvc/uvc_status.c | 55 ++++++++++++++++++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c   | 22 +++++----------
> >  drivers/media/usb/uvc/uvcvideo.h   | 10 ++++---
> >  4 files changed, 67 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 55132688e363..c8c0352af1e5 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2135,7 +2135,6 @@ static int uvc_probe(struct usb_interface *intf,
> >       INIT_LIST_HEAD(&dev->streams);
> >       kref_init(&dev->ref);
> >       atomic_set(&dev->nmappings, 0);
> > -     mutex_init(&dev->lock);
> >
> >       dev->udev = usb_get_dev(udev);
> >       dev->intf = usb_get_intf(intf);
> > @@ -2301,10 +2300,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
> >       /* Controls are cached on the fly so they don't need to be saved. */
> >       if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> >           UVC_SC_VIDEOCONTROL) {
> > -             mutex_lock(&dev->lock);
> > -             if (dev->users)
> > -                     uvc_status_stop(dev);
> > -             mutex_unlock(&dev->lock);
> > +             uvc_status_suspend(dev);
> >               return 0;
> >       }
> >
> > @@ -2335,12 +2331,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
> >                               return ret;
> >               }
> >
> > -             mutex_lock(&dev->lock);
> > -             if (dev->users)
> > -                     ret = uvc_status_start(dev, GFP_NOIO);
> > -             mutex_unlock(&dev->lock);
> > -
> > -             return ret;
> > +             return uvc_status_resume(dev);
> >       }
> >
> >       list_for_each_entry(stream, &dev->streams, list) {
> > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> > index a78a88c710e2..375a95dd3011 100644
> > --- a/drivers/media/usb/uvc/uvc_status.c
> > +++ b/drivers/media/usb/uvc/uvc_status.c
> > @@ -257,6 +257,8 @@ int uvc_status_init(struct uvc_device *dev)
> >       unsigned int pipe;
> >       int interval;
> >
> > +     mutex_init(&dev->status_lock);
> > +
> >       if (ep == NULL)
> >               return 0;
> >
> > @@ -302,18 +304,22 @@ void uvc_status_cleanup(struct uvc_device *dev)
> >       kfree(dev->status);
> >  }
> >
> > -int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> > +static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
>
> Do we need a double underscore prefix here and for the stop() function ?
>
> >  {
> > +     lockdep_assert_held(&dev->status_lock);
> > +
> >       if (dev->int_urb == NULL)
> >               return 0;
> >
> >       return usb_submit_urb(dev->int_urb, flags);
> >  }
> >
> > -void uvc_status_stop(struct uvc_device *dev)
> > +static void __uvc_status_stop(struct uvc_device *dev)
> >  {
> >       struct uvc_ctrl_work *w = &dev->async_ctrl;
> >
> > +     lockdep_assert_held(&dev->status_lock);
> > +
> >       /*
> >        * Prevent the asynchronous control handler from requeing the URB. The
> >        * barrier is needed so the flush_status change is visible to other
> > @@ -350,3 +356,48 @@ void uvc_status_stop(struct uvc_device *dev)
> >        */
> >       smp_store_release(&dev->flush_status, false);
> >  }
> > +
> > +int uvc_status_resume(struct uvc_device *dev)
> > +{
> > +     int ret = 0;
> > +
> > +     mutex_lock(&dev->status_lock);
> > +     if (dev->status_users)
> > +             ret = __uvc_status_start(dev, GFP_NOIO);
> > +     mutex_unlock(&dev->status_lock);
> > +
> > +     return ret;
>
> Now that we have scoped guards, this can be written as
>
>         guard(mutex)(&dev->status_lock);
>
>         if (!dev->status_users)
>                 return 0;
>
>         return __uvc_status_start(dev, GFP_NOIO);
>
> > +}
> > +
> > +void uvc_status_suspend(struct uvc_device *dev)
> > +{
> > +     mutex_lock(&dev->status_lock);
> > +     if (dev->status_users)
> > +             __uvc_status_stop(dev);
> > +     mutex_unlock(&dev->status_lock);
>
>         guard(mutex)(&dev->status_lock);
>
>         if (dev->status_users)
>                 __uvc_status_stop(dev);
>
> Same below.
>
> > +}
> > +
> > +int uvc_status_get(struct uvc_device *dev)
> > +{
> > +     int ret = 0;
> > +
> > +     mutex_lock(&dev->status_lock);
> > +     if (!dev->status_users)
> > +             ret = __uvc_status_start(dev, GFP_KERNEL);
> > +     if (!ret)
> > +             dev->status_users++;
> > +     mutex_unlock(&dev->status_lock);
> > +
> > +     return ret;
> > +}
> > +
> > +void uvc_status_put(struct uvc_device *dev)
> > +{
> > +     mutex_lock(&dev->status_lock);
> > +     if (dev->status_users == 1)
> > +             __uvc_status_stop(dev);
> > +     WARN_ON(!dev->status_users);
>
> Is this needed, or could we keep the original code ?
>
>         if (--dev->status_users == 0)
>                 __uvc_status_stop(dev);

I'd prefer to keep the code. When I was playing with the granular PM
it was very nice to display a WARN and make sure that status_users was
never underflowed.


>
> All those comments are quite minor, the next version will have my R-b.
>
> > +     if (dev->status_users)
> > +             dev->status_users--;
> > +     mutex_unlock(&dev->status_lock);
> > +}
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index f4988f03640a..97c5407f6603 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -628,20 +628,13 @@ static int uvc_v4l2_open(struct file *file)
> >               return -ENOMEM;
> >       }
> >
> > -     mutex_lock(&stream->dev->lock);
> > -     if (stream->dev->users == 0) {
> > -             ret = uvc_status_start(stream->dev, GFP_KERNEL);
> > -             if (ret < 0) {
> > -                     mutex_unlock(&stream->dev->lock);
> > -                     usb_autopm_put_interface(stream->dev->intf);
> > -                     kfree(handle);
> > -                     return ret;
> > -             }
> > +     ret = uvc_status_get(stream->dev);
> > +     if (ret) {
> > +             usb_autopm_put_interface(stream->dev->intf);
> > +             kfree(handle);
> > +             return ret;
> >       }
> >
> > -     stream->dev->users++;
> > -     mutex_unlock(&stream->dev->lock);
> > -
> >       v4l2_fh_init(&handle->vfh, &stream->vdev);
> >       v4l2_fh_add(&handle->vfh);
> >       handle->chain = stream->chain;
> > @@ -670,10 +663,7 @@ static int uvc_v4l2_release(struct file *file)
> >       kfree(handle);
> >       file->private_data = NULL;
> >
> > -     mutex_lock(&stream->dev->lock);
> > -     if (--stream->dev->users == 0)
> > -             uvc_status_stop(stream->dev);
> > -     mutex_unlock(&stream->dev->lock);
> > +     uvc_status_put(stream->dev);
> >
> >       usb_autopm_put_interface(stream->dev->intf);
> >       return 0;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 6fb0a78b1b00..00b600eb058c 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -555,8 +555,6 @@ struct uvc_device {
> >
> >       const struct uvc_device_info *info;
> >
> > -     struct mutex lock;              /* Protects users */
> > -     unsigned int users;
> >       atomic_t nmappings;
> >
> >       /* Video control interface */
> > @@ -578,6 +576,8 @@ struct uvc_device {
> >       struct usb_host_endpoint *int_ep;
> >       struct urb *int_urb;
> >       struct uvc_status *status;
> > +     struct mutex status_lock; /* Protects status_users */
> > +     unsigned int status_users;
> >       bool flush_status;
> >
> >       struct input_dev *input;
> > @@ -744,8 +744,10 @@ int uvc_register_video_device(struct uvc_device *dev,
> >  int uvc_status_init(struct uvc_device *dev);
> >  void uvc_status_unregister(struct uvc_device *dev);
> >  void uvc_status_cleanup(struct uvc_device *dev);
> > -int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> > -void uvc_status_stop(struct uvc_device *dev);
> > +int uvc_status_resume(struct uvc_device *dev);
> > +void uvc_status_suspend(struct uvc_device *dev);
> > +int uvc_status_get(struct uvc_device *dev);
> > +void uvc_status_put(struct uvc_device *dev);
> >
> >  /* Controls */
> >  extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

On 25/09/2024 10:32, Hans Verkuil wrote:
> Hi Laurent,
> 
> We discussed this patch last week, and you thought that there was still
> a race condition if the uvc device was unplugged while an application was
> in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
> op is called, which unlocks the serialization mutex).
> 
> I'll go through the code below, explaining why that isn't an issue.
> 
> On 14/06/2024 14:41, Ricardo Ribalda wrote:
>> uvc_unregister_video() can be called asynchronously from
>> uvc_disconnect(). If the device is still streaming when that happens, a
>> plethora of race conditions can occur.
>>
>> Make sure that the device has stopped streaming before exiting this
>> function.
>>
>> If the user still holds handles to the driver's file descriptors, any
>> ioctl will return -ENODEV from the v4l2 core.
>>
>> This change makes uvc more consistent with the rest of the v4l2 drivers
>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index bbd90123a4e7..55132688e363 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>>  	struct uvc_streaming *stream;
>>  
>>  	list_for_each_entry(stream, &dev->streams, list) {
>> +		/* Nothing to do here, continue. */
>>  		if (!video_is_registered(&stream->vdev))
>>  			continue;
>>  
>> +		/*
>> +		 * For stream->vdev we follow the same logic as:
>> +		 * vb2_video_unregister_device().
>> +		 */
>> +
>> +		/* 1. Take a reference to vdev */
>> +		get_device(&stream->vdev.dev);
> 
> This ensures that the device refcount won't go to 0 if video_unregister_device
> is called (which calls put_device).
> 
> But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
> then that open filehandle also called get_device(). So while that application is
> waiting, the device refcount will never go to 0.
> 
>> +
>> +		/* 2. Ensure that no new ioctls can be called. */
>>  		video_unregister_device(&stream->vdev);

After calling video_unregister_device, the /dev/videoX device disappears, so
nobody can open it again, and any file operations other than close() will
return an error.

>> -		video_unregister_device(&stream->meta.vdev);
>> +
>> +		/* 3. Wait for old ioctls to finish. */
>> +		mutex_lock(&stream->mutex);
> 
> If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
> lock here. So in that case this won't wait for that specific ioctl to finish.
> 
>> +
>> +		/* 4. Stop streaming. */
>> +		uvc_queue_release(&stream->queue);
> 
> This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
> buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
> waiting for the mutex to become available.

vb2_core_queue_release() calls __vb2_queue_cancel and also __vb2_queue_free(),
and that last call is done with mmap_lock held. This ensures that it safely
serializes with mmap() calls: either mmap() succeeded before the queue buffers
are freed (and so a mapping will still exist for a buffer), or the queue buffers
were freed first and mmap will return an error since the buffer it wanted to
mmap is already gone.

So the handling of mmap() is also safe to the best of my knowledge.

I remembered we talked briefly about this as well during the conference, so I
wanted to describe this as well.

Regards,

	Hans

> 
>> +
>> +		mutex_unlock(&stream->mutex);
> 
> At this point it can take the mutex again. But since q->streaming is now false,
> (due to the __vb2_queue_cancel call) this will return an error which is returned
> to userspace.
> 
>> +
>> +		put_device(&stream->vdev.dev);
> 
> This releases the reference we took earlier. If the application has already closed
> the filehandle, then this will release all memory. If the application still has the
> fh open, then only when it closes that fh will the memory be released.
> 
> Conclusion: there is no race condition here, this is handled correctly by the core.
> 
>> +
>> +		/*
>> +		 * For stream->meta.vdev we can directly call:
>> +		 * vb2_video_unregister_device().
>> +		 */
>> +		vb2_video_unregister_device(&stream->meta.vdev);
> 
> Perhaps a patch adding more comments to the vb2_video_unregister_device()
> function might help document this sequence better.
> 
> Regards,
> 
> 	Hans
> 
>> +
>> +		/*
>> +		 * Now both vdevs are not streaming and all the ioctls will
>> +		 * return -ENODEV.
>> +		 */
>>  
>>  		uvc_debugfs_cleanup_stream(stream);
>>  	}
>>
> 
>