[patch] mtd/docg3: fix error handling in docg3_probe()

[patch] mtd/docg3: fix error handling in docg3_probe()

[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,
+						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:

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[walter harms]

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
> 
> 

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Robert Jarzmik]

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.

-- 
Robert

[1]: http://lists.infradead.org/pipermail/linux-mtd/2011-November/038496.html
[2]: http://lists.infradead.org/pipermail/linux-mtd/2011-November/038483.html

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Robert Jarzmik]

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.

-- 
Robert

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Artem Bityutskiy]

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.

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Artem Bityutskiy]

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.

Re: [patch] mtd/docg3: fix error handling in docg3_probe()

[Robert Jarzmik]

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.

-- 
Robert

[patch v2] mtd/docg3: dereferencing an ERR_PTR() in docg3_probe()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

If doc_probe_device() returned an ERR_PTR, then we accidentally saved
that to docg3_floors[floor] = mtd; which gets derefenced in the error
handling when we call doc_release_device().

I've reworked the error handling to take care of that and hopefully
make it a little simpler.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Version 1 conflicted with another patch in stream.

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index d7df311..f7490a014 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -2027,21 +2027,24 @@ static int __init docg3_probe(struct platform_device *pdev)
 	if (!docg3_bch)
 		goto nomem2;
 
-	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++;
 	}
 
 	ret = doc_register_sysfs(pdev, docg3_floors);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch v2] mtd/docg3: dereferencing an ERR_PTR() in docg3_probe()

[Robert Jarzmik]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> If doc_probe_device() returned an ERR_PTR, then we accidentally saved
> that to docg3_floors[floor] = mtd; which gets derefenced in the error
> handling when we call doc_release_device().
>
> I've reworked the error handling to take care of that and hopefully
> make it a little simpler.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Version 1 conflicted with another patch in stream.
Ok, applied and tested locally (even if that branch of code obviously is not
triggered in my case), and everything should work even better with it.

Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [patch v2] mtd/docg3: dereferencing an ERR_PTR() in docg3_probe()

[Artem Bityutskiy]

On Mon, 2011-11-28 at 16:53 +0300, Dan Carpenter wrote:
> If doc_probe_device() returned an ERR_PTR, then we accidentally saved
> that to docg3_floors[floor] = mtd; which gets derefenced in the error
> handling when we call doc_release_device().
> 
> I've reworked the error handling to take care of that and hopefully
> make it a little simpler.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Pushed to l2-mtd-2.6.git, thanks!

Artem.