Re: [PATCH v2 2/2] media: uvcvideo: Avoid partial metadata buffers

[Hans de Goede <hansg@kernel.org>]

Hi,

On 17-Apr-26 07:19, Ricardo Ribalda wrote:
> If the metadata queue that is empty receives a new buffer while we are
> in the middle of processing a frame, the first metadata buffer will
> contain partial information.
> 
> Avoid this by tracking the state of the metadata buffer and making sure
> that it is in sync with the data buffer.
> 
> Now that we are at it, make sure that we skip buffers of size 1 or 0.
> They are not allowed by the spec... but it is a simple check to add and
> better be safe than sorry.
> 
> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  drivers/media/usb/uvc/uvc_video.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 5d45c74c6041..cd77ca6f8136 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1149,7 +1149,9 @@ static void uvc_video_stats_stop(struct uvc_streaming *stream)
>   * uvc_video_decode_end will never be called with a NULL buffer.
>   */
>  static int uvc_video_decode_start(struct uvc_streaming *stream,
> -		struct uvc_buffer *buf, const u8 *data, int len)
> +				  struct uvc_buffer *buf,
> +				  struct uvc_buffer *meta_buf,
> +				  const u8 *data, int len)
>  {
>  	u8 header_len;
>  	u8 fid;
> @@ -1278,6 +1280,8 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>  
>  		/* TODO: Handle PTS and SCR. */
>  		buf->state = UVC_BUF_STATE_ACTIVE;
> +		if (meta_buf)
> +			meta_buf->state = UVC_BUF_STATE_ACTIVE;
>  	}
>  
>  	stream->last_fid = fid;
> @@ -1435,7 +1439,7 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
>  	ktime_t time;
>  	const u8 *scr;
>  
> -	if (!meta_buf || length == 2)
> +	if (length <= 2 || !meta_buf || meta_buf->state != UVC_BUF_STATE_ACTIVE)
>  		return;
>  
>  	has_pts = mem[1] & UVC_STREAM_PTS;
> @@ -1552,7 +1556,7 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
>  		/* Decode the payload header. */
>  		mem = urb->transfer_buffer + urb->iso_frame_desc[i].offset;
>  		do {
> -			ret = uvc_video_decode_start(stream, buf, mem,
> +			ret = uvc_video_decode_start(stream, buf, meta_buf, mem,
>  				urb->iso_frame_desc[i].actual_length);
>  			if (ret == -EAGAIN)
>  				uvc_video_next_buffers(stream, &buf, &meta_buf);
> @@ -1601,7 +1605,8 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
>  	 */
>  	if (stream->bulk.header_size == 0 && !stream->bulk.skip_payload) {
>  		do {
> -			ret = uvc_video_decode_start(stream, buf, mem, len);
> +			ret = uvc_video_decode_start(stream, buf, meta_buf, mem,
> +						     len);
>  			if (ret == -EAGAIN)
>  				uvc_video_next_buffers(stream, &buf, &meta_buf);
>  		} while (ret == -EAGAIN);
>