LinuxTV Patchwork [1/6,v5] UVC: fix .queue_setup() to check the number of planes

login
register
mail settings
Submitter Guennadi Liakhovetski
Date July 28, 2017, 12:33 p.m.
Message ID <1501245205-15802-2-git-send-email-g.liakhovetski@gmx.de>
Download mbox | patch
Permalink /patch/42796/
State New
Delegated to: Laurent Pinchart
Headers show

Comments

Guennadi Liakhovetski - July 28, 2017, 12:33 p.m.
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(-)
Laurent Pinchart - July 31, 2017, 1:57 p.m.
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;
Laurent Pinchart - July 31, 2017, 1:58 p.m.
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;

Patch

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. */
 	if (*nplanes)
-		return sizes[0] < size ? -EINVAL : 0;
+		return sizes[0] < size || *nplanes != 1 ? -EINVAL : 0;
 	*nplanes = 1;
 	sizes[0] = size;
 	return 0;

Privacy Policy