[PATCH v2 0/2] media: uvcvideo: Fix race condition on metadata buffers

[PATCH v2 0/2] media: uvcvideo: Fix race condition on metadata buffers

[Ricardo Ribalda]

This series fixes a race condition when calling streamoff only on the
metada queue while streaming.

The first patch fixes the race condition and the second patch replaces a
busy wait with a waitqueue. It is probably overkilled and this is why it
is a follow-up patch.

Feel free to apply the first patch, both, or squash them into one patch.

Regards!!

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2:
- Use a new flag in_flight to avoid keeping the spinlock for long
  periods of time
- Link to v1: https://lore.kernel.org/r/20250714-uvc-racemeta-v1-1-360de2e15a9a@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Fix race condition for meta buffer list
      media: uvcvideo: Use wait queue for metadata streamoff

 drivers/media/usb/uvc/uvc_queue.c | 11 +++++++++++
 drivers/media/usb/uvc/uvc_video.c | 32 +++++++++++++++++++++++++++++++-
 drivers/media/usb/uvc/uvcvideo.h  |  3 +++
 3 files changed, 45 insertions(+), 1 deletion(-)
---
base-commit: 253355887a1ab0ac8f33b356c7c1140eee554d18
change-id: 20250714-uvc-racemeta-fee2e69bbfcd

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

[PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Ricardo Ribalda]

queue->irqueue contains a list of the buffers owned by the driver. The
list is protected by queue->irqlock. uvc_queue_get_current_buffer()
returns a pointer to the current buffer in that list, but does not
remove the buffer from it. This can lead to race conditions.

Inspecting the code, it seems that the candidate for such race is
uvc_queue_return_buffers(). For the capture queue, that function is
called with the device streamoff, so no race can occur. On the other
hand, the metadata queue, could trigger a race condition, because
stop_streaming can be called with the device in any streaming state.

We can solve this issue introducing a flag, stream->meta.in_flight,
protected with a spinlock. When there is a buffer in flight that can
write into metadata the flag is raised, notifying the stop streaming
that it needs to wait.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
Cc: stable@vger.kernel.org
Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
 drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
 drivers/media/usb/uvc/uvcvideo.h  |  2 ++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 3c002c8f442f..af9dbfcf6f53 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
 static void uvc_stop_streaming_meta(struct vb2_queue *vq)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+	struct uvc_streaming *stream = queue->stream;
 
 	lockdep_assert_irqs_enabled();
 
+	spin_lock_irq(&stream->meta.irqlock);
+	while (stream->meta.in_flight) {
+		spin_unlock_irq(&stream->meta.irqlock);
+		schedule();
+		spin_lock_irq(&stream->meta.irqlock);
+	}
+	stream->meta.in_flight = true;
+	spin_unlock_irq(&stream->meta.irqlock);
+
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+
+	scoped_guard(spinlock_irq, &stream->meta.irqlock) {
+		stream->meta.in_flight = false;
+	}
 }
 
 static const struct vb2_ops uvc_queue_qops = {
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index fc3536a4399f..f6b55b3a3308 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
 	urb->transfer_buffer_length = stream->urb_size - len;
 }
 
+static struct uvc_buffer *
+uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
+{
+	struct uvc_video_queue *queue = &stream->meta.queue;
+	struct uvc_buffer *buf;
+
+	buf = uvc_queue_get_current_buffer(queue);
+	if (!buf)
+		return NULL;
+
+	guard(spinlock_irqsave)(&stream->meta.irqlock);
+
+	if (stream->meta.in_flight)
+		return NULL;
+
+	stream->meta.in_flight = true;
+
+	return buf;
+}
+
 static void uvc_video_complete(struct urb *urb)
 {
 	struct uvc_urb *uvc_urb = urb->context;
@@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
 	buf = uvc_queue_get_current_buffer(queue);
 
 	if (vb2_qmeta)
-		buf_meta = uvc_queue_get_current_buffer(qmeta);
+		buf_meta = uvc_video_get_current_meta_buffer(stream);
 
 	/* Re-initialise the URB async work. */
 	uvc_urb->async_operations = 0;
@@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
 	 */
 	stream->decode(uvc_urb, buf, buf_meta);
 
+	if (buf_meta) {
+		scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
+			stream->meta.in_flight = false;
+		}
+	}
+
 	/* If no async work is needed, resubmit the URB immediately. */
 	if (!uvc_urb->async_operations) {
 		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
@@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
 	for_each_uvc_urb(uvc_urb, stream)
 		INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
 
+	spin_lock_init(&stream->meta.irqlock);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b6bcee4a222f..6f1a3381d392 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -484,6 +484,8 @@ struct uvc_streaming {
 		struct uvc_video_queue queue;
 		u32 format;
 		u32 buffersize;
+		bool in_flight;
+		spinlock_t irqlock; /* Protects in_flight. */
 	} meta;
 
 	/* Context data used by the bulk completion handler. */

-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Hans de Goede]

Hi Ricardo,

On 29-Jun-26 19:31, Ricardo Ribalda wrote:
> queue->irqueue contains a list of the buffers owned by the driver. The
> list is protected by queue->irqlock. uvc_queue_get_current_buffer()
> returns a pointer to the current buffer in that list, but does not
> remove the buffer from it. This can lead to race conditions.
> 
> Inspecting the code, it seems that the candidate for such race is
> uvc_queue_return_buffers(). For the capture queue, that function is
> called with the device streamoff, so no race can occur. On the other
> hand, the metadata queue, could trigger a race condition, because
> stop_streaming can be called with the device in any streaming state.
> 
> We can solve this issue introducing a flag, stream->meta.in_flight,
> protected with a spinlock. When there is a buffer in flight that can
> write into metadata the flag is raised, notifying the stop streaming
> that it needs to wait.
> 
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
> Cc: stable@vger.kernel.org
> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

