Patchwork mtd/docg3: fix error handling in docg3_probe()

login
register
mail settings
Submitter Dan Carpenter
Date Nov. 24, 2011, 7:21 a.m.
Message ID <20111124072117.GC14122@elgon.mountain>
Download mbox | patch
Permalink /patch/127447/
State New
Headers show

Comments

Dan Carpenter - Nov. 24, 2011, 7:21 a.m.
There was a kfree(docg3_floors); missing from the error handling
here.  Also we set docg3_floors[floor] = mtd; when mtd was an ERR_PTR
and then we call doc_release_device() on it.

I reworked it a bit, so hopefully the code is more clear now.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
walter harms - Nov. 24, 2011, 9:02 a.m.
Am 24.11.2011 08:21, schrieb Dan Carpenter:
> There was a kfree(docg3_floors); missing from the error handling
> here.  Also we set docg3_floors[floor] = mtd; when mtd was an ERR_PTR
> and then we call doc_release_device() on it.
> 
> I reworked it a bit, so hopefully the code is more clear now.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 27c4fea..bfc1ea1 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -1110,21 +1110,24 @@ static int __init docg3_probe(struct platform_device *pdev)
>  	if (!docg3_floors)
>  		goto nomem;
>  
> -	ret = 0;
>  	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		mtd = doc_probe_device(base, floor, dev);
> -		if (floor == 0 && !mtd)
> -			goto notfound;
> -		if (!IS_ERR_OR_NULL(mtd))
> -			ret = mtd_device_parse_register(mtd, part_probes,
> -							NULL, NULL, 0);
> -		else
> +		if (IS_ERR(mtd)) {
>  			ret = PTR_ERR(mtd);
> +			goto err_probe;
> +		}
> +		if (!mtd) {
> +			if (floor == 0)
> +				goto notfound;
> +			else
> +				continue;
> +		}
>  		docg3_floors[floor] = mtd;
> +		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
> +		
	the code nicely shows that docg3_floors[floor] will be foobar if mtd==NULL and floor > 0
	I guess that can cause problems with the "doc_release_device(docg3_floors[floor]);"
	
	perhaps a "docg3_floors[floor] = NULL;" can help here ?

	re,
	 wh
				0);
>  		if (ret)
>  			goto err_probe;
> -		if (mtd)
> -			found++;
> +		found++;
>  	}
>  
>  	if (!found)
> @@ -1138,9 +1141,11 @@ notfound:
>  	ret = -ENODEV;
>  	dev_info(dev, "No supported DiskOnChip found\n");
>  err_probe:
> -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
> +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		if (docg3_floors[floor])
>  			doc_release_device(docg3_floors[floor]);
> +	}
> +	kfree(docg3_floors);
>  nomem:
>  	iounmap(base);
>  noress:
> --
> 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
> 
>
Dan Carpenter - Nov. 24, 2011, 9:31 a.m.
On Thu, Nov 24, 2011 at 10:02:53AM +0100, walter harms wrote:
> 	the code nicely shows that docg3_floors[floor] will be foobar if mtd==NULL and floor > 0
> 	I guess that can cause problems with the "doc_release_device(docg3_floors[floor]);"
> 	
> 	perhaps a "docg3_floors[floor] = NULL;" can help here ?
> 

It's allocated with kzalloc() so it already is.

regards,
dan carpenter
Robert Jarzmik - Nov. 24, 2011, 10:17 a.m.
Dan Carpenter <dan.carpenter@oracle.com> writes:

> There was a kfree(docg3_floors); missing from the error handling
> here.  Also we set docg3_floors[floor] = mtd; when mtd was an ERR_PTR
> and then we call doc_release_device() on it.
Hi Dan,

The missing kfree was dealt with by a later patch amending the probe path :
"mtd/docg3: add ECC correction code" submited in [1]. Your patch is in conflict
with this later one.
The doc_release_device() is an excellent catch. I wonder how you found it.

> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 27c4fea..bfc1ea1 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -1110,21 +1110,24 @@ static int __init docg3_probe(struct platform_device *pdev)
>  	if (!docg3_floors)
>  		goto nomem;
>  
> -	ret = 0;
>  	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		mtd = doc_probe_device(base, floor, dev);
> -		if (floor == 0 && !mtd)
> -			goto notfound;
> -		if (!IS_ERR_OR_NULL(mtd))
> -			ret = mtd_device_parse_register(mtd, part_probes,
> -							NULL, NULL, 0);
> -		else
> +		if (IS_ERR(mtd)) {
>  			ret = PTR_ERR(mtd);
> +			goto err_probe;
> +		}
> +		if (!mtd) {
> +			if (floor == 0)
> +				goto notfound;
> +			else
> +				continue;
> +		}
>  		docg3_floors[floor] = mtd;
> +		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
> +						0);
>  		if (ret)
>  			goto err_probe;
> -		if (mtd)
> -			found++;
> +		found++;
>  	}
Okay, this looks better that the original code.

>  
>  	if (!found)
> @@ -1138,9 +1141,11 @@ notfound:
>  	ret = -ENODEV;
>  	dev_info(dev, "No supported DiskOnChip found\n");
>  err_probe:
> -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
> +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>  		if (docg3_floors[floor])
>  			doc_release_device(docg3_floors[floor]);
> +	}
> +	kfree(docg3_floors);
This is in conflict. Could you drop that hunk and wait for the other patch to go
upstream ? Or alternatively use the whole serie in [2] as your base ?
I think some patches of the serie didn't make it in the tree you're
using.

Could you have a look at the tree with the whole serie, and rebase your patch on
top of it ?


Cheers.
Dan Carpenter - Nov. 24, 2011, 10:31 a.m.
On Thu, Nov 24, 2011 at 11:17:56AM +0100, Robert Jarzmik wrote:
> >  	if (!found)
> > @@ -1138,9 +1141,11 @@ notfound:
> >  	ret = -ENODEV;
> >  	dev_info(dev, "No supported DiskOnChip found\n");
> >  err_probe:
> > -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
> > +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
> >  		if (docg3_floors[floor])
> >  			doc_release_device(docg3_floors[floor]);
> > +	}
> > +	kfree(docg3_floors);
> This is in conflict. Could you drop that hunk and wait for the other patch to go
> upstream ? Or alternatively use the whole serie in [2] as your base ?
> I think some patches of the serie didn't make it in the tree you're
> using.
> 

I'm on linux-next.  They're all going to hit linux next soon right?
I can redo the patch later in that case.

regards,
dan carpenter
Robert Jarzmik - Nov. 24, 2011, 9 p.m.
Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Thu, Nov 24, 2011 at 11:17:56AM +0100, Robert Jarzmik wrote:
>> >  	if (!found)
>> > @@ -1138,9 +1141,11 @@ notfound:
>> >  	ret = -ENODEV;
>> >  	dev_info(dev, "No supported DiskOnChip found\n");
>> >  err_probe:
>> > -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
>> > +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
>> >  		if (docg3_floors[floor])
>> >  			doc_release_device(docg3_floors[floor]);
>> > +	}
>> > +	kfree(docg3_floors);
>> This is in conflict. Could you drop that hunk and wait for the other patch to go
>> upstream ? Or alternatively use the whole serie in [2] as your base ?
>> I think some patches of the serie didn't make it in the tree you're
>> using.
>> 
>
> I'm on linux-next.  They're all going to hit linux next soon right?
Euh, I don't know, it's Artem decision there, as he's taking my patches.

