| Message ID | 1346517191-8794-4-git-send-email-Julia.Lawall@lip6.fr |
|---|---|
| State | New, archived |
| Headers | show |
Aiaiai! :-) [1] [2] I've build-tested this using aiaiai and it reports that this change breaks the build: dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox Tested the patch(es) on top of the following commits: ba64756 Quick fixes - applied by aiaiai 651c6fa JFFS2: don't fail on bitflips in OOB e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data ea9d312 mtd: cmdlinepart: minor cleanups -------------------------------------------------------------------------------- Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)": 0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups ... drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe': drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable] drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable] drivers/built-in.o: In function `mpc5121_nfc_probe': mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get' make[1]: *** [vmlinux] Error 1 -------------------------------------------------------------------------------- I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people. So, not taking this patch. References: 1. http://git.infradead.org/users/dedekind/aiaiai.git 2. http://git.infradead.org/users/dedekind/maintaining.git On Sat, 2012-09-01 at 18:33 +0200, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > devm free functions should not have to be explicitly used. > > The only thing left that is useful in the function mpc5121_nfc_free is the > call to clk_disable, which is moved to the call sites. > > This function also incorrectly called iounmap on devm_ioremap allocated > data. > > Use devm_request_and_ioremap in place of devm_request_mem_region and > devm_ioremap. > > Use devm_clk_get. > > A semantic match that finds the first problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > @@ > > ( > * devm_kfree(...); > | > * devm_free_irq(...); > | > * devm_iounmap(...); > | > * devm_release_region(...); > | > * devm_release_mem_region(...); > ) > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > drivers/mtd/nand/mpc5121_nfc.c | 35 +++++------------------------------ > 1 file changed, 5 insertions(+), 30 deletions(-) > > diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c > index c259c24..45183ba 100644 > --- a/drivers/mtd/nand/mpc5121_nfc.c > +++ b/drivers/mtd/nand/mpc5121_nfc.c > @@ -632,21 +632,6 @@ out: > return ret; > } > > -/* Free driver resources */ > -static void mpc5121_nfc_free(struct device *dev, struct mtd_info *mtd) > -{ > - struct nand_chip *chip = mtd->priv; > - struct mpc5121_nfc_prv *prv = chip->priv; > - > - if (prv->clk) { > - clk_disable(prv->clk); > - clk_put(prv->clk); > - } > - > - if (prv->csreg) > - iounmap(prv->csreg); > -} > - > static int __devinit mpc5121_nfc_probe(struct platform_device *op) > { > struct device_node *rootnode, *dn = op->dev.of_node; > @@ -713,12 +698,7 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) > regs_paddr = res.start; > regs_size = resource_size(&res); > > - if (!devm_request_mem_region(dev, regs_paddr, regs_size, DRV_NAME)) { > - dev_err(dev, "Error requesting memory region!\n"); > - return -EBUSY; > - } > - > - prv->regs = devm_ioremap(dev, regs_paddr, regs_size); > + prv->regs = devm_request_and_ioremap(dev, &res); > if (!prv->regs) { > dev_err(dev, "Error mapping memory region!\n"); > return -ENOMEM; > @@ -752,11 +732,10 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) > of_node_put(rootnode); > > /* Enable NFC clock */ > - prv->clk = clk_get(dev, "nfc_clk"); > + prv->clk = devm_clk_get(dev, "nfc_clk"); > if (IS_ERR(prv->clk)) { > dev_err(dev, "Unable to acquire NFC clock!\n"); > - retval = PTR_ERR(prv->clk); > - goto error; > + return PTR_ERR(prv->clk); > } > > clk_enable(prv->clk); > @@ -803,7 +782,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) > /* Detect NAND chips */ > if (nand_scan(mtd, be32_to_cpup(chips_no))) { > dev_err(dev, "NAND Flash not found !\n"); > - devm_free_irq(dev, prv->irq, mtd); > retval = -ENXIO; > goto error; > } > @@ -828,7 +806,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) > > default: > dev_err(dev, "Unsupported NAND flash!\n"); > - devm_free_irq(dev, prv->irq, mtd); > retval = -ENXIO; > goto error; > } > @@ -839,13 +816,12 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) > retval = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); > if (retval) { > dev_err(dev, "Error adding MTD device!\n"); > - devm_free_irq(dev, prv->irq, mtd); > goto error; > } > > return 0; > error: > - mpc5121_nfc_free(dev, mtd); > + clk_disable(prv->clk); > return retval; > } > > @@ -857,8 +833,7 @@ static int __devexit mpc5121_nfc_remove(struct platform_device *op) > struct mpc5121_nfc_prv *prv = chip->priv; > > nand_release(mtd); > - devm_free_irq(dev, prv->irq, mtd); > - mpc5121_nfc_free(dev, mtd); > + clk_disable(prv->clk); > > return 0; > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 09/04/2012 10:42 AM, Artem Bityutskiy wrote: > Aiaiai! :-) [1] [2] > > I've build-tested this using aiaiai and it reports that this change breaks the build: > > dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox > Tested the patch(es) on top of the following commits: > ba64756 Quick fixes - applied by aiaiai > 651c6fa JFFS2: don't fail on bitflips in OOB > e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data > ea9d312 mtd: cmdlinepart: minor cleanups > > -------------------------------------------------------------------------------- > Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)": > > 0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups > > ... > drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe': > drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable] > drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable] > drivers/built-in.o: In function `mpc5121_nfc_probe': > mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get' > make[1]: *** [vmlinux] Error 1 > > -------------------------------------------------------------------------------- > > I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people. > I've been bitten by the same issue recently, also cause by one of these cocci devm patches. devm_clk_get is only available if the generic clk_get/clk_put implementation is used. Not all architectures do this and some implement their own clk_get/clk_put, etc functions. Since devm_clk_get is merely a wrapper around clk_get/clk_put there is no reason why it should depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set), but it is on a different machine right now, will try to submit it later today. - Lars
On Tue, 4 Sep 2012, Lars-Peter Clausen wrote: > On 09/04/2012 10:42 AM, Artem Bityutskiy wrote: > > Aiaiai! :-) [1] [2] > > > > I've build-tested this using aiaiai and it reports that this change breaks the build: > > > > dedekind@blue:~/git/maintaining$ ./verify ../l2-mtd/ mpc5121_nfc < ~/tmp/julia2.mbox > > Tested the patch(es) on top of the following commits: > > ba64756 Quick fixes - applied by aiaiai > > 651c6fa JFFS2: don't fail on bitflips in OOB > > e22ac84 mtd: autcpu12-nvram: drop frees of devm_ alloc'd data > > ea9d312 mtd: cmdlinepart: minor cleanups > > > > -------------------------------------------------------------------------------- > > Failed to build the following commit for configuration "powerpc-mpc512x_defconfig" (architecture powerpc)": > > > > 0fe13ab drivers/mtd/nand/mpc5121_nfc.c: some devm_ cleanups > > > > ... > > drivers/mtd/nand/mpc5121_nfc.c: In function 'mpc5121_nfc_probe': > > drivers/mtd/nand/mpc5121_nfc.c:622:28: warning: variable 'regs_size' set but not used [-Wunused-but-set-variable] > > drivers/mtd/nand/mpc5121_nfc.c:622:16: warning: variable 'regs_paddr' set but not used [-Wunused-but-set-variable] > > drivers/built-in.o: In function `mpc5121_nfc_probe': > > mpc5121_nfc.c:(.devinit.text+0x2a14): undefined reference to `devm_clk_get' > > make[1]: *** [vmlinux] Error 1 > > > > -------------------------------------------------------------------------------- > > > > I do not really know why, but it seems that clock framework is not supported for powerpc. CCing the PPC mailing list. Preserved the context below for the PPC people. > > > > I've been bitten by the same issue recently, also cause by one of these > cocci devm patches. devm_clk_get is only available if the generic > clk_get/clk_put implementation is used. Not all architectures do this and > some implement their own clk_get/clk_put, etc functions. Since devm_clk_get > is merely a wrapper around clk_get/clk_put there is no reason why it should > depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically > available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set), > but it is on a different machine right now, will try to submit it later today. Sorry about this. I wasn't aware that devm_clk_get wasn't supported by all architectures, and I have no way of compiling code for these architectures... But I wonder why it is not, since devm-ness doesn't seem to have anything to do with architecture-specific details? It would be really nice to have it for all architectures, because the clock functions are just as (or at least almost as) common as kzalloc, ioremap, etc. thanks, julia
On Tue, 2012-09-04 at 11:44 +0200, Julia Lawall wrote: > > I've been bitten by the same issue recently, also cause by one of these > > cocci devm patches. devm_clk_get is only available if the generic > > clk_get/clk_put implementation is used. Not all architectures do this and > > some implement their own clk_get/clk_put, etc functions. Since devm_clk_get > > is merely a wrapper around clk_get/clk_put there is no reason why it should > > depend CLKDEV_LOOKUP. I've prepared a patch which makes them generically > > available if the clk_get/clk_put are implemented (i.e. if HAVE_CLK is set), > > but it is on a different machine right now, will try to submit it later today. > > Sorry about this. I wasn't aware that devm_clk_get wasn't supported by > all architectures, and I have no way of compiling code for these > architectures... But I wonder why it is not, since devm-ness doesn't seem > to have anything to do with architecture-specific details? It would be > really nice to have it for all architectures, because the clock functions > are just as (or at least almost as) common as kzalloc, ioremap, etc. It looks like Lars is going to fix this. I am personally fine if you send patches without build-testing them. Your patches are generally of good quality and you send many of them, so build-testing each would be too much for you. And at least for MTD, I can build-test myself.
diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c index c259c24..45183ba 100644 --- a/drivers/mtd/nand/mpc5121_nfc.c +++ b/drivers/mtd/nand/mpc5121_nfc.c @@ -632,21 +632,6 @@ out: return ret; } -/* Free driver resources */ -static void mpc5121_nfc_free(struct device *dev, struct mtd_info *mtd) -{ - struct nand_chip *chip = mtd->priv; - struct mpc5121_nfc_prv *prv = chip->priv; - - if (prv->clk) { - clk_disable(prv->clk); - clk_put(prv->clk); - } - - if (prv->csreg) - iounmap(prv->csreg); -} - static int __devinit mpc5121_nfc_probe(struct platform_device *op) { struct device_node *rootnode, *dn = op->dev.of_node; @@ -713,12 +698,7 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) regs_paddr = res.start; regs_size = resource_size(&res); - if (!devm_request_mem_region(dev, regs_paddr, regs_size, DRV_NAME)) { - dev_err(dev, "Error requesting memory region!\n"); - return -EBUSY; - } - - prv->regs = devm_ioremap(dev, regs_paddr, regs_size); + prv->regs = devm_request_and_ioremap(dev, &res); if (!prv->regs) { dev_err(dev, "Error mapping memory region!\n"); return -ENOMEM; @@ -752,11 +732,10 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) of_node_put(rootnode); /* Enable NFC clock */ - prv->clk = clk_get(dev, "nfc_clk"); + prv->clk = devm_clk_get(dev, "nfc_clk"); if (IS_ERR(prv->clk)) { dev_err(dev, "Unable to acquire NFC clock!\n"); - retval = PTR_ERR(prv->clk); - goto error; + return PTR_ERR(prv->clk); } clk_enable(prv->clk); @@ -803,7 +782,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) /* Detect NAND chips */ if (nand_scan(mtd, be32_to_cpup(chips_no))) { dev_err(dev, "NAND Flash not found !\n"); - devm_free_irq(dev, prv->irq, mtd); retval = -ENXIO; goto error; } @@ -828,7 +806,6 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) default: dev_err(dev, "Unsupported NAND flash!\n"); - devm_free_irq(dev, prv->irq, mtd); retval = -ENXIO; goto error; } @@ -839,13 +816,12 @@ static int __devinit mpc5121_nfc_probe(struct platform_device *op) retval = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); if (retval) { dev_err(dev, "Error adding MTD device!\n"); - devm_free_irq(dev, prv->irq, mtd); goto error; } return 0; error: - mpc5121_nfc_free(dev, mtd); + clk_disable(prv->clk); return retval; } @@ -857,8 +833,7 @@ static int __devexit mpc5121_nfc_remove(struct platform_device *op) struct mpc5121_nfc_prv *prv = chip->priv; nand_release(mtd); - devm_free_irq(dev, prv->irq, mtd); - mpc5121_nfc_free(dev, mtd); + clk_disable(prv->clk); return 0; }