First of all thank you for looking into fixing this.

I'm sorry, but this feels more like a band-aid then a proper fix.

How about adding a started bool to struct uvc_streaming which gets
set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().

And then call uvc_video_stop_streaming() from either 
uvc_stop_streaming_video() or uvc_stop_streaming_meta()
depending on which one gets called first ?

With a mutex protecting the started bool and being held
over calling uvc_video_stop_streaming() ?

So stop the actual hw streaming when either of the
2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
callback called?

And to this before draining the buffer queue.

That seems cleaner then this approach?

Regards,

Hans

p.s.

1. It is tempting to also apply the same approach to
vb2_ops.start_streaming, but allowing the meta queue to be
the one to start streaming will likely cause issues. E.g.
the streaming code assumes having a meta-queue active is
optional, but not the other way around.

TL;DR: vb2_ops.start_streaming should stay as is.

2. While looking into this I noticed that struct uvc_streaming
already has an active member, but unless I'm missing something
that ever only gets initialized to 0. So I think that can be
dropped. (If you re-use this please change it to a bool, no
need to have it atomic while protected by a mutex).



> ---
>  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
>  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
>  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
>  3 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 3c002c8f442f..af9dbfcf6f53 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
>  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> +	struct uvc_streaming *stream = queue->stream;
>  
>  	lockdep_assert_irqs_enabled();
>  
> +	spin_lock_irq(&stream->meta.irqlock);
> +	while (stream->meta.in_flight) {
> +		spin_unlock_irq(&stream->meta.irqlock);
> +		schedule();
> +		spin_lock_irq(&stream->meta.irqlock);
> +	}
> +	stream->meta.in_flight = true;
> +	spin_unlock_irq(&stream->meta.irqlock);
> +
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> +
> +	scoped_guard(spinlock_irq, &stream->meta.irqlock) {
> +		stream->meta.in_flight = false;
> +	}
>  }
>  
>  static const struct vb2_ops uvc_queue_qops = {
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index fc3536a4399f..f6b55b3a3308 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
>  	urb->transfer_buffer_length = stream->urb_size - len;
>  }
>  
> +static struct uvc_buffer *
> +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
> +{
> +	struct uvc_video_queue *queue = &stream->meta.queue;
> +	struct uvc_buffer *buf;
> +
> +	buf = uvc_queue_get_current_buffer(queue);
> +	if (!buf)
> +		return NULL;
> +
> +	guard(spinlock_irqsave)(&stream->meta.irqlock);
> +
> +	if (stream->meta.in_flight)
> +		return NULL;
> +
> +	stream->meta.in_flight = true;
> +
> +	return buf;
> +}
> +
>  static void uvc_video_complete(struct urb *urb)
>  {
>  	struct uvc_urb *uvc_urb = urb->context;
> @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
>  	buf = uvc_queue_get_current_buffer(queue);
>  
>  	if (vb2_qmeta)
> -		buf_meta = uvc_queue_get_current_buffer(qmeta);
> +		buf_meta = uvc_video_get_current_meta_buffer(stream);
>  
>  	/* Re-initialise the URB async work. */
>  	uvc_urb->async_operations = 0;
> @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
>  	 */
>  	stream->decode(uvc_urb, buf, buf_meta);
>  
> +	if (buf_meta) {
> +		scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
> +			stream->meta.in_flight = false;
> +		}
> +	}
> +
>  	/* If no async work is needed, resubmit the URB immediately. */
>  	if (!uvc_urb->async_operations) {
>  		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	for_each_uvc_urb(uvc_urb, stream)
>  		INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
>  
> +	spin_lock_init(&stream->meta.irqlock);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b6bcee4a222f..6f1a3381d392 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -484,6 +484,8 @@ struct uvc_streaming {
>  		struct uvc_video_queue queue;
>  		u32 format;
>  		u32 buffersize;
> +		bool in_flight;
> +		spinlock_t irqlock; /* Protects in_flight. */
>  	} meta;
>  
>  	/* Context data used by the bulk completion handler. */
> 

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Ricardo Ribalda]

Hi Hans,

Thanks for the prompt reply.

On Tue, 30 Jun 2026 at 11:47, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 29-Jun-26 19:31, Ricardo Ribalda wrote:
> > queue->irqueue contains a list of the buffers owned by the driver. The
> > list is protected by queue->irqlock. uvc_queue_get_current_buffer()
> > returns a pointer to the current buffer in that list, but does not
> > remove the buffer from it. This can lead to race conditions.
> >
> > Inspecting the code, it seems that the candidate for such race is
> > uvc_queue_return_buffers(). For the capture queue, that function is
> > called with the device streamoff, so no race can occur. On the other
> > hand, the metadata queue, could trigger a race condition, because
> > stop_streaming can be called with the device in any streaming state.
> >
> > We can solve this issue introducing a flag, stream->meta.in_flight,
> > protected with a spinlock. When there is a buffer in flight that can
> > write into metadata the flag is raised, notifying the stop streaming
> > that it needs to wait.
> >
> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
> > Cc: stable@vger.kernel.org
> > Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> First of all thank you for looking into fixing this.
>
> I'm sorry, but this feels more like a band-aid then a proper fix.
>
> How about adding a started bool to struct uvc_streaming which gets
> set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().
>
> And then call uvc_video_stop_streaming() from either
> uvc_stop_streaming_video() or uvc_stop_streaming_meta()
> depending on which one gets called first ?
>
> With a mutex protecting the started bool and being held
> over calling uvc_video_stop_streaming() ?
>
> So stop the actual hw streaming when either of the
> 2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
> callback called?
>
> And to this before draining the buffer queue.
>
> That seems cleaner then this approach?

