1. Patchew
  2. linux
  3. [PATCH v3 0/2] media: uvcvideo: Fixes for frame sequence number
  4. View patch

[PATCH v3 2/2] media: uvcvideo: Fix sequence number when no EOF

Ricardo Ribalda posted 2 patches 3 weeks ago
There is a newer version of this series
[PATCH v3 2/2] media: uvcvideo: Fix sequence number when no EOF
Posted by Ricardo Ribalda 3 weeks ago 
If the driver could not detect the EOF, the sequence number is increased
twice:
 1) When we enter uvc_video_decode_start() with the old buffer and FID has
   fliped => We return -EAGAIN and last_fid is not flipped
 2) When we enter uvc_video_decode_start() with the new buffer.

Fix this issue by saving last_fid on the first FID flip.

Cc: stable@kernel.org
Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
Reported-by: Hans de Goede <hansg@kernel.org>
Closes: https://lore.kernel.org/linux-media/CANiDSCuj4cPuB5_v2xyvAagA5FjoN8V5scXiFFOeD3aKDMqkCg@mail.gmail.com/T/#me39fb134e8c2c085567a31548c3403eb639625e4
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 9e06b1d0f0f9..3e6ded69388f 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1254,6 +1254,12 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 		uvc_dbg(stream->dev, FRAME,
 			"Frame complete (FID bit toggled)\n");
 		buf->state = UVC_BUF_STATE_READY;
+
+		/*
+		 * If the EOF detection has failed, we need to save the last_fid
+		 * to avoid increasing the sequence number twice.
+		 */
+		stream->last_fid = fid;
 		return -EAGAIN;
 	}
 

-- 
2.53.0.851.ga537e3e6e9-goog
Re: [PATCH v3 2/2] media: uvcvideo: Fix sequence number when no EOF
Posted by Hans de Goede 2 weeks, 3 days ago 
Hi,

On 16-Mar-26 14:30, Ricardo Ribalda wrote:
> If the driver could not detect the EOF, the sequence number is increased
> twice:
>  1) When we enter uvc_video_decode_start() with the old buffer and FID has
>    fliped => We return -EAGAIN and last_fid is not flipped
>  2) When we enter uvc_video_decode_start() with the new buffer.
> 
> Fix this issue by saving last_fid on the first FID flip.
> 
> Cc: stable@kernel.org
> Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
> Reported-by: Hans de Goede <hansg@kernel.org>
> Closes: https://lore.kernel.org/linux-media/CANiDSCuj4cPuB5_v2xyvAagA5FjoN8V5scXiFFOeD3aKDMqkCg@mail.gmail.com/T/#me39fb134e8c2c085567a31548c3403eb639625e4
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 9e06b1d0f0f9..3e6ded69388f 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1254,6 +1254,12 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>  		uvc_dbg(stream->dev, FRAME,
>  			"Frame complete (FID bit toggled)\n");
>  		buf->state = UVC_BUF_STATE_READY;
> +
> +		/*
> +		 * If the EOF detection has failed, we need to save the last_fid
> +		 * to avoid increasing the sequence number twice.
> +		 */
> +		stream->last_fid = fid;

AFAICT, there is still a problem after this patch:

1. We have an incomplete frame, so no EOF, first run through
uvc_video_decode_start()

2. We hit the first if (stream->last_fid != fid) check do
stream->sequence++ . And after patch 1/2 we do NOT update
"buf->buf.sequence = stream->sequence" because buf->bytesused != 0
(which is good, the incomplete frame should not get the new
sequence-no).

3. Still first run through uvc_video_decode_start() we hit the:
if (fid != stream->last_fid && buf->bytesused != 0) check further
down, update "stream->last_fid = fid" (after this patch) and
return -EAGAIN.

4. Second run through uvc_video_decode_start() the first
if (stream->last_fid != fid) check no longer triggers, we
don't increase the sequence-no (good) but we also do not
set "buf->buf.sequence = stream->sequence" for the new buffer
we are called with on the second run!

So the combination of these 2 fixes is broken.

Regards,

Hans
Re: [PATCH v3 2/2] media: uvcvideo: Fix sequence number when no EOF
Posted by Ricardo Ribalda 2 weeks, 3 days ago 
Hi Hans

Good catch. Sending a new version.

Hope that is less broken :P

On Fri, 20 Mar 2026 at 13:17, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 16-Mar-26 14:30, Ricardo Ribalda wrote:
> > If the driver could not detect the EOF, the sequence number is increased
> > twice:
> >  1) When we enter uvc_video_decode_start() with the old buffer and FID has
> >    fliped => We return -EAGAIN and last_fid is not flipped
> >  2) When we enter uvc_video_decode_start() with the new buffer.
> >
> > Fix this issue by saving last_fid on the first FID flip.
> >
> > Cc: stable@kernel.org
> > Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
> > Reported-by: Hans de Goede <hansg@kernel.org>
> > Closes: https://lore.kernel.org/linux-media/CANiDSCuj4cPuB5_v2xyvAagA5FjoN8V5scXiFFOeD3aKDMqkCg@mail.gmail.com/T/#me39fb134e8c2c085567a31548c3403eb639625e4
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 9e06b1d0f0f9..3e6ded69388f 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1254,6 +1254,12 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> >               uvc_dbg(stream->dev, FRAME,
> >                       "Frame complete (FID bit toggled)\n");
> >               buf->state = UVC_BUF_STATE_READY;
> > +
> > +             /*
> > +              * If the EOF detection has failed, we need to save the last_fid
> > +              * to avoid increasing the sequence number twice.
> > +              */
> > +             stream->last_fid = fid;
>
> AFAICT, there is still a problem after this patch:
>
> 1. We have an incomplete frame, so no EOF, first run through
> uvc_video_decode_start()
>
> 2. We hit the first if (stream->last_fid != fid) check do
> stream->sequence++ . And after patch 1/2 we do NOT update
> "buf->buf.sequence = stream->sequence" because buf->bytesused != 0
> (which is good, the incomplete frame should not get the new
> sequence-no).
>
> 3. Still first run through uvc_video_decode_start() we hit the:
> if (fid != stream->last_fid && buf->bytesused != 0) check further
> down, update "stream->last_fid = fid" (after this patch) and
> return -EAGAIN.
>
> 4. Second run through uvc_video_decode_start() the first
> if (stream->last_fid != fid) check no longer triggers, we
> don't increase the sequence-no (good) but we also do not
> set "buf->buf.sequence = stream->sequence" for the new buffer
> we are called with on the second run!
>
> So the combination of these 2 fixes is broken.
>
> Regards,
>
> Hans
>
>


-- 
Ricardo Ribalda

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.