Cheers.
Artem Bityutskiy - Nov. 24, 2011, 9:08 p.m.
On Thu, 2011-11-24 at 22:00 +0100, Robert Jarzmik wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > On Thu, Nov 24, 2011 at 11:17:56AM +0100, Robert Jarzmik wrote:
> >> >  	if (!found)
> >> > @@ -1138,9 +1141,11 @@ notfound:
> >> >  	ret = -ENODEV;
> >> >  	dev_info(dev, "No supported DiskOnChip found\n");
> >> >  err_probe:
> >> > -	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
> >> > +	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
> >> >  		if (docg3_floors[floor])
> >> >  			doc_release_device(docg3_floors[floor]);
> >> > +	}
> >> > +	kfree(docg3_floors);
> >> This is in conflict. Could you drop that hunk and wait for the other patch to go
> >> upstream ? Or alternatively use the whole serie in [2] as your base ?
> >> I think some patches of the serie didn't make it in the tree you're
> >> using.
> >> 
> >
> > I'm on linux-next.  They're all going to hit linux next soon right?
> Euh, I don't know, it's Artem decision there, as he's taking my patches.

My l2-mtd-2.6.git tre is in linux-next, which means all your patches
from your tree are also in linux next already.

Please, send incremental fixes - for this and for the compilation/sparse
warnings. I will also ad Reviewed-by from Ivan and Mike.

Artem.
Artem Bityutskiy - Nov. 24, 2011, 9:29 p.m.
On Thu, 2011-11-24 at 22:00 +0100, Robert Jarzmik wrote:
> > I'm on linux-next.  They're all going to hit linux next soon right?
> Euh, I don't know, it's Artem decision there, as he's taking my patches.

Sorry, I've just noticed that I actually pushed out only some of your
patches. Fixed now - pushed the whole series and added Reviewed-by.
Please, check. There are a couple of sparse/gcc-4.6 warnings still there
when I apply whole series:

  CHECK   drivers/mtd/devices/docg3.c
drivers/mtd/devices/docg3.c:577:20: warning: dubious: !x | y
  CC [M]  drivers/mtd/devices/docg3.o
drivers/mtd/devices/docg3.c: In function ‘doc_write_page_ecc_init’:
drivers/mtd/devices/docg3.c:577:6: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’ [-Wparentheses]
drivers/mtd/devices/docg3.c: At top level:
drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined but not used [-Wunused-function]
  CHK     include/linux/version.h

Artem.
Robert Jarzmik - Nov. 26, 2011, 10:58 a.m.
Dan Carpenter <dan.carpenter@oracle.com> writes:

> I'm on linux-next.  They're all going to hit linux next soon right?
> I can redo the patch later in that case.
Hi Dan,

I saw the patches reach linux-next.
Could you rework your patch on top of the current linux-next please (ie. on top
of next-20111125 or later), as hopefully the code won't evolve much now ?

Cheers.

Patch

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 27c4fea..bfc1ea1 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1110,21 +1110,24 @@  static int __init docg3_probe(struct platform_device *pdev)
 	if (!docg3_floors)
 		goto nomem;
 
-	ret = 0;
 	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
 		mtd = doc_probe_device(base, floor, dev);
-		if (floor == 0 && !mtd)
-			goto notfound;
-		if (!IS_ERR_OR_NULL(mtd))
-			ret = mtd_device_parse_register(mtd, part_probes,
-							NULL, NULL, 0);
-		else
+		if (IS_ERR(mtd)) {
 			ret = PTR_ERR(mtd);
+			goto err_probe;
+		}
+		if (!mtd) {
+			if (floor == 0)
+				goto notfound;
+			else
+				continue;
+		}
 		docg3_floors[floor] = mtd;
+		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
+						0);
 		if (ret)
 			goto err_probe;
-		if (mtd)
-			found++;
+		found++;
 	}
 
 	if (!found)
@@ -1138,9 +1141,11 @@  notfound:
 	ret = -ENODEV;
 	dev_info(dev, "No supported DiskOnChip found\n");
 err_probe:
-	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
+	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
 		if (docg3_floors[floor])
 			doc_release_device(docg3_floors[floor]);
+	}
+	kfree(docg3_floors);
 nomem:
 	iounmap(base);
 noress: