Re: [PATCH 1/3] media: uvcvideo: Let uvc_parse_frame() report a skipped frame

[Ricardo Ribalda]

Hi Natasha

On Thu, 20 Aug 2026 at 11:56, Natasha Klaus
<natalie.klaus@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> uvc_parse_frame() returns the descriptor length on success and a
> negative error code on failure, and uvc_parse_format() treats every
> negative value as fatal for the whole streaming interface. There is no
> way for the parser to say "this frame descriptor is unusable, but the
> rest of the format is fine".
>
> Change the return convention so it can. Return 0 on success and let the
> caller advance by buffer[0], which is the value the function returned
> anyway. Report a truncated descriptor with -ENODATA, which stays fatal,
> and leave every other negative value to mean "skip this frame descriptor
> and carry on with the next one".
>
> -ENODATA is currently the only error the function can return, so the
> skip path is unreachable until later patches add checks that use it. The
> one behavioural change is the truncated-descriptor diagnostic, which
> moves from uvc_dbg() to dev_warn() so a malformed descriptor is reported
> without the DESCR debug flag. That leaves the local alts variable
> unused, and the kernel builds -Wunused-variable as an error, so it goes
> too.
>
> Suggested-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> Link: https://lore.kernel.org/linux-media/CANiDSCue8yyiGubzbAybRqSUTTFuB=-Y2TZy6yvOx32SpAASWg@xxxxxxxxxxxxxx/
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Natasha Klaus <natalie.klaus@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> The Cc: stable line is present without a Fixes: tag because this patch is
> a prerequisite for 2/3 rather than a fix in its own right. Stable needs
> both or neither: backported alone, 2/3's -EINVAL would revert to meaning
> "discard the whole streaming interface".
>
>  drivers/media/usb/uvc/uvc_driver.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e289cc71ba98..0cc0e351d139 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -230,7 +230,6 @@ static int uvc_parse_frame(struct uvc_device *dev,
>                            u32 **intervals, u8 ftype, int width_multiplier,
>                            const unsigned char *buffer, int buflen)
>  {
> -       struct usb_host_interface *alts = streaming->intf->cur_altsetting;
>         unsigned int maxIntervalIndex;
>         unsigned int interval;
>         unsigned int i, n;
> @@ -243,10 +242,10 @@ static int uvc_parse_frame(struct uvc_device *dev,
>         n = n ? n : 3;
>
>         if (buflen < 26 + 4 * n) {
> -               uvc_dbg(dev, DESCR,
> -                       "device %d videostreaming interface %d FRAME error\n",
> -                       dev->udev->devnum, alts->desc.bInterfaceNumber);
> -               return -EINVAL;
> +               dev_warn(&streaming->intf->dev,
> +                        "UVC non compliance: FRAME descriptor is %d bytes, expected at least %u.\n",
> +                        buflen, 26 + 4 * n);
> +               return -ENODATA;
>         }
>
>         frame->bFrameIndex = buffer[3];
> @@ -329,7 +328,7 @@ static int uvc_parse_frame(struct uvc_device *dev,
>
>         *intervals += n;
>
> -       return buffer[0];
> +       return 0;
>  }
>
>  static int uvc_parse_format(struct uvc_device *dev,
> @@ -492,11 +491,12 @@ static int uvc_parse_format(struct uvc_device *dev,
>                         ret = uvc_parse_frame(dev, streaming, format, frame,
>                                               intervals, ftype, width_multiplier,
>                                               buffer, buflen);
> -                       if (ret < 0)
> +                       if (!ret)
> +                               format->nframes++;
> +                       if (ret == -ENODATA)
>                                 return ret;

I know this is my proposal, but if you resubmit the series I think it
is more correct to swap the order (sorry about that):
 +                       if (ret == -ENODATA)
 +                                return ret;
 +                       if (!ret)
 +                               format->nframes++;

> -                       format->nframes++;
> -                       buflen -= ret;
> -                       buffer += ret;
> +                       buflen -= buffer[0];
> +                       buffer += buffer[0];
>                 }
>         }
>
> --
> 2.34.1
>
With that minor nitpick, regardless of whether you change it or not.

Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>

-- 
Ricardo Ribalda