mtd/docg3: fix error handling in docg3_probe()

PATCH In review Dan Carpenter 4 participants 12 messages Nov 24, 2011 – Dec 1, 2011

Subsystems (3)

Kernel janitors Memory technology devices (MTD) Msystems diskonchip G3 MTD driver

lore

Share & tools

lore

Dan Carpenter

author

2011-11-24 07:21 UTC #0 lore raw

PATCH 1/1 mtd/docg3: fix error handling in docg3_probe()

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 <[email protected]>

drivers/mtd/devices/docg3.c +15 ~~-10~~

index 27c4fea..bfc1ea1 100644
@@ -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:

walter harms

2011-11-24 09:02 UTC #1 lore raw

Am 24.11.2011 08:21, schrieb Dan Carpenter:

Quoted 28 lines

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 <[email protected]>
drivers/mtd/devices/docg3.c +2 -7
index 27c4fea..bfc1ea1 100644
@@ -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;

drivers/mtd/devices/docg3.c static int __init docg3_probe(struct platform_device *pdev) · line 1127

Quoted 10 lines

drivers/mtd/devices/docg3.c +9 -0

+		}
+		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);

Quoted 27 lines

drivers/mtd/devices/docg3.c +4 -4

 		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2011-11-24 09:31 UTC #2 lore raw

Quoted 5 lines · walter harms

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

maintainer

2011-11-24 10:17 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 3 lines · 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.

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.

Quoted 26 lines

drivers/mtd/devices/docg3.c +8 -7

index 27c4fea..bfc1ea1 100644
@@ -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;

drivers/mtd/devices/docg3.c static int __init docg3_probe(struct platform_device *pdev) · line 1131

Quoted 10 lines

drivers/mtd/devices/docg3.c +4 -2

+		}
 		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.

drivers/mtd/devices/docg3.c notfound: · line 1148

Quoted 12 lines

drivers/mtd/devices/docg3.c +3 -1

 	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.

Signature · 4 lines

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

Dan Carpenter

author

2011-11-24 10:31 UTC #4 lore raw

Quoted 7 lines · Robert Jarzmik

Quoted 7 lines

drivers/mtd/devices/docg3.c +1 -1

 	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++) {

Quoted 9 lines

drivers/mtd/devices/docg3.c notfound: · line 1148
Quoted 4 lines
drivers/mtd/devices/docg3.c +2 -0
 		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

maintainer

2011-11-24 21:00 UTC #5 lore raw

Quoted 18 lines · Dan Carpenter

Quoted 7 lines · Robert Jarzmik
Quoted 7 lines
drivers/mtd/devices/docg3.c +1 -1
 	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++) {
Quoted 9 lines
drivers/mtd/devices/docg3.c notfound: · line 1148
Quoted 4 lines
drivers/mtd/devices/docg3.c +2 -0
 		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.

Quoted 1 line

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.

Signature · 1 line

Robert

Artem Bityutskiy

2011-11-24 21:08 UTC #6 lore raw

Quoted 20 lines · Robert Jarzmik

Quoted 18 lines · Dan Carpenter
Quoted 16 lines · Robert Jarzmik
drivers/mtd/devices/docg3.c notfound: · line 1148
Quoted 11 lines
drivers/mtd/devices/docg3.c +3 -1
 	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.

Quoted 2 lines

Quoted 1 line
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

2011-11-24 21:29 UTC #7 lore raw

In reply to Robert Jarzmik

Quoted 2 lines · Robert Jarzmik

Quoted 1 line
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

maintainer

2011-11-26 10:58 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 2 lines · Dan Carpenter

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.

Signature · 1 line

Robert

Dan Carpenter

author

2011-11-28 13:53 UTC #9 lore raw

PATCH v2 1/1 mtd/docg3: dereferencing an ERR_PTR() in docg3_probe()

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 <[email protected]>

---
v2: Version 1 conflicted with another patch in stream.

drivers/mtd/devices/docg3.c +12 -9

index d7df311..f7490a014 100644
@@ -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);

Robert Jarzmik

maintainer Acked-by on v2 1/1

2011-11-29 22:00 UTC #10 lore raw

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

Quoted 10 lines · Dan Carpenter

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 <[email protected]>
---
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 <[email protected]>

Cheers.

--
Robert

Artem Bityutskiy

Applied

2011-12-01 08:02 UTC #11 lore raw

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

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

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 <[email protected]>

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

Artem.