* [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap @ 2012-01-12 9:55 Julia Lawall 2012-01-12 10:45 ` Felipe Balbi 2012-01-13 6:25 ` Dong Aisheng-B29396 0 siblings, 2 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-12 9:55 UTC (permalink / raw) To: Liam Girdwood Cc: kernel-janitors, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel From: Julia Lawall <Julia.Lawall@lip6.fr> Add missing iounmap in error handling code, in a case where the function already preforms iounmap on some other execution path. A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ expression e; statement S,S1; int ret; @@ e = \(ioremap\|ioremap_nocache\)(...) ... when != iounmap(e) if (<+...e...+>) S ... when any when != iounmap(e) *if (...) { ... when != iounmap(e) return ...; } ... when any iounmap(e); // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- sound/soc/mxs/mxs-saif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index 049e543..5ee0adb 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start; @@ -739,6 +739,7 @@ failed_register: failed_get_irq2: free_irq(saif->irq, saif); failed_get_irq1: +failed_get_resource: iounmap(saif->base); failed_ioremap: release_mem_region(iores->start, resource_size(iores)); ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-12 9:55 [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall @ 2012-01-12 10:45 ` Felipe Balbi 2012-01-13 6:25 ` Dong Aisheng-B29396 1 sibling, 0 replies; 31+ messages in thread From: Felipe Balbi @ 2012-01-12 10:45 UTC (permalink / raw) To: Julia Lawall Cc: Liam Girdwood, kernel-janitors, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1224 bytes --] Hi, On Thu, Jan 12, 2012 at 10:55:03AM +0100, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Add missing iounmap in error handling code, in a case where the function > already preforms iounmap on some other execution path. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression e; > statement S,S1; > int ret; > @@ > e = \(ioremap\|ioremap_nocache\)(...) > ... when != iounmap(e) > if (<+...e...+>) S > ... when any > when != iounmap(e) > *if (...) > { ... when != iounmap(e) > return ...; } > ... when any > iounmap(e); > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> This is not related to this patch actually, but when you send a series of patches could you make sure to send them all as a thread ? It makes it easier (at least for me) to see that those are all related to each other (in your case, sematich patches). In order to do that with git you could: $ git format-patch -o patches --cover-letter linus/master [ edit cover letter] $ git send-email --to lkml patches/ that will do the trick. Thanks -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-12 9:55 [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 2012-01-12 10:45 ` Felipe Balbi @ 2012-01-13 6:25 ` Dong Aisheng-B29396 2012-01-13 6:32 ` Julia Lawall 2012-01-24 17:29 ` Julia Lawall 1 sibling, 2 replies; 31+ messages in thread From: Dong Aisheng-B29396 @ 2012-01-13 6:25 UTC (permalink / raw) To: Julia Lawall, Liam Girdwood Cc: kernel-janitors, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel > -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Julia Lawall > Sent: Thursday, January 12, 2012 5:55 PM > To: Liam Girdwood > Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai; > alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org > Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap > > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Add missing iounmap in error handling code, in a case where the function already > preforms iounmap on some other execution path. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression e; > statement S,S1; > int ret; > @@ > e = \(ioremap\|ioremap_nocache\)(...) > ... when != iounmap(e) > if (<+...e...+>) S > ... when any > when != iounmap(e) > *if (...) > { ... when != iounmap(e) > return ...; } > ... when any > iounmap(e); > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > sound/soc/mxs/mxs-saif.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index > 049e543..5ee0adb 100644 > --- a/sound/soc/mxs/mxs-saif.c > +++ b/sound/soc/mxs/mxs-saif.c > @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) > ret = -ENODEV; > dev_err(&pdev->dev, "failed to get dma resource: %d\n", > ret); > - goto failed_ioremap; > + goto failed_get_resource; > } > saif->dma_param.chan_num = dmares->start; > > @@ -739,6 +739,7 @@ failed_register: > failed_get_irq2: > free_irq(saif->irq, saif); > failed_get_irq1: > +failed_get_resource: There's already a 'failed_get_resource' there, wouldn't your change break The original code? BTW, I guess a better way is that you can submit a patch to change the driver To use devm_alloc_* and it's friend routines, then we do not need to fix Such things any more. Regards Dong Aisheng > iounmap(saif->base); > failed_ioremap: > release_mem_region(iores->start, resource_size(iores)); > > -- > 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-13 6:25 ` Dong Aisheng-B29396 @ 2012-01-13 6:32 ` Julia Lawall 2012-01-24 17:29 ` Julia Lawall 1 sibling, 0 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-13 6:32 UTC (permalink / raw) To: Dong Aisheng-B29396 Cc: Julia Lawall, Liam Girdwood, kernel-janitors, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel On Fri, 13 Jan 2012, Dong Aisheng-B29396 wrote: >> -----Original Message----- >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- >> owner@vger.kernel.org] On Behalf Of Julia Lawall >> Sent: Thursday, January 12, 2012 5:55 PM >> To: Liam Girdwood >> Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai; >> alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org >> Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap >> >> From: Julia Lawall <Julia.Lawall@lip6.fr> >> >> Add missing iounmap in error handling code, in a case where the function already >> preforms iounmap on some other execution path. >> >> A simplified version of the semantic match that finds this problem is as >> follows: (http://coccinelle.lip6.fr/) >> >> // <smpl> >> @@ >> expression e; >> statement S,S1; >> int ret; >> @@ >> e = \(ioremap\|ioremap_nocache\)(...) >> ... when != iounmap(e) >> if (<+...e...+>) S >> ... when any >> when != iounmap(e) >> *if (...) >> { ... when != iounmap(e) >> return ...; } >> ... when any >> iounmap(e); >> // </smpl> >> >> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >> >> --- >> sound/soc/mxs/mxs-saif.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index >> 049e543..5ee0adb 100644 >> --- a/sound/soc/mxs/mxs-saif.c >> +++ b/sound/soc/mxs/mxs-saif.c >> @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) >> ret = -ENODEV; >> dev_err(&pdev->dev, "failed to get dma resource: %d\n", >> ret); >> - goto failed_ioremap; >> + goto failed_get_resource; >> } >> saif->dma_param.chan_num = dmares->start; >> >> @@ -739,6 +739,7 @@ failed_register: >> failed_get_irq2: >> free_irq(saif->irq, saif); >> failed_get_irq1: >> +failed_get_resource: > There's already a 'failed_get_resource' there, wouldn't your change break > The original code? > > BTW, I guess a better way is that you can submit a patch to change the driver > To use devm_alloc_* and it's friend routines, then we do not need to fix > Such things any more. Thanks for the suggestions. I will fix it up with the devm_ solution. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-13 6:25 ` Dong Aisheng-B29396 2012-01-13 6:32 ` Julia Lawall @ 2012-01-24 17:29 ` Julia Lawall 2012-01-24 20:22 ` Mark Brown 1 sibling, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-01-24 17:29 UTC (permalink / raw) To: Dong Aisheng-B29396 Cc: Julia Lawall, Liam Girdwood, kernel-janitors, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel From: Julia Lawall <Julia.Lawall@lip6.fr> The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function. By removing the need for iounmap, this also eliminates a missing iounmap problem, in the the original error handling code following a successful call to ioremap. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- sound/soc/mxs/mxs-saif.c | 44 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index f204dba..6a72ad9 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -630,7 +630,7 @@ static int mxs_saif_probe(struct platform_device *pdev) if (pdev->id >= ARRAY_SIZE(mxs_saif)) return -EINVAL; - saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM; @@ -652,10 +652,9 @@ static int mxs_saif_probe(struct platform_device *pdev) saif->clk = clk_get(&pdev->dev, NULL); if (IS_ERR(saif->clk)) { - ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return PTR_ERR(saif->clk); } iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -666,18 +665,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; } - if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { + if (!devm_request_mem_region(&pdev->dev, iores->start, + resource_size(iores), "mxs-saif")) { dev_err(&pdev->dev, "request_mem_region failed\n"); ret = -EBUSY; goto failed_get_resource; } - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_ioremap(&pdev->dev, iores->start, + resource_size(iores)); if (!saif->base) { dev_err(&pdev->dev, "ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; } dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -685,7 +685,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start; @@ -694,14 +694,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; } saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; } saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -709,7 +710,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; } platform_set_drvdata(pdev, saif); @@ -717,7 +718,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; } saif->soc_platform_pdev = platform_device_alloc( @@ -740,36 +741,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif); return ret; } static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev); platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif); return 0; } ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-24 17:29 ` Julia Lawall @ 2012-01-24 20:22 ` Mark Brown 2012-01-24 20:36 ` Julia Lawall 2012-01-24 20:45 ` Julia Lawall 0 siblings, 2 replies; 31+ messages in thread From: Mark Brown @ 2012-01-24 20:22 UTC (permalink / raw) To: Julia Lawall Cc: Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 374 bytes --] On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote: > if (IS_ERR(saif->clk)) { > - ret = PTR_ERR(saif->clk); > dev_err(&pdev->dev, "Cannot get the clock: %d\n", > ret); > - goto failed_clk; > + return PTR_ERR(saif->clk); Looks like the transformation is overly enthusiastic here - the assignment to ret is removed but ret is used in the log message. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-24 20:22 ` Mark Brown @ 2012-01-24 20:36 ` Julia Lawall 2012-01-24 20:45 ` Julia Lawall 1 sibling, 0 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-24 20:36 UTC (permalink / raw) To: Mark Brown Cc: Julia Lawall, Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel On Tue, 24 Jan 2012, Mark Brown wrote: > On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote: > >> if (IS_ERR(saif->clk)) { >> - ret = PTR_ERR(saif->clk); >> dev_err(&pdev->dev, "Cannot get the clock: %d\n", >> ret); >> - goto failed_clk; >> + return PTR_ERR(saif->clk); > > Looks like the transformation is overly enthusiastic here - the > assignment to ret is removed but ret is used in the log message. Uh, no, I did that stupid thing by hand. I'll fix it. Thanks. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-24 20:22 ` Mark Brown 2012-01-24 20:36 ` Julia Lawall @ 2012-01-24 20:45 ` Julia Lawall 2012-01-26 10:53 ` Mark Brown 2012-01-26 11:05 ` [alsa-devel] " Wolfram Sang 1 sibling, 2 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-24 20:45 UTC (permalink / raw) To: Mark Brown Cc: Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel >From nobody Tue Jan 24 18:26:10 CET 2012 From: Julia Lawall <Julia.Lawall@lip6.fr> To: Liam Girdwood <lrg@ti.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>,Jaroslav Kysela <perex@perex.cz>,Takashi Iwai <tiwai@suse.de>,alsa-devel@alsa-project.org,linux-kernel@vger.kernel.org Subject: [PATCH] sound/soc/mxs/mxs-saif.c: use devm_ functions From: Julia Lawall <Julia.Lawall@lip6.fr> The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function. By removing the need for iounmap, this also eliminates a missing iounmap problem, in the the original error handling code following a successful call to ioremap. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- sound/soc/mxs/mxs-saif.c | 43 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index f204dba..7d755e0 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -630,7 +630,7 @@ static int mxs_saif_probe(struct platform_device *pdev) if (pdev->id >= ARRAY_SIZE(mxs_saif)) return -EINVAL; - saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM; @@ -655,7 +655,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return ret; } iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; } - if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { + if (!devm_request_mem_region(&pdev->dev, iores->start, + resource_size(iores), "mxs-saif")) { dev_err(&pdev->dev, "request_mem_region failed\n"); ret = -EBUSY; goto failed_get_resource; } - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_ioremap(&pdev->dev, iores->start, + resource_size(iores)); if (!saif->base) { dev_err(&pdev->dev, "ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; } dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -685,7 +686,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start; @@ -694,14 +695,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; } saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; } saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -709,7 +711,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; } platform_set_drvdata(pdev, saif); @@ -717,7 +719,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; } saif->soc_platform_pdev = platform_device_alloc( @@ -740,36 +742,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif); return ret; } static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev); platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif); return 0; } ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-24 20:45 ` Julia Lawall @ 2012-01-26 10:53 ` Mark Brown 2012-01-26 11:24 ` Julia Lawall 2012-01-26 11:05 ` [alsa-devel] " Wolfram Sang 1 sibling, 1 reply; 31+ messages in thread From: Mark Brown @ 2012-01-26 10:53 UTC (permalink / raw) To: Julia Lawall Cc: Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 517 bytes --] On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote: > The various devm_ functions allocate memory that is released when a driver > detaches. This patch uses these functions for data that is allocated in > the probe function of a platform device and is only freed in the remove > function. This is a really nice cleanup - I tried to apply it but unfortunately it needs a rebase due to the change to use clk_prepare()/clk_unprepare() which went in recently. Could you rebase against current -next please? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 10:53 ` Mark Brown @ 2012-01-26 11:24 ` Julia Lawall 2012-01-26 11:27 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-01-26 11:24 UTC (permalink / raw) To: Mark Brown Cc: Julia Lawall, Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel On Thu, 26 Jan 2012, Mark Brown wrote: > On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote: > >> The various devm_ functions allocate memory that is released when a driver >> detaches. This patch uses these functions for data that is allocated in >> the probe function of a platform device and is only freed in the remove >> function. > > This is a really nice cleanup - I tried to apply it but unfortunately it > needs a rebase due to the change to use clk_prepare()/clk_unprepare() > which went in recently. Could you rebase against current -next please? Could you tell me in more detail what the problem is? I pulled linux-next and regenerated the patch, but got exactly the same thing as before. I do see some calls to clk_prepare_enable in the code. thanks, julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:24 ` Julia Lawall @ 2012-01-26 11:27 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2012-01-26 11:27 UTC (permalink / raw) To: Julia Lawall Cc: Dong Aisheng-B29396, Liam Girdwood, kernel-janitors, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 797 bytes --] On Thu, Jan 26, 2012 at 12:24:21PM +0100, Julia Lawall wrote: > On Thu, 26 Jan 2012, Mark Brown wrote: > >This is a really nice cleanup - I tried to apply it but unfortunately it > >needs a rebase due to the change to use clk_prepare()/clk_unprepare() > >which went in recently. Could you rebase against current -next please? > Could you tell me in more detail what the problem is? I pulled > linux-next and regenerated the patch, but got exactly the same thing > as before. I do see some calls to clk_prepare_enable in the code. I tried to apply it with git am but even with -3 it wouldn't apply (-3 said it didn't know about the original blobs which was worrying), looking at the logs the clk_prepare stuff seemed most likely to have overlapped. Perhaps your regeneration will be enough. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-24 20:45 ` Julia Lawall 2012-01-26 10:53 ` Mark Brown @ 2012-01-26 11:05 ` Wolfram Sang 2012-01-26 11:21 ` Julia Lawall 2012-01-26 11:57 ` walter harms 1 sibling, 2 replies; 31+ messages in thread From: Wolfram Sang @ 2012-01-26 11:05 UTC (permalink / raw) To: Julia Lawall Cc: Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 815 bytes --] > @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) > goto failed_get_resource; > } > > - if (!request_mem_region(iores->start, resource_size(iores), > - "mxs-saif")) { > + if (!devm_request_mem_region(&pdev->dev, iores->start, > + resource_size(iores), "mxs-saif")) { > dev_err(&pdev->dev, "request_mem_region failed\n"); > ret = -EBUSY; > goto failed_get_resource; > } > > - saif->base = ioremap(iores->start, resource_size(iores)); > + saif->base = devm_ioremap(&pdev->dev, iores->start, > + resource_size(iores)); devm_request_and_ioremap() to save even more code? -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:05 ` [alsa-devel] " Wolfram Sang @ 2012-01-26 11:21 ` Julia Lawall 2012-01-26 11:22 ` Mark Brown 2012-01-26 11:25 ` Wolfram Sang 2012-01-26 11:57 ` walter harms 1 sibling, 2 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-26 11:21 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood On Thu, 26 Jan 2012, Wolfram Sang wrote: >> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) >> goto failed_get_resource; >> } >> >> - if (!request_mem_region(iores->start, resource_size(iores), >> - "mxs-saif")) { >> + if (!devm_request_mem_region(&pdev->dev, iores->start, >> + resource_size(iores), "mxs-saif")) { >> dev_err(&pdev->dev, "request_mem_region failed\n"); >> ret = -EBUSY; >> goto failed_get_resource; >> } >> >> - saif->base = ioremap(iores->start, resource_size(iores)); >> + saif->base = devm_ioremap(&pdev->dev, iores->start, >> + resource_size(iores)); > > devm_request_and_ioremap() to save even more code? I didn't do that because apparently it is not yet in a release? I have another for introducing those... julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:21 ` Julia Lawall @ 2012-01-26 11:22 ` Mark Brown 2012-01-26 13:40 ` Julia Lawall 2012-01-28 7:03 ` [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 2012-01-26 11:25 ` Wolfram Sang 1 sibling, 2 replies; 31+ messages in thread From: Mark Brown @ 2012-01-26 11:22 UTC (permalink / raw) To: Julia Lawall Cc: Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 352 bytes --] On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote: > On Thu, 26 Jan 2012, Wolfram Sang wrote: > >devm_request_and_ioremap() to save even more code? > I didn't do that because apparently it is not yet in a release? I > have another for introducing those... It went in during the merge window so patches using it should be good to go now. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:22 ` Mark Brown @ 2012-01-26 13:40 ` Julia Lawall 2012-01-26 13:51 ` Wolfram Sang 2012-01-28 7:03 ` [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 1 sibling, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-01-26 13:40 UTC (permalink / raw) To: Mark Brown Cc: Julia Lawall, Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood From: Julia Lawall <Julia.Lawall@lip6.fr> The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function. By removing the need for kfree and iounmap, this also eliminates missing resource-release problems. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- This is a patch against the version in git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git sound/soc/mxs/mxs-saif.c | 47 ++++++++++++----------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-) diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev) return ret; } - saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM; @@ -652,7 +652,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return ret; } iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; } - if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - ret = -EBUSY; - goto failed_get_resource; - } - - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_request_and_ioremap(&pdev->dev, iores); if (!saif->base) { - dev_err(&pdev->dev, "ioremap failed\n"); + dev_err(&pdev->dev, "devm_request_and_ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; } dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -682,7 +675,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start; @@ -691,14 +684,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; } saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; } saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -706,7 +700,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; } platform_set_drvdata(pdev, saif); @@ -714,7 +708,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; } saif->soc_platform_pdev = platform_device_alloc( @@ -737,36 +731,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif); return ret; } static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev); platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif); return 0; } ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 13:40 ` Julia Lawall @ 2012-01-26 13:51 ` Wolfram Sang 2012-01-26 14:07 ` Julia Lawall 2012-02-04 18:25 ` devm_request_and_ioremap Julia Lawall 0 siblings, 2 replies; 31+ messages in thread From: Wolfram Sang @ 2012-01-26 13:51 UTC (permalink / raw) To: Julia Lawall Cc: Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 945 bytes --] Julia, > iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) > goto failed_get_resource; > } > > - if (!request_mem_region(iores->start, resource_size(iores), > - "mxs-saif")) { > - dev_err(&pdev->dev, "request_mem_region failed\n"); > - ret = -EBUSY; > - goto failed_get_resource; > - } > - > - saif->base = ioremap(iores->start, resource_size(iores)); > + saif->base = devm_request_and_ioremap(&pdev->dev, iores); You can skip checking 'iores', too. I also did that in the example, but a lot of people seem to miss it. Where did you get the information how to use devm_request_and_ioremap? I probably need to spread the word even more... Thanks, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 13:51 ` Wolfram Sang @ 2012-01-26 14:07 ` Julia Lawall 2012-01-28 8:26 ` Wolfram Sang 2012-02-04 18:25 ` devm_request_and_ioremap Julia Lawall 1 sibling, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-01-26 14:07 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood On Thu, 26 Jan 2012, Wolfram Sang wrote: > Julia, > >> iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) >> goto failed_get_resource; >> } >> >> - if (!request_mem_region(iores->start, resource_size(iores), >> - "mxs-saif")) { >> - dev_err(&pdev->dev, "request_mem_region failed\n"); >> - ret = -EBUSY; >> - goto failed_get_resource; >> - } >> - >> - saif->base = ioremap(iores->start, resource_size(iores)); >> + saif->base = devm_request_and_ioremap(&pdev->dev, iores); > > You can skip checking 'iores', too. I also did that in the example, but > a lot of people seem to miss it. I can try to do that, but it seems a little bit unintuitive. Perhaps it would be easier for people to remember to put in error handling code when they need it if they always have to do it? If I remove it, there will be one call that has no test and then another call a few lines later that does. > Where did you get the information how > to use devm_request_and_ioremap? I probably need to spread the word even > more... I saw it in Documentation/driver-model/devres.txt and then looked around for some examples. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 14:07 ` Julia Lawall @ 2012-01-28 8:26 ` Wolfram Sang 2012-01-28 8:51 ` Julia Lawall 0 siblings, 1 reply; 31+ messages in thread From: Wolfram Sang @ 2012-01-28 8:26 UTC (permalink / raw) To: Julia Lawall Cc: Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 936 bytes --] > >You can skip checking 'iores', too. I also did that in the example, but > >a lot of people seem to miss it. > > I can try to do that, but it seems a little bit unintuitive. > Perhaps it would be easier for people to remember to put in error > handling code when they need it if they always have to do it? If I > remove it, there will be one call that has no test and then another > call a few lines later that does. I see your point. I would still like to get rid of the duplicated code (then it can't be forgotten as well). Maybe I should have named the function something alike devm_check_and_request_and_ioremap()? Then I could have also introduced a similar function for requesting irq. Will think about this a bit more. Thanks for updating your patch! -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-28 8:26 ` Wolfram Sang @ 2012-01-28 8:51 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-28 8:51 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood On Sat, 28 Jan 2012, Wolfram Sang wrote: >>> You can skip checking 'iores', too. I also did that in the example, but >>> a lot of people seem to miss it. >> >> I can try to do that, but it seems a little bit unintuitive. >> Perhaps it would be easier for people to remember to put in error >> handling code when they need it if they always have to do it? If I >> remove it, there will be one call that has no test and then another >> call a few lines later that does. > > I see your point. I would still like to get rid of the duplicated code > (then it can't be forgotten as well). Maybe I should have named the > function something alike devm_check_and_request_and_ioremap()? Then I > could have also introduced a similar function for requesting irq. Will > think about this a bit more. Thanks for updating your patch! Despite believing in my point, error handling code is where people make a lot of mistakes. Getting rid of it seems always to be a good idea. Maybe such checks could be moved into more uses of the result of platform_get_resource. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* devm_request_and_ioremap 2012-01-26 13:51 ` Wolfram Sang 2012-01-26 14:07 ` Julia Lawall @ 2012-02-04 18:25 ` Julia Lawall 2012-02-04 18:46 ` devm_request_and_ioremap Mark Brown 1 sibling, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-02-04 18:25 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood How important is the name argument to request_mem_region? devm_request_and_ioremap uses: name = res->name ?: dev_name(dev); In trying to introduce uses of devm_request_and_ioremap I thus look for res->name and dev_name(dev) as well as the name in the driver structure in the platform_driver structure. But sometimes the name is pdev->name, ie the name field of the argument to the probe function. And sometimes it is a string that is slightly different than what is in the driver structure. thanks, julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: devm_request_and_ioremap 2012-02-04 18:25 ` devm_request_and_ioremap Julia Lawall @ 2012-02-04 18:46 ` Mark Brown 2012-02-04 22:08 ` devm_request_and_ioremap Julia Lawall 0 siblings, 1 reply; 31+ messages in thread From: Mark Brown @ 2012-02-04 18:46 UTC (permalink / raw) To: Julia Lawall Cc: Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 155 bytes --] On Sat, Feb 04, 2012 at 07:25:05PM +0100, Julia Lawall wrote: > How important is the name argument to request_mem_region? It's just used for diagnostics. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: devm_request_and_ioremap 2012-02-04 18:46 ` devm_request_and_ioremap Mark Brown @ 2012-02-04 22:08 ` Julia Lawall 2012-02-06 16:21 ` devm_request_and_ioremap Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Julia Lawall @ 2012-02-04 22:08 UTC (permalink / raw) To: Mark Brown Cc: Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood And how about a function devm_clk_get? It looks like there would be around 150 opportunities for its use. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: devm_request_and_ioremap 2012-02-04 22:08 ` devm_request_and_ioremap Julia Lawall @ 2012-02-06 16:21 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2012-02-06 16:21 UTC (permalink / raw) To: Julia Lawall Cc: Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 289 bytes --] On Sat, Feb 04, 2012 at 11:08:24PM +0100, Julia Lawall wrote: > And how about a function devm_clk_get? It looks like there would be > around 150 opportunities for its use. Note that currently there's no standard clk API so any work there is a bit more painful than it perhaps should be. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:22 ` Mark Brown 2012-01-26 13:40 ` Julia Lawall @ 2012-01-28 7:03 ` Julia Lawall 2012-01-28 8:29 ` Wolfram Sang 2012-02-06 16:20 ` Mark Brown 1 sibling, 2 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-28 7:03 UTC (permalink / raw) To: Mark Brown Cc: Julia Lawall, Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood From: Julia Lawall <Julia.Lawall@lip6.fr> The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function. By removing the need for kfree and iounmap, this also eliminates missing resource-release problems. This also removes a now unneeded test on iores. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- patch against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git sound/soc/mxs/mxs-saif.c | 53 ++++++++++------------------------------------- 1 file changed, 12 insertions(+), 41 deletions(-) diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev) return ret; } - saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM; @@ -652,29 +652,16 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return ret; } iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!iores) { - ret = -ENODEV; - dev_err(&pdev->dev, "failed to get io resource: %d\n", - ret); - goto failed_get_resource; - } - if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - ret = -EBUSY; - goto failed_get_resource; - } - - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_request_and_ioremap(&pdev->dev, iores); if (!saif->base) { - dev_err(&pdev->dev, "ioremap failed\n"); + dev_err(&pdev->dev, "devm_request_and_ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; } dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -682,7 +669,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start; @@ -691,14 +678,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; } saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; } saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -706,7 +694,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; } platform_set_drvdata(pdev, saif); @@ -714,7 +702,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; } saif->soc_platform_pdev = platform_device_alloc( @@ -737,36 +725,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif); return ret; } static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev); platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif); return 0; } ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-28 7:03 ` [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall @ 2012-01-28 8:29 ` Wolfram Sang 2012-02-06 16:20 ` Mark Brown 1 sibling, 0 replies; 31+ messages in thread From: Wolfram Sang @ 2012-01-28 8:29 UTC (permalink / raw) To: Julia Lawall Cc: Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 777 bytes --] On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > The various devm_ functions allocate memory that is released when a driver > detaches. This patch uses these functions for data that is allocated in > the probe function of a platform device and is only freed in the remove > function. > > By removing the need for kfree and iounmap, this also eliminates missing > resource-release problems. > > This also removes a now unneeded test on iores. > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> Looks good, will test it later today. -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-28 7:03 ` [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 2012-01-28 8:29 ` Wolfram Sang @ 2012-02-06 16:20 ` Mark Brown 2012-02-08 10:18 ` Dong Aisheng 1 sibling, 1 reply; 31+ messages in thread From: Mark Brown @ 2012-02-06 16:20 UTC (permalink / raw) To: Julia Lawall Cc: Wolfram Sang, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 437 bytes --] On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote: > patch against > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git Hrm, I know there had been some debate about what to generate against but this now once again fails to apply against for-3.4... No idea what's going on there and I didn't investigate the conflicts, sorry. I had been waiting in the vauge hope that some of the mxs maintainers would review. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-02-06 16:20 ` Mark Brown @ 2012-02-08 10:18 ` Dong Aisheng 2012-02-08 12:39 ` Julia Lawall 0 siblings, 1 reply; 31+ messages in thread From: Dong Aisheng @ 2012-02-08 10:18 UTC (permalink / raw) To: Mark Brown Cc: Julia Lawall, alsa-devel, Takashi Iwai, kernel-janitors, Wolfram Sang, linux-kernel, Dong Aisheng-B29396, Liam Girdwood On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote: > >> patch against >> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git > > Hrm, I know there had been some debate about what to generate against > but this now once again fails to apply against for-3.4... No idea > what's going on there and I didn't investigate the conflicts, sorry. > > I had been waiting in the vauge hope that some of the mxs maintainers > would review. > I also failed to apply it against for-3.4. It seemed the conflicts are a lot. Hi Julia, Can you regenerate the patch based on Mark's for-3.4 branch? BTW, i wonder if this patch's title should be changed a bit since what this patch does is not adding missing iounmap anymore. Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends. Regards Dong Aisheng ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-02-08 10:18 ` Dong Aisheng @ 2012-02-08 12:39 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2012-02-08 12:39 UTC (permalink / raw) To: Dong Aisheng Cc: Mark Brown, Julia Lawall, alsa-devel, Takashi Iwai, kernel-janitors, Wolfram Sang, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: TEXT/PLAIN, Size: 1056 bytes --] On Wed, 8 Feb 2012, Dong Aisheng wrote: > On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote: >> >>> patch against >>> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git >> >> Hrm, I know there had been some debate about what to generate against >> but this now once again fails to apply against for-3.4... No idea >> what's going on there and I didn't investigate the conflicts, sorry. >> >> I had been waiting in the vauge hope that some of the mxs maintainers >> would review. >> > I also failed to apply it against for-3.4. > It seemed the conflicts are a lot. > > Hi Julia, > > Can you regenerate the patch based on Mark's for-3.4 branch? > > BTW, i wonder if this patch's title should be changed a bit > since what this patch does is not adding missing iounmap anymore. > Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends. Yes, thanks for the suggestions. I will do both shortly (today or tomorrow). julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:21 ` Julia Lawall 2012-01-26 11:22 ` Mark Brown @ 2012-01-26 11:25 ` Wolfram Sang 2012-01-26 11:28 ` Julia Lawall 1 sibling, 1 reply; 31+ messages in thread From: Wolfram Sang @ 2012-01-26 11:25 UTC (permalink / raw) To: Julia Lawall Cc: Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood [-- Attachment #1: Type: text/plain, Size: 1333 bytes --] On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote: > On Thu, 26 Jan 2012, Wolfram Sang wrote: > > >>@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) > >> goto failed_get_resource; > >> } > >> > >>- if (!request_mem_region(iores->start, resource_size(iores), > >>- "mxs-saif")) { > >>+ if (!devm_request_mem_region(&pdev->dev, iores->start, > >>+ resource_size(iores), "mxs-saif")) { > >> dev_err(&pdev->dev, "request_mem_region failed\n"); > >> ret = -EBUSY; > >> goto failed_get_resource; > >> } > >> > >>- saif->base = ioremap(iores->start, resource_size(iores)); > >>+ saif->base = devm_ioremap(&pdev->dev, iores->start, > >>+ resource_size(iores)); > > > >devm_request_and_ioremap() to save even more code? > > I didn't do that because apparently it is not yet in a release? I > have another for introducing those... It got included in the last merge window: commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0 Author: Wolfram Sang <w.sang@pengutronix.de> Date: Tue Oct 25 15:16:47 2011 +0200 lib: devres: add convenience function to remap a resource ... -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:25 ` Wolfram Sang @ 2012-01-26 11:28 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2012-01-26 11:28 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood On Thu, 26 Jan 2012, Wolfram Sang wrote: > On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote: >> On Thu, 26 Jan 2012, Wolfram Sang wrote: >> >>>> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) >>>> goto failed_get_resource; >>>> } >>>> >>>> - if (!request_mem_region(iores->start, resource_size(iores), >>>> - "mxs-saif")) { >>>> + if (!devm_request_mem_region(&pdev->dev, iores->start, >>>> + resource_size(iores), "mxs-saif")) { >>>> dev_err(&pdev->dev, "request_mem_region failed\n"); >>>> ret = -EBUSY; >>>> goto failed_get_resource; >>>> } >>>> >>>> - saif->base = ioremap(iores->start, resource_size(iores)); >>>> + saif->base = devm_ioremap(&pdev->dev, iores->start, >>>> + resource_size(iores)); >>> >>> devm_request_and_ioremap() to save even more code? >> >> I didn't do that because apparently it is not yet in a release? I >> have another for introducing those... > > It got included in the last merge window: > > commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0 > Author: Wolfram Sang <w.sang@pengutronix.de> > Date: Tue Oct 25 15:16:47 2011 +0200 > > lib: devres: add convenience function to remap a resource > ... OK, thanks. I'll take that into account. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap 2012-01-26 11:05 ` [alsa-devel] " Wolfram Sang 2012-01-26 11:21 ` Julia Lawall @ 2012-01-26 11:57 ` walter harms 1 sibling, 0 replies; 31+ messages in thread From: walter harms @ 2012-01-26 11:57 UTC (permalink / raw) To: Wolfram Sang Cc: Julia Lawall, Mark Brown, alsa-devel, Takashi Iwai, kernel-janitors, linux-kernel, Dong Aisheng-B29396, Liam Girdwood Am 26.01.2012 12:05, schrieb Wolfram Sang: >> @@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) >> goto failed_get_resource; >> } >> >> - if (!request_mem_region(iores->start, resource_size(iores), >> - "mxs-saif")) { >> + if (!devm_request_mem_region(&pdev->dev, iores->start, >> + resource_size(iores), "mxs-saif")) { >> dev_err(&pdev->dev, "request_mem_region failed\n"); >> ret = -EBUSY; >> goto failed_get_resource; >> } >> >> - saif->base = ioremap(iores->start, resource_size(iores)); >> + saif->base = devm_ioremap(&pdev->dev, iores->start, >> + resource_size(iores)); > > devm_request_and_ioremap() to save even more code? > hi all, at first I am NOT the maintainer ... personally i would stay with Julia's patch and add devm_request_and_ioremap() in a second round. The devm stuff is brand new, and two steps here would allow to spot/locate problems more easy since structural changes are less intrusive in each step. re, wh ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2012-02-08 12:39 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-12 9:55 [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 2012-01-12 10:45 ` Felipe Balbi 2012-01-13 6:25 ` Dong Aisheng-B29396 2012-01-13 6:32 ` Julia Lawall 2012-01-24 17:29 ` Julia Lawall 2012-01-24 20:22 ` Mark Brown 2012-01-24 20:36 ` Julia Lawall 2012-01-24 20:45 ` Julia Lawall 2012-01-26 10:53 ` Mark Brown 2012-01-26 11:24 ` Julia Lawall 2012-01-26 11:27 ` Mark Brown 2012-01-26 11:05 ` [alsa-devel] " Wolfram Sang 2012-01-26 11:21 ` Julia Lawall 2012-01-26 11:22 ` Mark Brown 2012-01-26 13:40 ` Julia Lawall 2012-01-26 13:51 ` Wolfram Sang 2012-01-26 14:07 ` Julia Lawall 2012-01-28 8:26 ` Wolfram Sang 2012-01-28 8:51 ` Julia Lawall 2012-02-04 18:25 ` devm_request_and_ioremap Julia Lawall 2012-02-04 18:46 ` devm_request_and_ioremap Mark Brown 2012-02-04 22:08 ` devm_request_and_ioremap Julia Lawall 2012-02-06 16:21 ` devm_request_and_ioremap Mark Brown 2012-01-28 7:03 ` [alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap Julia Lawall 2012-01-28 8:29 ` Wolfram Sang 2012-02-06 16:20 ` Mark Brown 2012-02-08 10:18 ` Dong Aisheng 2012-02-08 12:39 ` Julia Lawall 2012-01-26 11:25 ` Wolfram Sang 2012-01-26 11:28 ` Julia Lawall 2012-01-26 11:57 ` walter harms
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).