Assuming /dev/video0 is the video node and /dev/video1 is the meta device.

Currently, we support something like:

1) yavta -c /dev/video0 &
2) yavta --capture=2 /dev/video1
3) yavta --capture=2 /dev/video1
4) kill %1


If I understood correctly, your proposal would cause the camera to
stop streaming when step 2 completes.

I think this risks breaking use cases.

As I see it, the issue is that the camera's live capture cycle is
controlled solely by video0. We need some kind of synchronization
mechanism with video1 if we do not want to change the behaviour and
risk breaking apps.

>
> Regards,
>
> Hans
>
> p.s.
>
> 1. It is tempting to also apply the same approach to
> vb2_ops.start_streaming, but allowing the meta queue to be
> the one to start streaming will likely cause issues. E.g.
> the streaming code assumes having a meta-queue active is
> optional, but not the other way around.
>
> TL;DR: vb2_ops.start_streaming should stay as is.
>
> 2. While looking into this I noticed that struct uvc_streaming
> already has an active member, but unless I'm missing something
> that ever only gets initialized to 0. So I think that can be
> dropped. (If you re-use this please change it to a bool, no
> need to have it atomic while protected by a mutex).

I will send a patch to fix this. Thanks for noticing :)

>
>
>
> > ---
> >  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
> >  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
> >  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
> >  3 files changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > index 3c002c8f442f..af9dbfcf6f53 100644
> > --- a/drivers/media/usb/uvc/uvc_queue.c
> > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
> >  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> >  {
> >       struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > +     struct uvc_streaming *stream = queue->stream;
> >
> >       lockdep_assert_irqs_enabled();
> >
> > +     spin_lock_irq(&stream->meta.irqlock);
> > +     while (stream->meta.in_flight) {
> > +             spin_unlock_irq(&stream->meta.irqlock);
> > +             schedule();
> > +             spin_lock_irq(&stream->meta.irqlock);
> > +     }
> > +     stream->meta.in_flight = true;
> > +     spin_unlock_irq(&stream->meta.irqlock);
> > +
> >       uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > +
> > +     scoped_guard(spinlock_irq, &stream->meta.irqlock) {
> > +             stream->meta.in_flight = false;
> > +     }
> >  }
> >
> >  static const struct vb2_ops uvc_queue_qops = {
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index fc3536a4399f..f6b55b3a3308 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
> >       urb->transfer_buffer_length = stream->urb_size - len;
> >  }
> >
> > +static struct uvc_buffer *
> > +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
> > +{
> > +     struct uvc_video_queue *queue = &stream->meta.queue;
> > +     struct uvc_buffer *buf;
> > +
> > +     buf = uvc_queue_get_current_buffer(queue);
> > +     if (!buf)
> > +             return NULL;
> > +
> > +     guard(spinlock_irqsave)(&stream->meta.irqlock);
> > +
> > +     if (stream->meta.in_flight)
> > +             return NULL;
> > +
> > +     stream->meta.in_flight = true;
> > +
> > +     return buf;
> > +}
> > +
> >  static void uvc_video_complete(struct urb *urb)
> >  {
> >       struct uvc_urb *uvc_urb = urb->context;
> > @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
> >       buf = uvc_queue_get_current_buffer(queue);
> >
> >       if (vb2_qmeta)
> > -             buf_meta = uvc_queue_get_current_buffer(qmeta);
> > +             buf_meta = uvc_video_get_current_meta_buffer(stream);
> >
> >       /* Re-initialise the URB async work. */
> >       uvc_urb->async_operations = 0;
> > @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
> >        */
> >       stream->decode(uvc_urb, buf, buf_meta);
> >
> > +     if (buf_meta) {
> > +             scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
> > +                     stream->meta.in_flight = false;
> > +             }
> > +     }
> > +
> >       /* If no async work is needed, resubmit the URB immediately. */
> >       if (!uvc_urb->async_operations) {
> >               ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> > @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> >       for_each_uvc_urb(uvc_urb, stream)
> >               INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> >
> > +     spin_lock_init(&stream->meta.irqlock);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index b6bcee4a222f..6f1a3381d392 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -484,6 +484,8 @@ struct uvc_streaming {
> >               struct uvc_video_queue queue;
> >               u32 format;
> >               u32 buffersize;
> > +             bool in_flight;
> > +             spinlock_t irqlock; /* Protects in_flight. */
> >       } meta;
> >
> >       /* Context data used by the bulk completion handler. */
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Hans de Goede]

Hi Ricardo,

On 30-Jun-26 12:17, Ricardo Ribalda wrote:
> Hi Hans,
> 
> Thanks for the prompt reply.
> 
> On Tue, 30 Jun 2026 at 11:47, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 29-Jun-26 19:31, Ricardo Ribalda wrote:
>>> queue->irqueue contains a list of the buffers owned by the driver. The
>>> list is protected by queue->irqlock. uvc_queue_get_current_buffer()
>>> returns a pointer to the current buffer in that list, but does not
>>> remove the buffer from it. This can lead to race conditions.
>>>
>>> Inspecting the code, it seems that the candidate for such race is
>>> uvc_queue_return_buffers(). For the capture queue, that function is
>>> called with the device streamoff, so no race can occur. On the other
>>> hand, the metadata queue, could trigger a race condition, because
>>> stop_streaming can be called with the device in any streaming state.
>>>
>>> We can solve this issue introducing a flag, stream->meta.in_flight,
>>> protected with a spinlock. When there is a buffer in flight that can
>>> write into metadata the flag is raised, notifying the stop streaming
>>> that it needs to wait.
>>>
>>> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
>>> Cc: stable@vger.kernel.org
>>> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> First of all thank you for looking into fixing this.
>>
>> I'm sorry, but this feels more like a band-aid then a proper fix.
>>
>> How about adding a started bool to struct uvc_streaming which gets
>> set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().
>>
>> And then call uvc_video_stop_streaming() from either
>> uvc_stop_streaming_video() or uvc_stop_streaming_meta()
>> depending on which one gets called first ?
>>
>> With a mutex protecting the started bool and being held
>> over calling uvc_video_stop_streaming() ?
>>
>> So stop the actual hw streaming when either of the
>> 2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
>> callback called?
>>
>> And to this before draining the buffer queue.
>>
>> That seems cleaner then this approach?
> 
> Assuming /dev/video0 is the video node and /dev/video1 is the meta device.
> 
> Currently, we support something like:
> 
> 1) yavta -c /dev/video0 &
> 2) yavta --capture=2 /dev/video1
> 3) yavta --capture=2 /dev/video1
> 4) kill %1
> 
> 
> If I understood correctly, your proposal would cause the camera to
> stop streaming when step 2 completes.

