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

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

And I forgot to mention, the usual prefix for the subject line is "uvcvideo:" 
followed by a capitalized word.

On Monday 31 Jul 2017 16:57:23 Laurent Pinchart wrote:
> 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