[patch] staging: bcm2835-camera: free first element in array

[patch] staging: bcm2835-camera: free first element in array

[Dan Carpenter]

We should free gdev[0] so the > should be >=.

Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..9bcd8e546a14 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -1998,7 +1998,7 @@ static int __init bm2835_mmal_init(void)
 free_dev:
 	kfree(dev);
 
-	for ( ; camera > 0; camera--) {
+	for ( ; camera >= 0; camera--) {
 		bcm2835_cleanup_instance(gdev[camera]);
 		gdev[camera] = NULL;
 	}

Re: [patch] staging: bcm2835-camera: free first element in array

[walter harms]

Am 15.02.2017 13:25, schrieb Dan Carpenter:
> We should free gdev[0] so the > should be >=.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..9bcd8e546a14 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1998,7 +1998,7 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>  	kfree(dev);
>  
> -	for ( ; camera > 0; camera--) {
> +	for ( ; camera >= 0; camera--) {
>  		bcm2835_cleanup_instance(gdev[camera]);
>  		gdev[camera] = NULL;
>  	}

since we already know that programmers are bad in counting backwards ...

is is possible to change that into std. loop like:

 for(i=0, i< camera; i++ {
	bcm2835_cleanup_instance(gdev[i]);
	gdev[i] = NULL;
  	}

this is way a much more common pattern.


just my 2 cents,
re,
 wh

Re: [patch] staging: bcm2835-camera: free first element in array

[Dan Carpenter]

On Wed, Feb 15, 2017 at 01:47:55PM +0100, walter harms wrote:
> 
> 
> Am 15.02.2017 13:25, schrieb Dan Carpenter:
> > We should free gdev[0] so the > should be >=.
> > 
> > Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> > index ca15a698e018..9bcd8e546a14 100644
> > --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> > +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> > @@ -1998,7 +1998,7 @@ static int __init bm2835_mmal_init(void)
> >  free_dev:
> >  	kfree(dev);
> >  
> > -	for ( ; camera > 0; camera--) {
> > +	for ( ; camera >= 0; camera--) {
> >  		bcm2835_cleanup_instance(gdev[camera]);
> >  		gdev[camera] = NULL;
> >  	}
> 
> since we already know that programmers are bad in counting backwards ...
> 
> is is possible to change that into std. loop like:
> 
>  for(i=0, i< camera; i++ {
> 	bcm2835_cleanup_instance(gdev[i]);
> 	gdev[i] = NULL;
>   	}
> 
> this is way a much more common pattern.

Hm...  My patch is buggy.  It frees the wong thing on the first
iteration through the loop.  I'll resend.

regards,
dan carpenter

[patch v2] staging: bcm2835-camera: fix error handling in init

[Dan Carpenter]

The unwinding here isn't right.  We don't free gdev[0] and instead
free 1 step past what was allocated.  Also we can't allocate "dev" then
we should unwind instead of returning directly.

Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Change the style to make Walter Harms happy.  Fix some additional
    bugs I missed in the first patch.

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
index ca15a698e018..c4dad30dd133 100644
--- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -1901,6 +1901,7 @@ static int __init bm2835_mmal_init(void)
 	unsigned int num_cameras;
 	struct vchiq_mmal_instance *instance;
 	unsigned int resolutions[MAX_BCM2835_CAMERAS][2];
+	int i;
 
 	ret = vchiq_mmal_init(&instance);
 	if (ret < 0)
@@ -1914,8 +1915,10 @@ static int __init bm2835_mmal_init(void)
 
 	for (camera = 0; camera < num_cameras; camera++) {
 		dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
-		if (!dev)
-			return -ENOMEM;
+		if (!dev) {
+			ret = -ENOMEM;
+			goto cleanup_gdev;
+		}
 
 		dev->camera_num = camera;
 		dev->max_width = resolutions[camera][0];
@@ -1998,9 +2001,10 @@ static int __init bm2835_mmal_init(void)
 free_dev:
 	kfree(dev);
 
-	for ( ; camera > 0; camera--) {
-		bcm2835_cleanup_instance(gdev[camera]);
-		gdev[camera] = NULL;
+cleanup_gdev:
+	for (i = 0; i < camera; i++) {
+		bcm2835_cleanup_instance(gdev[i]);
+		gdev[i] = NULL;
 	}
 	pr_info("%s: error %d while loading driver\n",
 		BM2835_MMAL_MODULE_NAME, ret);

Re: [patch v2] staging: bcm2835-camera: fix error handling in init

[walter harms]

looks more readable now :)

Acked-by: wharms@bfs.de

Am 18.02.2017 00:20, schrieb Dan Carpenter:
> The unwinding here isn't right.  We don't free gdev[0] and instead
> free 1 step past what was allocated.  Also we can't allocate "dev" then
> we should unwind instead of returning directly.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Change the style to make Walter Harms happy.  Fix some additional
>     bugs I missed in the first patch.
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..c4dad30dd133 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1901,6 +1901,7 @@ static int __init bm2835_mmal_init(void)
>  	unsigned int num_cameras;
>  	struct vchiq_mmal_instance *instance;
>  	unsigned int resolutions[MAX_BCM2835_CAMERAS][2];
> +	int i;
>  
>  	ret = vchiq_mmal_init(&instance);
>  	if (ret < 0)
> @@ -1914,8 +1915,10 @@ static int __init bm2835_mmal_init(void)
>  
>  	for (camera = 0; camera < num_cameras; camera++) {
>  		dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
> -		if (!dev)
> -			return -ENOMEM;
> +		if (!dev) {
> +			ret = -ENOMEM;
> +			goto cleanup_gdev;
> +		}
>  
>  		dev->camera_num = camera;
>  		dev->max_width = resolutions[camera][0];
> @@ -1998,9 +2001,10 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>  	kfree(dev);
>  
> -	for ( ; camera > 0; camera--) {
> -		bcm2835_cleanup_instance(gdev[camera]);
> -		gdev[camera] = NULL;
> +cleanup_gdev:
> +	for (i = 0; i < camera; i++) {
> +		bcm2835_cleanup_instance(gdev[i]);
> +		gdev[i] = NULL;
>  	}
>  	pr_info("%s: error %d while loading driver\n",
>  		BM2835_MMAL_MODULE_NAME, ret);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>