Yes. But this very much feels like a case of:

https://quotefancy.com/media/wallpaper/1600x900/5523002-Henny-Youngman-Quote-The-patient-says-Doctor-it-hurts-when-I-do.jpg

> I think this risks breaking use cases.

That would have to be some rather convoluted use-case.

IMHO the simplicity of fixing the race you're trying to fix is
worth the userspace regression risk (which I deem low).

Worst case we revert the fix and go back to the drawing board.

> As I see it, the issue is that the camera's live capture cycle is
> controlled solely by video0. We need some kind of synchronization
> mechanism with video1 if we do not want to change the behaviour and
> risk breaking apps.

IMHO for a device with multiple /dev/video# nodes it makes sense
to wait with actually starting streaming/DMA-engines until all
enabled queues are started and stop when the first queue is stopped.

The problem with uvcvideo is that we do not know if the metadata
queue is going to get used at all. In hindsight we should maybe
have had some way for userspace to explictly enable/disable metadata
support.

So we start as soon as the main video node is opened, still I think
that stopping as soon as one of the queues is stopped makes sense.

Laurent, do you have any input here?

Regards,

Hans




>> p.s.
>>
>> 1. It is tempting to also apply the same approach to
>> vb2_ops.start_streaming, but allowing the meta queue to be
>> the one to start streaming will likely cause issues. E.g.
>> the streaming code assumes having a meta-queue active is
>> optional, but not the other way around.
>>
>> TL;DR: vb2_ops.start_streaming should stay as is.
>>
>> 2. While looking into this I noticed that struct uvc_streaming
>> already has an active member, but unless I'm missing something
>> that ever only gets initialized to 0. So I think that can be
>> dropped. (If you re-use this please change it to a bool, no
>> need to have it atomic while protected by a mutex).
> 
> I will send a patch to fix this. Thanks for noticing :)
> 
>>
>>
>>
>>> ---
>>>  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
>>>  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
>>>  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
>>>  3 files changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>>> index 3c002c8f442f..af9dbfcf6f53 100644
>>> --- a/drivers/media/usb/uvc/uvc_queue.c
>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>>> @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
>>>  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>>>  {
>>>       struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>> +     struct uvc_streaming *stream = queue->stream;
>>>
>>>       lockdep_assert_irqs_enabled();
>>>
>>> +     spin_lock_irq(&stream->meta.irqlock);
>>> +     while (stream->meta.in_flight) {
>>> +             spin_unlock_irq(&stream->meta.irqlock);
>>> +             schedule();
>>> +             spin_lock_irq(&stream->meta.irqlock);
>>> +     }
>>> +     stream->meta.in_flight = true;
>>> +     spin_unlock_irq(&stream->meta.irqlock);
>>> +
>>>       uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>> +
>>> +     scoped_guard(spinlock_irq, &stream->meta.irqlock) {
>>> +             stream->meta.in_flight = false;
>>> +     }
>>>  }
>>>
>>>  static const struct vb2_ops uvc_queue_qops = {
>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>>> index fc3536a4399f..f6b55b3a3308 100644
>>> --- a/drivers/media/usb/uvc/uvc_video.c
>>> +++ b/drivers/media/usb/uvc/uvc_video.c
>>> @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
>>>       urb->transfer_buffer_length = stream->urb_size - len;
>>>  }
>>>
>>> +static struct uvc_buffer *
>>> +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
>>> +{
>>> +     struct uvc_video_queue *queue = &stream->meta.queue;
>>> +     struct uvc_buffer *buf;
>>> +
>>> +     buf = uvc_queue_get_current_buffer(queue);
>>> +     if (!buf)
>>> +             return NULL;
>>> +
>>> +     guard(spinlock_irqsave)(&stream->meta.irqlock);
>>> +
>>> +     if (stream->meta.in_flight)
>>> +             return NULL;
>>> +
>>> +     stream->meta.in_flight = true;
>>> +
>>> +     return buf;
>>> +}
>>> +
>>>  static void uvc_video_complete(struct urb *urb)
>>>  {
>>>       struct uvc_urb *uvc_urb = urb->context;
>>> @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
>>>       buf = uvc_queue_get_current_buffer(queue);
>>>
>>>       if (vb2_qmeta)
>>> -             buf_meta = uvc_queue_get_current_buffer(qmeta);
>>> +             buf_meta = uvc_video_get_current_meta_buffer(stream);
>>>
>>>       /* Re-initialise the URB async work. */
>>>       uvc_urb->async_operations = 0;
>>> @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
>>>        */
>>>       stream->decode(uvc_urb, buf, buf_meta);
>>>
>>> +     if (buf_meta) {
>>> +             scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
>>> +                     stream->meta.in_flight = false;
>>> +             }
>>> +     }
>>> +
>>>       /* If no async work is needed, resubmit the URB immediately. */
>>>       if (!uvc_urb->async_operations) {
>>>               ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
>>> @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>>>       for_each_uvc_urb(uvc_urb, stream)
>>>               INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
>>>
>>> +     spin_lock_init(&stream->meta.irqlock);
>>> +
>>>       return 0;
>>>  }
>>>
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index b6bcee4a222f..6f1a3381d392 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -484,6 +484,8 @@ struct uvc_streaming {
>>>               struct uvc_video_queue queue;
>>>               u32 format;
>>>               u32 buffersize;
>>> +             bool in_flight;
>>> +             spinlock_t irqlock; /* Protects in_flight. */
>>>       } meta;
>>>
>>>       /* Context data used by the bulk completion handler. */
>>>
>>
> 
> 

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Ricardo Ribalda]

