| Message ID | 20111124072117.GC14122@elgon.mountain |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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
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.
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
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.
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.
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.
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.
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:
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>