Re: [PATCH 1/6 v5] UVC: fix .queue_setup() to check the number of planes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guennadi,

Thank you for the patch.

On Friday 28 Jul 2017 14:33:20 Guennadi Liakhovetski wrote:
> According to documentation of struct vb2_ops the .queue_setup() callback
> should return an error if the number of planes parameter contains an
> invalid value on input. Fix this instead of ignoring the value.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index aa21997..371a4ad 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -84,7 +84,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
> 
>  	/* Make sure the image size is large enough. */

Nitpicking, I'd update the comment as well.

        /*
	 * When called with plane sizes, validate them. The driver supports
	 * single planar formats only, and requires buffers to be large enough
	 * to store a complete frame.
	 */

>  	if (*nplanes)
> -		return sizes[0] < size ? -EINVAL : 0;
> +		return sizes[0] < size || *nplanes != 1 ? -EINVAL : 0;

Nitpicking again, I'd test *nplanes first, as it conditions which entries of 
the sizes array are valid. If course the if (*nplanes) test ensures that entry 
0 is valid, so it won't make a difference at runtime, it's just about code 
readability.

The patch looks good otherwise,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	*nplanes = 1;
>  	sizes[0] = size;
>  	return 0;

-- 
Regards,

Laurent Pinchart