Hi Hans,

On Tue, 30 Jun 2026 at 15:21, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 30-Jun-26 12:17, Ricardo Ribalda wrote:
> > Hi Hans,
> >
> > Thanks for the prompt reply.
> >
> > On Tue, 30 Jun 2026 at 11:47, Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 29-Jun-26 19:31, Ricardo Ribalda wrote:
> >>> queue->irqueue contains a list of the buffers owned by the driver. The
> >>> list is protected by queue->irqlock. uvc_queue_get_current_buffer()
> >>> returns a pointer to the current buffer in that list, but does not
> >>> remove the buffer from it. This can lead to race conditions.
> >>>
> >>> Inspecting the code, it seems that the candidate for such race is
> >>> uvc_queue_return_buffers(). For the capture queue, that function is
> >>> called with the device streamoff, so no race can occur. On the other
> >>> hand, the metadata queue, could trigger a race condition, because
> >>> stop_streaming can be called with the device in any streaming state.
> >>>
> >>> We can solve this issue introducing a flag, stream->meta.in_flight,
> >>> protected with a spinlock. When there is a buffer in flight that can
> >>> write into metadata the flag is raised, notifying the stop streaming
> >>> that it needs to wait.
> >>>
> >>> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>
> >> First of all thank you for looking into fixing this.
> >>
> >> I'm sorry, but this feels more like a band-aid then a proper fix.
> >>
> >> How about adding a started bool to struct uvc_streaming which gets
> >> set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().
> >>
> >> And then call uvc_video_stop_streaming() from either
> >> uvc_stop_streaming_video() or uvc_stop_streaming_meta()
> >> depending on which one gets called first ?
> >>
> >> With a mutex protecting the started bool and being held
> >> over calling uvc_video_stop_streaming() ?
> >>
> >> So stop the actual hw streaming when either of the
> >> 2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
> >> callback called?
> >>
> >> And to this before draining the buffer queue.
> >>
> >> That seems cleaner then this approach?
> >
> > Assuming /dev/video0 is the video node and /dev/video1 is the meta device.
> >
> > Currently, we support something like:
> >
> > 1) yavta -c /dev/video0 &
> > 2) yavta --capture=2 /dev/video1
> > 3) yavta --capture=2 /dev/video1
> > 4) kill %1
> >
> >
> > If I understood correctly, your proposal would cause the camera to
> > stop streaming when step 2 completes.
>
> Yes. But this very much feels like a case of:
>
> https://quotefancy.com/media/wallpaper/1600x900/5523002-Henny-Youngman-Quote-The-patient-says-Doctor-it-hurts-when-I-do.jpg

We have a similar joke in Spanish:
Doctor, doctor, it hurts here, here, here, here, here. What do I have?
A broken finger :P

>
> > I think this risks breaking use cases.
>
> That would have to be some rather convoluted use-case.

I believe we have a similar scheme to test the metadata node in
ChromeOS... but we can change that.
My worry is the outside apps that we do not control.

>
> IMHO the simplicity of fixing the race you're trying to fix is
> worth the userspace regression risk (which I deem low).
>
> Worst case we revert the fix and go back to the drawing board.

Are you concerned of this asymmetric behaviour, or do you think that it is fine?

open /dev/video0 (streaming starts)
open /dev/video1
close /dev/video1 (streaming stops)
open /dev/video1 (streaming still off)


vs

open /dev/video0 (streaming starts)
open /dev/video1
close /dev/video0 (streaming stops)
open /dev/video0 (streaming resumes)


