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

[Laurent Pinchart]

On Thu, Aug 20, 2026 at 02:15:54PM +0300, Natasha Klaus 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".

On what device have you seen this occurring ? How did you test this
patch ?

> 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
> Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> Tested-by: Noam Ben Shimon <noambs2999@xxxxxxxxx>
> 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".
> 
> The caller checks -ENODATA before counting the frame, per Ricardo's review.
> Behaviour is unchanged either way, since -ENODATA is non-zero, but the fatal
> case reads better first.
> 
>  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..b94fe5366e55 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 == -ENODATA)
>  				return ret;
> -			format->nframes++;
> -			buflen -= ret;
> -			buffer += ret;
> +			if (!ret)
> +				format->nframes++;
> +			buflen -= buffer[0];
> +			buffer += buffer[0];
>  		}
>  	}
>  

-- 
Regards,

Laurent Pinchart