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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; }
-void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf, - struct uvc_buffer *meta_buf) +void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect) * Buffers may span multiple packets, and even URBs, therefore the active buffer * remains on the queue until the EOF marker. */ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) { + lockdep_assert_held(&queue->irqlock); + if (list_empty(&queue->irqqueue)) return NULL;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream, * previous header. */ static void uvc_video_decode_meta(struct uvc_streaming *stream, - struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length) { + struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; + struct uvc_video_queue *qmeta = &stream->meta.queue; + struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr; @@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
- if (!meta_buf || length == 2) + if (!vb2_qmeta || length <= 2) + return; + + guard(spinlock_irqsave)(&qmeta->irqlock); + + meta_buf = __uvc_queue_get_current_buffer(qmeta); + if (!meta_buf) return;
has_pts = mem[1] & UVC_STREAM_PTS; @@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream, * Completion handler for video URBs. */
-static void uvc_video_next_buffers(struct uvc_streaming *stream, - struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf) +static void uvc_video_next_meta(struct uvc_streaming *stream, + struct uvc_buffer *video_buf) { - uvc_video_validate_buffer(stream, *video_buf); + struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; + struct uvc_video_queue *qmeta = &stream->meta.queue; + struct uvc_buffer *meta_buf; + struct vb2_v4l2_buffer *vb2_meta; + const struct vb2_v4l2_buffer *vb2_video;
- if (*meta_buf) { - struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf; - const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf; + if (!vb2_qmeta) + return;
- vb2_meta->sequence = vb2_video->sequence; - vb2_meta->field = vb2_video->field; - vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp; + guard(spinlock_irqsave)(&qmeta->irqlock);
- (*meta_buf)->state = UVC_BUF_STATE_READY; - if (!(*meta_buf)->error) - (*meta_buf)->error = (*video_buf)->error; - *meta_buf = uvc_queue_next_buffer(&stream->meta.queue, - *meta_buf); - } - *video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf); + meta_buf = __uvc_queue_get_current_buffer(qmeta); + if (!meta_buf) + return; + list_del(&meta_buf->queue); + + vb2_meta = &meta_buf->buf; + vb2_video = &video_buf->buf; + + vb2_meta->sequence = vb2_video->sequence; + vb2_meta->field = vb2_video->field; + vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp; + meta_buf->state = UVC_BUF_STATE_READY; + if (!meta_buf->error) + meta_buf->error = video_buf->error; + + uvc_queue_buffer_release(meta_buf); +} + +static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream, + struct uvc_buffer *video_buf) +{ + uvc_video_validate_buffer(stream, video_buf); + uvc_video_next_meta(stream, video_buf); + return uvc_queue_next_buffer(&stream->queue, video_buf); }
static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, - struct uvc_buffer *buf, struct uvc_buffer *meta_buf) + struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN) - uvc_video_next_buffers(stream, &buf, &meta_buf); + buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN);
if (ret < 0) continue;
- uvc_video_decode_meta(stream, meta_buf, mem, ret); + uvc_video_decode_meta(stream, mem, ret);
/* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret, @@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY) - uvc_video_next_buffers(stream, &buf, &meta_buf); + buf = uvc_video_next_buffer(stream, buf); } }
static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, - struct uvc_buffer *buf, struct uvc_buffer *meta_buf) + struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN) - uvc_video_next_buffers(stream, &buf, &meta_buf); + buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN);
/* If an error occurred skip the rest of the payload. */ @@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
- uvc_video_decode_meta(stream, meta_buf, mem, ret); + uvc_video_decode_meta(stream, mem, ret);
mem += ret; len -= ret; @@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY) - uvc_video_next_buffers(stream, &buf, &meta_buf); + buf = uvc_video_next_buffer(stream, buf); }
stream->bulk.header_size = 0; @@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, }
static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb, - struct uvc_buffer *buf, struct uvc_buffer *meta_buf) + struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL; - struct uvc_buffer *buf_meta = NULL; - unsigned long flags; int ret;
switch (urb->status) { @@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb)
buf = uvc_queue_get_current_buffer(queue);
- if (vb2_qmeta) { - spin_lock_irqsave(&qmeta->irqlock, flags); - if (!list_empty(&qmeta->irqqueue)) - buf_meta = list_first_entry(&qmeta->irqqueue, - struct uvc_buffer, queue); - spin_unlock_irqrestore(&qmeta->irqlock, flags); - } - /* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;
@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */ - stream->decode(uvc_urb, buf, buf_meta); + stream->decode(uvc_urb, buf);
/* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) { diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq; - void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf, - struct uvc_buffer *meta_buf); + void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf);
struct { struct video_device vdev; @@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
/* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb, - struct uvc_buffer *buf, - struct uvc_buffer *meta_buf); + struct uvc_buffer *buf);
/* debugfs and statistics */ void uvc_debugfs_init(void);
--- base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
Best regards,
Hi Ricardo,
Thank you for the patch.
On Mon, Jul 14, 2025 at 10:23:45AM +0000, 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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; } -void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf)+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
- Buffers may span multiple packets, and even URBs, therefore the active buffer
- remains on the queue until the EOF marker.
*/ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) {
- lockdep_assert_held(&queue->irqlock);
- if (list_empty(&queue->irqqueue)) return NULL;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
- previous header.
*/ static void uvc_video_decode_meta(struct uvc_streaming *stream,
struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length){
- struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;
- struct uvc_video_queue *qmeta = &stream->meta.queue;
- struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr;
@@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
- if (!meta_buf || length == 2)
- if (!vb2_qmeta || length <= 2)
return;- guard(spinlock_irqsave)(&qmeta->irqlock);
This keeps the spinlock held for longer than I would like. We should really try to minimize the amount of work performed with a spinlock held.
- meta_buf = __uvc_queue_get_current_buffer(qmeta);
- if (!meta_buf) return;
has_pts = mem[1] & UVC_STREAM_PTS; @@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
- Completion handler for video URBs.
*/ -static void uvc_video_next_buffers(struct uvc_streaming *stream,
struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf)+static void uvc_video_next_meta(struct uvc_streaming *stream,
struct uvc_buffer *video_buf){
- uvc_video_validate_buffer(stream, *video_buf);
- struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;
- struct uvc_video_queue *qmeta = &stream->meta.queue;
- struct uvc_buffer *meta_buf;
- struct vb2_v4l2_buffer *vb2_meta;
- const struct vb2_v4l2_buffer *vb2_video;
- if (*meta_buf) {
struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf;const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf;
- if (!vb2_qmeta)
return;
vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
- guard(spinlock_irqsave)(&qmeta->irqlock);
(*meta_buf)->state = UVC_BUF_STATE_READY;if (!(*meta_buf)->error)(*meta_buf)->error = (*video_buf)->error;*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,*meta_buf);- }
- *video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
- meta_buf = __uvc_queue_get_current_buffer(qmeta);
- if (!meta_buf)
return;- list_del(&meta_buf->queue);
- vb2_meta = &meta_buf->buf;
- vb2_video = &video_buf->buf;
- vb2_meta->sequence = vb2_video->sequence;
- vb2_meta->field = vb2_video->field;
- vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
- meta_buf->state = UVC_BUF_STATE_READY;
- if (!meta_buf->error)
meta_buf->error = video_buf->error;- uvc_queue_buffer_release(meta_buf);
+}
+static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream,
struct uvc_buffer *video_buf)+{
- uvc_video_validate_buffer(stream, video_buf);
- uvc_video_next_meta(stream, video_buf);
- return uvc_queue_next_buffer(&stream->queue, video_buf);
} static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf);if (ret < 0) continue;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret);/* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret, @@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf);} static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf);/* If an error occurred skip the rest of the payload. */ @@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret);mem += ret; len -= ret; @@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf);stream->bulk.header_size = 0; @@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, } static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
- struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL;
- struct uvc_buffer *buf_meta = NULL;
- unsigned long flags; int ret;
switch (urb->status) { @@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb) buf = uvc_queue_get_current_buffer(queue);
- if (vb2_qmeta) {
spin_lock_irqsave(&qmeta->irqlock, flags);if (!list_empty(&qmeta->irqqueue))buf_meta = list_first_entry(&qmeta->irqqueue,struct uvc_buffer, queue);spin_unlock_irqrestore(&qmeta->irqlock, flags);- }
- /* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;
@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */
- stream->decode(uvc_urb, buf, buf_meta);
- stream->decode(uvc_urb, buf);
/* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) { diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq;
- void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf);
- void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf);
struct { struct video_device vdev; @@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep); /* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
struct uvc_buffer *buf);/* debugfs and statistics */ void uvc_debugfs_init(void);
base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
Hi Laurent
On Mon, 8 Sept 2025 at 12:25, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Mon, Jul 14, 2025 at 10:23:45AM +0000, 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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; }
-void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf)+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
- Buffers may span multiple packets, and even URBs, therefore the active buffer
- remains on the queue until the EOF marker.
*/ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) {
lockdep_assert_held(&queue->irqlock);if (list_empty(&queue->irqqueue)) return NULL;diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
- previous header.
*/ static void uvc_video_decode_meta(struct uvc_streaming *stream,
struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length){
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr;@@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
if (!meta_buf || length == 2)
if (!vb2_qmeta || length <= 2)return;guard(spinlock_irqsave)(&qmeta->irqlock);This keeps the spinlock held for longer than I would like. We should really try to minimize the amount of work performed with a spinlock held.
We are using meta_buf the whole function, which can disappear if the user closes the metadata file descriptor.
Besides memcopying meta_buf, how would you suggest reducing the spinlock held time?
Regards!
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf) return; has_pts = mem[1] & UVC_STREAM_PTS;@@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
- Completion handler for video URBs.
*/
-static void uvc_video_next_buffers(struct uvc_streaming *stream,
struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf)+static void uvc_video_next_meta(struct uvc_streaming *stream,
struct uvc_buffer *video_buf){
uvc_video_validate_buffer(stream, *video_buf);
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf;struct vb2_v4l2_buffer *vb2_meta;const struct vb2_v4l2_buffer *vb2_video;
if (*meta_buf) {struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf;const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf;
if (!vb2_qmeta)return;
vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
guard(spinlock_irqsave)(&qmeta->irqlock);
(*meta_buf)->state = UVC_BUF_STATE_READY;if (!(*meta_buf)->error)(*meta_buf)->error = (*video_buf)->error;*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,*meta_buf);}*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf)return;list_del(&meta_buf->queue);vb2_meta = &meta_buf->buf;vb2_video = &video_buf->buf;vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;meta_buf->state = UVC_BUF_STATE_READY;if (!meta_buf->error)meta_buf->error = video_buf->error;uvc_queue_buffer_release(meta_buf);+}
+static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream,
struct uvc_buffer *video_buf)+{
uvc_video_validate_buffer(stream, video_buf);uvc_video_next_meta(stream, video_buf);return uvc_queue_next_buffer(&stream->queue, video_buf);}
static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); if (ret < 0) continue;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); /* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret,@@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); }}
static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); /* If an error occurred skip the rest of the payload. */@@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); mem += ret; len -= ret;@@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } stream->bulk.header_size = 0;@@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, }
static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL;
struct uvc_buffer *buf_meta = NULL;unsigned long flags; int ret; switch (urb->status) {@@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb)
buf = uvc_queue_get_current_buffer(queue);
if (vb2_qmeta) {spin_lock_irqsave(&qmeta->irqlock, flags);if (!list_empty(&qmeta->irqqueue))buf_meta = list_first_entry(&qmeta->irqqueue,struct uvc_buffer, queue);spin_unlock_irqrestore(&qmeta->irqlock, flags);}/* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */
stream->decode(uvc_urb, buf, buf_meta);
stream->decode(uvc_urb, buf); /* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) {diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq;
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf); struct { struct video_device vdev;@@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
/* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
struct uvc_buffer *buf);/* debugfs and statistics */ void uvc_debugfs_init(void);
base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
-- Regards,
Laurent Pinchart
On Mon, 8 Sept 2025 at 12:57, Ricardo Ribalda ribalda@chromium.org wrote:
Hi Laurent
On Mon, 8 Sept 2025 at 12:25, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Mon, Jul 14, 2025 at 10:23:45AM +0000, 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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; }
-void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf)+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
- Buffers may span multiple packets, and even URBs, therefore the active buffer
- remains on the queue until the EOF marker.
*/ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) {
lockdep_assert_held(&queue->irqlock);if (list_empty(&queue->irqqueue)) return NULL;diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
- previous header.
*/ static void uvc_video_decode_meta(struct uvc_streaming *stream,
struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length){
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr;@@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
if (!meta_buf || length == 2)
if (!vb2_qmeta || length <= 2)return;guard(spinlock_irqsave)(&qmeta->irqlock);This keeps the spinlock held for longer than I would like. We should really try to minimize the amount of work performed with a spinlock held.
We are using meta_buf the whole function, which can disappear if the user closes the metadata file descriptor.
Besides memcopying meta_buf, how would you suggest reducing the spinlock held time?
Actually, it can be slightly improved....
But I am not sure if holding and releasing the spinlock twice will be any better....
Let me know if you prefer the previous version or this one.
Regards
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 178f1e40c189..fa96789e6089 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1430,11 +1430,12 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, if (!vb2_qmeta || length <= 2) return;
- guard(spinlock_irqsave)(&qmeta->irqlock); - - meta_buf = __uvc_queue_get_current_buffer(qmeta); - if (!meta_buf) + spin_lock_irq(&qmeta->irqlock); + if (list_empty(qmeta->irqqueue)) { + spin_unlock_irq(&qmeta->irqlock); return; + } + spin_unlock_irq(&qmeta->irqlock);
has_pts = mem[1] & UVC_STREAM_PTS; has_scr = mem[1] & UVC_STREAM_SCR; @@ -1456,27 +1457,38 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, !memcmp(scr, stream->clock.last_scr, 6))) return;
+ if (has_scr) + memcpy(stream->clock.last_scr, scr, 6); + + local_irq_save(flags); + time = uvc_video_get_time(); + sof = usb_get_current_frame_number(stream->dev->udev); + local_irq_restore(flags); + + spin_lock_irq(&qmeta->irqlock); + + meta_buf = __uvc_queue_get_current_buffer(qmeta); + if (!meta_buf) { + spin_unlock_irq(&qmeta->irqlock); + return; + } + if (meta_buf->length - meta_buf->bytesused < length + sizeof(meta->ns) + sizeof(meta->sof)) { meta_buf->error = 1; + spin_unlock_irq(&qmeta->irqlock); return; }
meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf->bytesused); - local_irq_save(flags); - time = uvc_video_get_time(); - sof = usb_get_current_frame_number(stream->dev->udev); - local_irq_restore(flags); put_unaligned(ktime_to_ns(time), &meta->ns); put_unaligned(sof, &meta->sof);
- if (has_scr) - memcpy(stream->clock.last_scr, scr, 6); - meta->length = mem[0]; meta->flags = mem[1]; memcpy(meta->buf, &mem[2], length - 2); meta_buf->bytesused += length + sizeof(meta->ns) + sizeof(meta->sof); + spin_unlock_irq(&qmeta->irqlock);
uvc_dbg(stream->dev, FRAME, "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame SOF %u\n",
Regards!
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf) return; has_pts = mem[1] & UVC_STREAM_PTS;@@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
- Completion handler for video URBs.
*/
-static void uvc_video_next_buffers(struct uvc_streaming *stream,
struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf)+static void uvc_video_next_meta(struct uvc_streaming *stream,
struct uvc_buffer *video_buf){
uvc_video_validate_buffer(stream, *video_buf);
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf;struct vb2_v4l2_buffer *vb2_meta;const struct vb2_v4l2_buffer *vb2_video;
if (*meta_buf) {struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf;const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf;
if (!vb2_qmeta)return;
vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
guard(spinlock_irqsave)(&qmeta->irqlock);
(*meta_buf)->state = UVC_BUF_STATE_READY;if (!(*meta_buf)->error)(*meta_buf)->error = (*video_buf)->error;*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,*meta_buf);}*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf)return;list_del(&meta_buf->queue);vb2_meta = &meta_buf->buf;vb2_video = &video_buf->buf;vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;meta_buf->state = UVC_BUF_STATE_READY;if (!meta_buf->error)meta_buf->error = video_buf->error;uvc_queue_buffer_release(meta_buf);+}
+static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream,
struct uvc_buffer *video_buf)+{
uvc_video_validate_buffer(stream, video_buf);uvc_video_next_meta(stream, video_buf);return uvc_queue_next_buffer(&stream->queue, video_buf);}
static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); if (ret < 0) continue;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); /* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret,@@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); }}
static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); /* If an error occurred skip the rest of the payload. */@@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); mem += ret; len -= ret;@@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } stream->bulk.header_size = 0;@@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, }
static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL;
struct uvc_buffer *buf_meta = NULL;unsigned long flags; int ret; switch (urb->status) {@@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb)
buf = uvc_queue_get_current_buffer(queue);
if (vb2_qmeta) {spin_lock_irqsave(&qmeta->irqlock, flags);if (!list_empty(&qmeta->irqqueue))buf_meta = list_first_entry(&qmeta->irqqueue,struct uvc_buffer, queue);spin_unlock_irqrestore(&qmeta->irqlock, flags);}/* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */
stream->decode(uvc_urb, buf, buf_meta);
stream->decode(uvc_urb, buf); /* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) {diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq;
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf); struct { struct video_device vdev;@@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
/* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
struct uvc_buffer *buf);/* debugfs and statistics */ void uvc_debugfs_init(void);
base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
-- Regards,
Laurent Pinchart
-- Ricardo Ribalda
On Mon, Sep 08, 2025 at 12:57:03PM +0200, Ricardo Ribalda wrote:
On Mon, 8 Sept 2025 at 12:25, Laurent Pinchart wrote:
On Mon, Jul 14, 2025 at 10:23:45AM +0000, 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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; }
-void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf)+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
- Buffers may span multiple packets, and even URBs, therefore the active buffer
- remains on the queue until the EOF marker.
*/ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) {
lockdep_assert_held(&queue->irqlock);if (list_empty(&queue->irqqueue)) return NULL;diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
- previous header.
*/ static void uvc_video_decode_meta(struct uvc_streaming *stream,
struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length){
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr;@@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
if (!meta_buf || length == 2)
if (!vb2_qmeta || length <= 2)return;guard(spinlock_irqsave)(&qmeta->irqlock);This keeps the spinlock held for longer than I would like. We should really try to minimize the amount of work performed with a spinlock held.
We are using meta_buf the whole function, which can disappear if the user closes the metadata file descriptor.
Besides memcopying meta_buf, how would you suggest reducing the spinlock held time?
I'm thinking about a handshake with .stop_streaming(). The .stop_streaming() operation can sleep, so we can just guard with a spinlock the operation that acquires a metadata buffer, if we ensure that .stop_streaming() waits until it completes.
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf) return; has_pts = mem[1] & UVC_STREAM_PTS;@@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
- Completion handler for video URBs.
*/
-static void uvc_video_next_buffers(struct uvc_streaming *stream,
struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf)+static void uvc_video_next_meta(struct uvc_streaming *stream,
struct uvc_buffer *video_buf){
uvc_video_validate_buffer(stream, *video_buf);
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf;struct vb2_v4l2_buffer *vb2_meta;const struct vb2_v4l2_buffer *vb2_video;
if (*meta_buf) {struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf;const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf;
if (!vb2_qmeta)return;
vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
guard(spinlock_irqsave)(&qmeta->irqlock);
(*meta_buf)->state = UVC_BUF_STATE_READY;if (!(*meta_buf)->error)(*meta_buf)->error = (*video_buf)->error;*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,*meta_buf);}*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf)return;list_del(&meta_buf->queue);vb2_meta = &meta_buf->buf;vb2_video = &video_buf->buf;vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;meta_buf->state = UVC_BUF_STATE_READY;if (!meta_buf->error)meta_buf->error = video_buf->error;uvc_queue_buffer_release(meta_buf);+}
+static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream,
struct uvc_buffer *video_buf)+{
uvc_video_validate_buffer(stream, video_buf);uvc_video_next_meta(stream, video_buf);return uvc_queue_next_buffer(&stream->queue, video_buf);}
static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); if (ret < 0) continue;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); /* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret,@@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); }}
static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); /* If an error occurred skip the rest of the payload. */@@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); mem += ret; len -= ret;@@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } stream->bulk.header_size = 0;@@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, }
static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL;
struct uvc_buffer *buf_meta = NULL;unsigned long flags; int ret; switch (urb->status) {@@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb)
buf = uvc_queue_get_current_buffer(queue);
if (vb2_qmeta) {spin_lock_irqsave(&qmeta->irqlock, flags);if (!list_empty(&qmeta->irqqueue))buf_meta = list_first_entry(&qmeta->irqqueue,struct uvc_buffer, queue);spin_unlock_irqrestore(&qmeta->irqlock, flags);}/* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */
stream->decode(uvc_urb, buf, buf_meta);
stream->decode(uvc_urb, buf); /* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) {diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq;
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf); struct { struct video_device vdev;@@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
/* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
struct uvc_buffer *buf);/* debugfs and statistics */ void uvc_debugfs_init(void);
base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
On Mon, 8 Sept 2025 at 16:01, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Mon, Sep 08, 2025 at 12:57:03PM +0200, Ricardo Ribalda wrote:
On Mon, 8 Sept 2025 at 12:25, Laurent Pinchart wrote:
On Mon, Jul 14, 2025 at 10:23:45AM +0000, 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 modifying the way the metadata buffer lifetime works. We can keep the queue->irqlock while the use the current metadata buffer.
The core of this change is uvc_video_decode_meta(), it now obtains the buffer and holds the spinlock instead of getting the buffer as an argument.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@pendragon.ideason... 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_isight.c | 3 +- drivers/media/usb/uvc/uvc_queue.c | 4 +- drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++++---------------- drivers/media/usb/uvc/uvcvideo.h | 8 ++-- 4 files changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c index 43cda5e760a345af56186603e2f0594b814cdbcb..f0e71744d25cab98184335b46569b31ba1346e12 100644 --- a/drivers/media/usb/uvc/uvc_isight.c +++ b/drivers/media/usb/uvc/uvc_isight.c @@ -98,8 +98,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf, return 0; }
-void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
struct uvc_buffer *meta_buf)+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf) { struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 790184c9843d211d34fa7d66801631d5a07450bd..e184e3ae0f59f142a683263168724bca64509628 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -310,9 +310,11 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
- Buffers may span multiple packets, and even URBs, therefore the active buffer
- remains on the queue until the EOF marker.
*/ -static struct uvc_buffer * +struct uvc_buffer * __uvc_queue_get_current_buffer(struct uvc_video_queue *queue) {
lockdep_assert_held(&queue->irqlock);if (list_empty(&queue->irqqueue)) return NULL;diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 2e377e7b9e81599aca19b800a171cc16a09c1e8a..d6777090d0f892ffe93696c915acd4ec171ca798 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1428,9 +1428,11 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
- previous header.
*/ static void uvc_video_decode_meta(struct uvc_streaming *stream,
struct uvc_buffer *meta_buf, const u8 *mem, unsigned int length){
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf; struct uvc_meta_buf *meta; size_t len_std = 2; bool has_pts, has_scr;@@ -1439,7 +1441,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, ktime_t time; const u8 *scr;
if (!meta_buf || length == 2)
if (!vb2_qmeta || length <= 2)return;guard(spinlock_irqsave)(&qmeta->irqlock);This keeps the spinlock held for longer than I would like. We should really try to minimize the amount of work performed with a spinlock held.
We are using meta_buf the whole function, which can disappear if the user closes the metadata file descriptor.
Besides memcopying meta_buf, how would you suggest reducing the spinlock held time?
I'm thinking about a handshake with .stop_streaming(). The .stop_streaming() operation can sleep, so we can just guard with a spinlock the operation that acquires a metadata buffer, if we ensure that .stop_streaming() waits until it completes.
Is this what you have in mind? (WARNING! non tested, think of it as pseudocode)
We still have to grab the spinlock twice in uvc_video_decode_meta(). I do not have the numbers, but I doubt that it is going to be much more efficient than the other versions.
We could try to use memory barriers... but the chances of screwing things up grow.
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index e184e3ae0f59..761be28b0088 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -212,7 +212,18 @@ static void uvc_stop_streaming_meta(struct vb2_queue *vq)
lockdep_assert_irqs_enabled();
+ while (1) { + spin_lock_irq(&queue->irqlock); + if (!queue->buffer_in_use) { + queue->buffer_in_use = true; + spin_unlock_irq(&queue->irqlock); + break; + } + spin_unlock_irq(&queue->irqlock); + } + uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR); + spin_lock_irq(&queue->irqlock); // probably not needed + queue->buffer_in_use = false; + spin_unlock_irq(&queue->irqlock); }
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 178f1e40c189..08a47e1f5d15 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1430,11 +1430,15 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, if (!vb2_qmeta || length <= 2) return;
- guard(spinlock_irqsave)(&qmeta->irqlock); - + spin_lock_irq(&qmeta->irqlock); meta_buf = __uvc_queue_get_current_buffer(qmeta); - if (!meta_buf) + if (!meta_buf || qmeta->buffer_in_use) { + spin_unlock_irq(&qmeta->irqlock); return; + } + + qmeta->buffer_in_use = true; + spin_unlock_irq(&qmeta->irqlock);
has_pts = mem[1] & UVC_STREAM_PTS; has_scr = mem[1] & UVC_STREAM_SCR; @@ -1454,12 +1458,12 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
if (length == len_std && (!has_scr || !memcmp(scr, stream->clock.last_scr, 6))) - return; + goto done;
if (meta_buf->length - meta_buf->bytesused < length + sizeof(meta->ns) + sizeof(meta->sof)) { meta_buf->error = 1; - return; + goto done; } meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf->bytesused); @@ -1485,6 +1489,12 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream, has_pts ? *(u32 *)meta->buf : 0, has_scr ? *(u32 *)scr : 0, has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0); + +done: + + spin_lock_irq(&qmeta->irqlock); + qmeta->buffer_in_use = false; + spin_unlock_irq(&qmeta->irqlock); }
/* ------------------------------------------------------------------------ diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index ccaab8c5a501..c5d954aacb20 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,8 +337,9 @@ struct uvc_video_queue { unsigned int flags; unsigned int buf_used;
- spinlock_t irqlock; /* Protects irqqueue */ + spinlock_t irqlock; /* Protects irqqueue, buffer_in_use */ struct list_head irqqueue; + bool buffer_in_use; };
struct uvc_video_chain {
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf) return; has_pts = mem[1] & UVC_STREAM_PTS;@@ -1512,30 +1520,48 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
- Completion handler for video URBs.
*/
-static void uvc_video_next_buffers(struct uvc_streaming *stream,
struct uvc_buffer **video_buf, struct uvc_buffer **meta_buf)+static void uvc_video_next_meta(struct uvc_streaming *stream,
struct uvc_buffer *video_buf){
uvc_video_validate_buffer(stream, *video_buf);
struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;struct uvc_video_queue *qmeta = &stream->meta.queue;struct uvc_buffer *meta_buf;struct vb2_v4l2_buffer *vb2_meta;const struct vb2_v4l2_buffer *vb2_video;
if (*meta_buf) {struct vb2_v4l2_buffer *vb2_meta = &(*meta_buf)->buf;const struct vb2_v4l2_buffer *vb2_video = &(*video_buf)->buf;
if (!vb2_qmeta)return;
vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;
guard(spinlock_irqsave)(&qmeta->irqlock);
(*meta_buf)->state = UVC_BUF_STATE_READY;if (!(*meta_buf)->error)(*meta_buf)->error = (*video_buf)->error;*meta_buf = uvc_queue_next_buffer(&stream->meta.queue,*meta_buf);}*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
meta_buf = __uvc_queue_get_current_buffer(qmeta);if (!meta_buf)return;list_del(&meta_buf->queue);vb2_meta = &meta_buf->buf;vb2_video = &video_buf->buf;vb2_meta->sequence = vb2_video->sequence;vb2_meta->field = vb2_video->field;vb2_meta->vb2_buf.timestamp = vb2_video->vb2_buf.timestamp;meta_buf->state = UVC_BUF_STATE_READY;if (!meta_buf->error)meta_buf->error = video_buf->error;uvc_queue_buffer_release(meta_buf);+}
+static struct uvc_buffer *uvc_video_next_buffer(struct uvc_streaming *stream,
struct uvc_buffer *video_buf)+{
uvc_video_validate_buffer(stream, video_buf);uvc_video_next_meta(stream, video_buf);return uvc_queue_next_buffer(&stream->queue, video_buf);}
static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1559,13 +1585,13 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, ret = uvc_video_decode_start(stream, buf, mem, urb->iso_frame_desc[i].actual_length); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); if (ret < 0) continue;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); /* Decode the payload data. */ uvc_video_decode_data(uvc_urb, buf, mem + ret,@@ -1576,12 +1602,12 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb, urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); }}
static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1607,7 +1633,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, do { ret = uvc_video_decode_start(stream, buf, mem, len); if (ret == -EAGAIN)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } while (ret == -EAGAIN); /* If an error occurred skip the rest of the payload. */@@ -1617,7 +1643,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, memcpy(stream->bulk.header, mem, ret); stream->bulk.header_size = ret;
uvc_video_decode_meta(stream, meta_buf, mem, ret);
uvc_video_decode_meta(stream, mem, ret); mem += ret; len -= ret;@@ -1644,7 +1670,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, uvc_video_decode_end(stream, buf, stream->bulk.header, stream->bulk.payload_size); if (buf->state == UVC_BUF_STATE_READY)
uvc_video_next_buffers(stream, &buf, &meta_buf);
buf = uvc_video_next_buffer(stream, buf); } stream->bulk.header_size = 0;@@ -1654,7 +1680,7 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb, }
static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
struct uvc_buffer *buf){ struct urb *urb = uvc_urb->urb; struct uvc_streaming *stream = uvc_urb->stream; @@ -1707,8 +1733,6 @@ static void uvc_video_complete(struct urb *urb) struct uvc_video_queue *qmeta = &stream->meta.queue; struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue; struct uvc_buffer *buf = NULL;
struct uvc_buffer *buf_meta = NULL;unsigned long flags; int ret; switch (urb->status) {@@ -1734,14 +1758,6 @@ static void uvc_video_complete(struct urb *urb)
buf = uvc_queue_get_current_buffer(queue);
if (vb2_qmeta) {spin_lock_irqsave(&qmeta->irqlock, flags);if (!list_empty(&qmeta->irqqueue))buf_meta = list_first_entry(&qmeta->irqqueue,struct uvc_buffer, queue);spin_unlock_irqrestore(&qmeta->irqlock, flags);}/* Re-initialise the URB async work. */ uvc_urb->async_operations = 0;@@ -1755,7 +1771,7 @@ static void uvc_video_complete(struct urb *urb) * Process the URB headers, and optionally queue expensive memcpy tasks * to be deferred to a work queue. */
stream->decode(uvc_urb, buf, buf_meta);
stream->decode(uvc_urb, buf); /* If no async work is needed, resubmit the URB immediately. */ if (!uvc_urb->async_operations) {diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 757254fc4fe930ae61c9d0425f04d4cd074a617e..bb41477ce4ff5cdbf27bc9d830b63a60645e3fa1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -479,8 +479,7 @@ struct uvc_streaming { unsigned int frozen : 1; struct uvc_video_queue queue; struct workqueue_struct *async_wq;
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf); struct { struct video_device vdev;@@ -694,6 +693,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type); void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect); struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer *buf); +struct uvc_buffer * +__uvc_queue_get_current_buffer(struct uvc_video_queue *queue); struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue); void uvc_queue_buffer_release(struct uvc_buffer *buf); static inline int uvc_queue_streaming(struct uvc_video_queue *queue) @@ -802,8 +803,7 @@ u16 uvc_endpoint_max_bpi(struct usb_device *dev, struct usb_host_endpoint *ep);
/* Quirks support */ void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
struct uvc_buffer *buf,struct uvc_buffer *meta_buf);
struct uvc_buffer *buf);/* debugfs and statistics */ void uvc_debugfs_init(void);
base-commit: d968e50b5c26642754492dea23cbd3592bde62d8 change-id: 20250714-uvc-racemeta-fee2e69bbfcd
-- Regards,
Laurent Pinchart
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Ricardo Ribalda