>
> > As I see it, the issue is that the camera's live capture cycle is
> > controlled solely by video0. We need some kind of synchronization
> > mechanism with video1 if we do not want to change the behaviour and
> > risk breaking apps.
>
> IMHO for a device with multiple /dev/video# nodes it makes sense
> to wait with actually starting streaming/DMA-engines until all
> enabled queues are started and stop when the first queue is stopped.
>
> The problem with uvcvideo is that we do not know if the metadata
> queue is going to get used at all. In hindsight we should maybe
> have had some way for userspace to explictly enable/disable metadata
> support.
>
> So we start as soon as the main video node is opened, still I think
> that stopping as soon as one of the queues is stopped makes sense.
>
> Laurent, do you have any input here?
>
> Regards,
>
> Hans
>
>
>
>
> >> p.s.
> >>
> >> 1. It is tempting to also apply the same approach to
> >> vb2_ops.start_streaming, but allowing the meta queue to be
> >> the one to start streaming will likely cause issues. E.g.
> >> the streaming code assumes having a meta-queue active is
> >> optional, but not the other way around.
> >>
> >> TL;DR: vb2_ops.start_streaming should stay as is.
> >>
> >> 2. While looking into this I noticed that struct uvc_streaming
> >> already has an active member, but unless I'm missing something
> >> that ever only gets initialized to 0. So I think that can be
> >> dropped. (If you re-use this please change it to a bool, no
> >> need to have it atomic while protected by a mutex).
> >
> > I will send a patch to fix this. Thanks for noticing :)
> >
> >>
> >>
> >>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
> >>>  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
> >>>  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
> >>>  3 files changed, 45 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >>> index 3c002c8f442f..af9dbfcf6f53 100644
> >>> --- a/drivers/media/usb/uvc/uvc_queue.c
> >>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >>> @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
> >>>  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> >>>  {
> >>>       struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>> +     struct uvc_streaming *stream = queue->stream;
> >>>
> >>>       lockdep_assert_irqs_enabled();
> >>>
> >>> +     spin_lock_irq(&stream->meta.irqlock);
> >>> +     while (stream->meta.in_flight) {
> >>> +             spin_unlock_irq(&stream->meta.irqlock);
> >>> +             schedule();
> >>> +             spin_lock_irq(&stream->meta.irqlock);
> >>> +     }
> >>> +     stream->meta.in_flight = true;
> >>> +     spin_unlock_irq(&stream->meta.irqlock);
> >>> +
> >>>       uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >>> +
> >>> +     scoped_guard(spinlock_irq, &stream->meta.irqlock) {
> >>> +             stream->meta.in_flight = false;
> >>> +     }
> >>>  }
> >>>
> >>>  static const struct vb2_ops uvc_queue_qops = {
> >>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> >>> index fc3536a4399f..f6b55b3a3308 100644
> >>> --- a/drivers/media/usb/uvc/uvc_video.c
> >>> +++ b/drivers/media/usb/uvc/uvc_video.c
> >>> @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
> >>>       urb->transfer_buffer_length = stream->urb_size - len;
> >>>  }
> >>>
> >>> +static struct uvc_buffer *
> >>> +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
> >>> +{
> >>> +     struct uvc_video_queue *queue = &stream->meta.queue;
> >>> +     struct uvc_buffer *buf;
> >>> +
> >>> +     buf = uvc_queue_get_current_buffer(queue);
> >>> +     if (!buf)
> >>> +             return NULL;
> >>> +
> >>> +     guard(spinlock_irqsave)(&stream->meta.irqlock);
> >>> +
> >>> +     if (stream->meta.in_flight)
> >>> +             return NULL;
> >>> +
> >>> +     stream->meta.in_flight = true;
> >>> +
> >>> +     return buf;
> >>> +}
> >>> +
> >>>  static void uvc_video_complete(struct urb *urb)
> >>>  {
> >>>       struct uvc_urb *uvc_urb = urb->context;
> >>> @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
> >>>       buf = uvc_queue_get_current_buffer(queue);
> >>>
> >>>       if (vb2_qmeta)
> >>> -             buf_meta = uvc_queue_get_current_buffer(qmeta);
> >>> +             buf_meta = uvc_video_get_current_meta_buffer(stream);
> >>>
> >>>       /* Re-initialise the URB async work. */
> >>>       uvc_urb->async_operations = 0;
> >>> @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
> >>>        */
> >>>       stream->decode(uvc_urb, buf, buf_meta);
> >>>
> >>> +     if (buf_meta) {
> >>> +             scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
> >>> +                     stream->meta.in_flight = false;
> >>> +             }
> >>> +     }
> >>> +
> >>>       /* If no async work is needed, resubmit the URB immediately. */
> >>>       if (!uvc_urb->async_operations) {
> >>>               ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> >>> @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> >>>       for_each_uvc_urb(uvc_urb, stream)
> >>>               INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> >>>
> >>> +     spin_lock_init(&stream->meta.irqlock);
> >>> +
> >>>       return 0;
> >>>  }
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>> index b6bcee4a222f..6f1a3381d392 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -484,6 +484,8 @@ struct uvc_streaming {
> >>>               struct uvc_video_queue queue;
> >>>               u32 format;
> >>>               u32 buffersize;
> >>> +             bool in_flight;
> >>> +             spinlock_t irqlock; /* Protects in_flight. */
> >>>       } meta;
> >>>
> >>>       /* Context data used by the bulk completion handler. */
> >>>
> >>
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Hans de Goede]

Hi Ricardo,

