Message ID | 1433193054-26865-7-git-send-email-richard@nod.at |
---|---|
State | Changes Requested |
Headers | show |
On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote: > Don't return a obfuscated null pointer using ERR_PTR(0). > If the no device is found clearly return -ENODEV. > This makes the code more clear and matches the comment > of doc_probe_device(). > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- Have you tested this patch? > drivers/mtd/devices/docg3.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c > index 5e67b4a..630e29a 100644 > --- a/drivers/mtd/devices/docg3.c > +++ b/drivers/mtd/devices/docg3.c > @@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev) > chip_id = doc_register_readw(docg3, DOC_CHIPID); > chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV); > > - ret = 0; > + ret = -ENODEV; > if (chip_id != (u16)(~chip_id_inv)) { > goto nomem4; > } > @@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev) > mtd = doc_probe_device(cascade, floor, dev); > if (IS_ERR(mtd)) { > ret = PTR_ERR(mtd); > - goto err_probe; > - } > - if (!mtd) { > - if (floor == 0) > - goto notfound; > - else > + if (ret == -ENODEV && floor == 0) I think you might have changed the logic when refactoring here. I think the right refactoring would be the following, no? if (ret == -ENODEV && floor != 0) continue; else goto err_probe; > continue; > + else > + goto err_probe; > } > cascade->floors[floor] = mtd; > ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL, > @@ -2091,10 +2088,9 @@ static int __init docg3_probe(struct platform_device *pdev) > doc_dbg_register(cascade->floors[0]->priv); > return 0; > > -notfound: > - ret = -ENODEV; > - dev_info(dev, "No supported DiskOnChip found\n"); > err_probe: > + if (ret == -ENODEV) > + dev_info(dev, "No supported DiskOnChip found\n"); > free_bch(cascade->bch); > for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) > if (cascade->floors[floor]) Brian
Am 17.06.2015 um 20:41 schrieb Brian Norris: > On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote: >> Don't return a obfuscated null pointer using ERR_PTR(0). >> If the no device is found clearly return -ENODEV. >> This makes the code more clear and matches the comment >> of doc_probe_device(). >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- > > Have you tested this patch? nah, I don't own such a device. >> drivers/mtd/devices/docg3.c | 16 ++++++---------- >> 1 file changed, 6 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c >> index 5e67b4a..630e29a 100644 >> --- a/drivers/mtd/devices/docg3.c >> +++ b/drivers/mtd/devices/docg3.c >> @@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev) >> chip_id = doc_register_readw(docg3, DOC_CHIPID); >> chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV); >> >> - ret = 0; >> + ret = -ENODEV; >> if (chip_id != (u16)(~chip_id_inv)) { >> goto nomem4; >> } >> @@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev) >> mtd = doc_probe_device(cascade, floor, dev); >> if (IS_ERR(mtd)) { >> ret = PTR_ERR(mtd); >> - goto err_probe; >> - } >> - if (!mtd) { >> - if (floor == 0) >> - goto notfound; >> - else >> + if (ret == -ENODEV && floor == 0) > > I think you might have changed the logic when refactoring here. I think > the right refactoring would be the following, no? > > if (ret == -ENODEV && floor != 0) > continue; > else > goto err_probe; > You are right, the logic changed. ;-\ Please drop this patch, I'll resend it for 4.3. Thanks, //richard
Richard Weinberger <richard@nod.at> writes: > Am 17.06.2015 um 20:41 schrieb Brian Norris: >> On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote: >>> Don't return a obfuscated null pointer using ERR_PTR(0). >>> If the no device is found clearly return -ENODEV. >>> This makes the code more clear and matches the comment >>> of doc_probe_device(). >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >> >> Have you tested this patch? > > nah, I don't own such a device. But I do. If you resend a patch, please Cc me. You can even ask for a test from time to time if you want a confirmation, I have a 2 floors docg3 device. Cheers. -- Robert
Hi Robert, On Tue, Jun 23, 2015 at 10:41:33PM +0200, Robert Jarzmik wrote: > Richard Weinberger <richard@nod.at> writes: > > > Am 17.06.2015 um 20:41 schrieb Brian Norris: > >> Have you tested this patch? > > > > nah, I don't own such a device. > But I do. If you resend a patch, please Cc me. You can even ask for a test from > time to time if you want a confirmation, I have a 2 floors docg3 device. Do you want to be on a MAINTAINERS entry for this driver? Brian
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c index 5e67b4a..630e29a 100644 --- a/drivers/mtd/devices/docg3.c +++ b/drivers/mtd/devices/docg3.c @@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev) chip_id = doc_register_readw(docg3, DOC_CHIPID); chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV); - ret = 0; + ret = -ENODEV; if (chip_id != (u16)(~chip_id_inv)) { goto nomem4; } @@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev) mtd = doc_probe_device(cascade, floor, dev); if (IS_ERR(mtd)) { ret = PTR_ERR(mtd); - goto err_probe; - } - if (!mtd) { - if (floor == 0) - goto notfound; - else + if (ret == -ENODEV && floor == 0) continue; + else + goto err_probe; } cascade->floors[floor] = mtd; ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL, @@ -2091,10 +2088,9 @@ static int __init docg3_probe(struct platform_device *pdev) doc_dbg_register(cascade->floors[0]->priv); return 0; -notfound: - ret = -ENODEV; - dev_info(dev, "No supported DiskOnChip found\n"); err_probe: + if (ret == -ENODEV) + dev_info(dev, "No supported DiskOnChip found\n"); free_bch(cascade->bch); for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) if (cascade->floors[floor])
Don't return a obfuscated null pointer using ERR_PTR(0). If the no device is found clearly return -ENODEV. This makes the code more clear and matches the comment of doc_probe_device(). Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/devices/docg3.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)