* [patch] staging: bcm2835-camera: free first element in array
@ 2017-02-15 12:25 Dan Carpenter
2017-02-15 12:47 ` walter harms
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2017-02-15 12:25 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Eric Anholt
Cc: Greg Kroah-Hartman, Stephen Warren, Lee Jones, Florian Fainelli,
Ray Jui, Scott Branden, bcm-kernel-feedback-list, Arnd Bergmann,
linux-media, devel, linux-rpi-kernel, kernel-janitors
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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch] staging: bcm2835-camera: free first element in array
2017-02-15 12:25 [patch] staging: bcm2835-camera: free first element in array Dan Carpenter
@ 2017-02-15 12:47 ` walter harms
2017-02-16 12:03 ` Dan Carpenter
2017-02-17 23:20 ` [patch v2] staging: bcm2835-camera: fix error handling in init Dan Carpenter
0 siblings, 2 replies; 5+ messages in thread
From: walter harms @ 2017-02-15 12:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Eric Anholt, Greg Kroah-Hartman,
Stephen Warren, Lee Jones, Florian Fainelli, Ray Jui,
Scott Branden, bcm-kernel-feedback-list, Arnd Bergmann,
linux-media, devel, linux-rpi-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] staging: bcm2835-camera: free first element in array
2017-02-15 12:47 ` walter harms
@ 2017-02-16 12:03 ` Dan Carpenter
2017-02-17 23:20 ` [patch v2] staging: bcm2835-camera: fix error handling in init Dan Carpenter
1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2017-02-16 12:03 UTC (permalink / raw)
To: walter harms
Cc: Mauro Carvalho Chehab, Eric Anholt, Greg Kroah-Hartman,
Stephen Warren, Lee Jones, Florian Fainelli, Ray Jui,
Scott Branden, bcm-kernel-feedback-list, Arnd Bergmann,
linux-media, devel, linux-rpi-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [patch v2] staging: bcm2835-camera: fix error handling in init
2017-02-15 12:47 ` walter harms
2017-02-16 12:03 ` Dan Carpenter
@ 2017-02-17 23:20 ` Dan Carpenter
2017-02-18 11:35 ` walter harms
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2017-02-17 23:20 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, Stephen Warren, Lee Jones, Eric Anholt,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Arnd Bergmann, linux-media, devel,
linux-rpi-kernel, kernel-janitors
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);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch v2] staging: bcm2835-camera: fix error handling in init
2017-02-17 23:20 ` [patch v2] staging: bcm2835-camera: fix error handling in init Dan Carpenter
@ 2017-02-18 11:35 ` walter harms
0 siblings, 0 replies; 5+ messages in thread
From: walter harms @ 2017-02-18 11:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, Stephen Warren,
Lee Jones, Eric Anholt, Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Arnd Bergmann, linux-media, devel,
linux-rpi-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-18 11:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 12:25 [patch] staging: bcm2835-camera: free first element in array Dan Carpenter
2017-02-15 12:47 ` walter harms
2017-02-16 12:03 ` Dan Carpenter
2017-02-17 23:20 ` [patch v2] staging: bcm2835-camera: fix error handling in init Dan Carpenter
2017-02-18 11:35 ` walter harms
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).