On 30-Jun-26 16:10, Ricardo Ribalda wrote:
> Hi Hans,
> 
> On Tue, 30 Jun 2026 at 15:21, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 30-Jun-26 12:17, Ricardo Ribalda wrote:
>>> Hi Hans,
>>>
>>> Thanks for the prompt reply.
>>>
>>> On Tue, 30 Jun 2026 at 11:47, Hans de Goede <hansg@kernel.org> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On 29-Jun-26 19:31, Ricardo Ribalda wrote:
>>>>> queue->irqueue contains a list of the buffers owned by the driver. The
>>>>> list is protected by queue->irqlock. uvc_queue_get_current_buffer()
>>>>> returns a pointer to the current buffer in that list, but does not
>>>>> remove the buffer from it. This can lead to race conditions.
>>>>>
>>>>> Inspecting the code, it seems that the candidate for such race is
>>>>> uvc_queue_return_buffers(). For the capture queue, that function is
>>>>> called with the device streamoff, so no race can occur. On the other
>>>>> hand, the metadata queue, could trigger a race condition, because
>>>>> stop_streaming can be called with the device in any streaming state.
>>>>>
>>>>> We can solve this issue introducing a flag, stream->meta.in_flight,
>>>>> protected with a spinlock. When there is a buffer in flight that can
>>>>> write into metadata the flag is raised, notifying the stop streaming
>>>>> that it needs to wait.
>>>>>
>>>>> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideasonboard.com/
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>
>>>> First of all thank you for looking into fixing this.
>>>>
>>>> I'm sorry, but this feels more like a band-aid then a proper fix.
>>>>
>>>> How about adding a started bool to struct uvc_streaming which gets
>>>> set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().
>>>>
>>>> And then call uvc_video_stop_streaming() from either
>>>> uvc_stop_streaming_video() or uvc_stop_streaming_meta()
>>>> depending on which one gets called first ?
>>>>
>>>> With a mutex protecting the started bool and being held
>>>> over calling uvc_video_stop_streaming() ?
>>>>
>>>> So stop the actual hw streaming when either of the
>>>> 2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
>>>> callback called?
>>>>
>>>> And to this before draining the buffer queue.
>>>>
>>>> That seems cleaner then this approach?
>>>
>>> Assuming /dev/video0 is the video node and /dev/video1 is the meta device.
>>>
>>> Currently, we support something like:
>>>
>>> 1) yavta -c /dev/video0 &
>>> 2) yavta --capture=2 /dev/video1
>>> 3) yavta --capture=2 /dev/video1
>>> 4) kill %1
>>>
>>>
>>> If I understood correctly, your proposal would cause the camera to
>>> stop streaming when step 2 completes.
>>
>> Yes. But this very much feels like a case of:
>>
>> https://quotefancy.com/media/wallpaper/1600x900/5523002-Henny-Youngman-Quote-The-patient-says-Doctor-it-hurts-when-I-do.jpg
> 
> We have a similar joke in Spanish:
> Doctor, doctor, it hurts here, here, here, here, here. What do I have?
> A broken finger :P
> 
>>
>>> I think this risks breaking use cases.
>>
>> That would have to be some rather convoluted use-case.
> 
> I believe we have a similar scheme to test the metadata node in
> ChromeOS... but we can change that.

That seems unlikely? Either I would expect some app/lib/dameon to
do a quick test stream for a few frames at init time to determine metadata
support, in which case I would expect streaming on both queues to get
stopped after the quick test.

Or testing is delayed till the first real start-streaming moment in which
case it makes no sense to stop + restart the metadata queue. What I guess
may happen is stopping the metadata queue when it does not generate any
data for a few frames, assuming there simply is no metadata support.

Hmm, that might actually be a troublesome case.

> My worry is the outside apps that we do not control.

Ack, thinking more about this, this might be more likely then
I assumed in the non metadata available case, see above.

So I'm no longer really convinced of my own proposal.

>> IMHO the simplicity of fixing the race you're trying to fix is
>> worth the userspace regression risk (which I deem low).
>>
>> Worst case we revert the fix and go back to the drawing board.
> 
> Are you concerned of this asymmetric behaviour, or do you think that it is fine?
> 
> open /dev/video0 (streaming starts)
> open /dev/video1
> close /dev/video1 (streaming stops)
> open /dev/video1 (streaming still off)
> 
> 
> vs
> 
> open /dev/video0 (streaming starts)
> open /dev/video1
> close /dev/video0 (streaming stops)
> open /dev/video0 (streaming resumes)

That second one actually is broken already, we don't flush
the metadata queue on streaming stop on the regular queue, so
it will possibly contain a half-filled metadata buffer which
we then continue to append to with fresh metadata. So any
multi-packet metadata will get corrupted for the first frame
in the second stream start in that case.

In hindsight having the metadata queue be a fully independent
queue without clearly defining how start/stop on both queues
works and enforcing the defined behavior at the driver level
was a mistake.

I'm starting to think that ideally we would simply flush both
queues on the stop on the regular node and not have a stop
queue-op on the metadata queue at all, but that is not possible
I'm afraid.

So I think we do need something like this series +
flush metadata-queue on regular queue stop.

I'll try to make some time to review this series as is, since
although the waiting solution still feels ugly it may be the
best we can do.

Regards,

Hans





> 
> 
>>
>>> As I see it, the issue is that the camera's live capture cycle is
>>> controlled solely by video0. We need some kind of synchronization
>>> mechanism with video1 if we do not want to change the behaviour and
>>> risk breaking apps.
>>
>> IMHO for a device with multiple /dev/video# nodes it makes sense
>> to wait with actually starting streaming/DMA-engines until all
>> enabled queues are started and stop when the first queue is stopped.
>>
>> The problem with uvcvideo is that we do not know if the metadata
>> queue is going to get used at all. In hindsight we should maybe
>> have had some way for userspace to explictly enable/disable metadata
>> support.
>>
>> So we start as soon as the main video node is opened, still I think
>> that stopping as soon as one of the queues is stopped makes sense.
>>
>> Laurent, do you have any input here?
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>> p.s.
>>>>
>>>> 1. It is tempting to also apply the same approach to
>>>> vb2_ops.start_streaming, but allowing the meta queue to be
>>>> the one to start streaming will likely cause issues. E.g.
>>>> the streaming code assumes having a meta-queue active is
>>>> optional, but not the other way around.
>>>>
>>>> TL;DR: vb2_ops.start_streaming should stay as is.
>>>>
>>>> 2. While looking into this I noticed that struct uvc_streaming
>>>> already has an active member, but unless I'm missing something
>>>> that ever only gets initialized to 0. So I think that can be
>>>> dropped. (If you re-use this please change it to a bool, no
>>>> need to have it atomic while protected by a mutex).
>>>
>>> I will send a patch to fix this. Thanks for noticing :)
>>>
>>>>
>>>>
>>>>
>>>>> ---
>>>>>  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
>>>>>  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
>>>>>  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
>>>>>  3 files changed, 45 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>>>>> index 3c002c8f442f..af9dbfcf6f53 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_queue.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>>>>> @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
>>>>>  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>>>>>  {
>>>>>       struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>>>> +     struct uvc_streaming *stream = queue->stream;
>>>>>
>>>>>       lockdep_assert_irqs_enabled();
>>>>>
>>>>> +     spin_lock_irq(&stream->meta.irqlock);
>>>>> +     while (stream->meta.in_flight) {
>>>>> +             spin_unlock_irq(&stream->meta.irqlock);
>>>>> +             schedule();
>>>>> +             spin_lock_irq(&stream->meta.irqlock);
>>>>> +     }
>>>>> +     stream->meta.in_flight = true;
>>>>> +     spin_unlock_irq(&stream->meta.irqlock);
>>>>> +
>>>>>       uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>>>> +
>>>>> +     scoped_guard(spinlock_irq, &stream->meta.irqlock) {
>>>>> +             stream->meta.in_flight = false;
>>>>> +     }
>>>>>  }
>>>>>
>>>>>  static const struct vb2_ops uvc_queue_qops = {
>>>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>>>>> index fc3536a4399f..f6b55b3a3308 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_video.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_video.c
>>>>> @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
>>>>>       urb->transfer_buffer_length = stream->urb_size - len;
>>>>>  }
>>>>>
>>>>> +static struct uvc_buffer *
>>>>> +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
>>>>> +{
>>>>> +     struct uvc_video_queue *queue = &stream->meta.queue;
>>>>> +     struct uvc_buffer *buf;
>>>>> +
>>>>> +     buf = uvc_queue_get_current_buffer(queue);
>>>>> +     if (!buf)
>>>>> +             return NULL;
>>>>> +
>>>>> +     guard(spinlock_irqsave)(&stream->meta.irqlock);
>>>>> +
>>>>> +     if (stream->meta.in_flight)
>>>>> +             return NULL;
>>>>> +
>>>>> +     stream->meta.in_flight = true;
>>>>> +
>>>>> +     return buf;
>>>>> +}
>>>>> +
>>>>>  static void uvc_video_complete(struct urb *urb)
>>>>>  {
>>>>>       struct uvc_urb *uvc_urb = urb->context;
>>>>> @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
>>>>>       buf = uvc_queue_get_current_buffer(queue);
>>>>>
>>>>>       if (vb2_qmeta)
>>>>> -             buf_meta = uvc_queue_get_current_buffer(qmeta);
>>>>> +             buf_meta = uvc_video_get_current_meta_buffer(stream);
>>>>>
>>>>>       /* Re-initialise the URB async work. */
>>>>>       uvc_urb->async_operations = 0;
>>>>> @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
>>>>>        */
>>>>>       stream->decode(uvc_urb, buf, buf_meta);
>>>>>
>>>>> +     if (buf_meta) {
>>>>> +             scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
>>>>> +                     stream->meta.in_flight = false;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>>       /* If no async work is needed, resubmit the URB immediately. */
>>>>>       if (!uvc_urb->async_operations) {
>>>>>               ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
>>>>> @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
>>>>>       for_each_uvc_urb(uvc_urb, stream)
>>>>>               INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
>>>>>
>>>>> +     spin_lock_init(&stream->meta.irqlock);
>>>>> +
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>>>> index b6bcee4a222f..6f1a3381d392 100644
>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>>>> @@ -484,6 +484,8 @@ struct uvc_streaming {
>>>>>               struct uvc_video_queue queue;
>>>>>               u32 format;
>>>>>               u32 buffersize;
>>>>> +             bool in_flight;
>>>>> +             spinlock_t irqlock; /* Protects in_flight. */
>>>>>       } meta;
>>>>>
>>>>>       /* Context data used by the bulk completion handler. */
>>>>>
>>>>
>>>
>>>
>>
> 
> 

[PATCH v2 2/2] media: uvcvideo: Use wait queue for metadata streamoff

[Ricardo Ribalda]

We are now using a schedule()/loop to wait for in-flight buffers. This
works, but it is not beautiful. Instead we can use wait queues.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c | 7 ++-----
 drivers/media/usb/uvc/uvc_video.c | 2 ++
 drivers/media/usb/uvc/uvcvideo.h  | 1 +
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index af9dbfcf6f53..266a75862f0a 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -214,11 +214,8 @@ static void uvc_stop_streaming_meta(struct vb2_queue *vq)
 	lockdep_assert_irqs_enabled();
 
 	spin_lock_irq(&stream->meta.irqlock);
-	while (stream->meta.in_flight) {
-		spin_unlock_irq(&stream->meta.irqlock);
-		schedule();
-		spin_lock_irq(&stream->meta.irqlock);
-	}
+	wait_event_lock_irq(stream->meta.wq, !stream->meta.in_flight,
+			    stream->meta.irqlock);
 	stream->meta.in_flight = true;
 	spin_unlock_irq(&stream->meta.irqlock);
 
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index f6b55b3a3308..735080d40862 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1802,6 +1802,7 @@ static void uvc_video_complete(struct urb *urb)
 		scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
 			stream->meta.in_flight = false;
 		}
+		wake_up(&stream->meta.wq);
 	}
 
 	/* If no async work is needed, resubmit the URB immediately. */
@@ -2357,6 +2358,7 @@ int uvc_video_init(struct uvc_streaming *stream)
 		INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
 
 	spin_lock_init(&stream->meta.irqlock);
+	init_waitqueue_head(&stream->meta.wq);
 
 	return 0;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6f1a3381d392..b28f418c6543 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -486,6 +486,7 @@ struct uvc_streaming {
 		u32 buffersize;
 		bool in_flight;
 		spinlock_t irqlock; /* Protects in_flight. */
+		wait_queue_head_t wq;
 	} meta;
 
 	/* Context data used by the bulk completion handler. */

-- 
2.55.0.rc0.799.gd6f94ed593-goog