The sequence of resource release in probe failure path in this
driver was wrong and needed fixes to cleanly unload the driver.
This changes does the same by switching to managed resources and
fixes return path to release resource in proper sequence.
Cc: Chris Ball <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: [email protected]
Signed-off-by: Pramod Gurav <[email protected]>
---
Changes since v1:
- Dropped IS_ERR check on devm_ioremap() return.
- Fixed sequence on mmc_remove_host in fail as well as in remove function
drivers/mmc/host/davinci_mmc.c | 91 +++++++++++++++-------------------------
1 file changed, 33 insertions(+), 58 deletions(-)
diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 5d4c5e0..dc49b22 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -1242,22 +1242,20 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
return -ENOENT;
}
- ret = -ENODEV;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
irq = platform_get_irq(pdev, 0);
if (!r || irq == NO_IRQ)
- goto out;
+ return -ENODEV;
- ret = -EBUSY;
mem_size = resource_size(r);
- mem = request_mem_region(r->start, mem_size, pdev->name);
+ mem = devm_request_mem_region(&pdev->dev, r->start, mem_size,
+ pdev->name);
if (!mem)
- goto out;
+ return -EBUSY;
- ret = -ENOMEM;
mmc = mmc_alloc_host(sizeof(struct mmc_davinci_host), &pdev->dev);
if (!mmc)
- goto out;
+ return -ENOMEM;
host = mmc_priv(mmc);
host->mmc = mmc; /* Important */
@@ -1275,15 +1273,16 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
host->txdma = r->start;
host->mem_res = mem;
- host->base = ioremap(mem->start, mem_size);
- if (!host->base)
- goto out;
+ host->base = devm_ioremap(&pdev->dev, mem->start, mem_size);
+ if (!host->base) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
- ret = -ENXIO;
- host->clk = clk_get(&pdev->dev, "MMCSDCLK");
+ host->clk = devm_clk_get(&pdev->dev, "MMCSDCLK");
if (IS_ERR(host->clk)) {
ret = PTR_ERR(host->clk);
- goto out;
+ goto err_ioremap;
}
clk_enable(host->clk);
host->mmc_input_clk = clk_get_rate(host->clk);
@@ -1350,20 +1349,22 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
ret = mmc_davinci_cpufreq_register(host);
if (ret) {
dev_err(&pdev->dev, "failed to register cpufreq\n");
- goto cpu_freq_fail;
+ goto err_cpu_freq;
}
ret = mmc_add_host(mmc);
if (ret < 0)
- goto out;
+ goto err_mmc_add;
- ret = request_irq(irq, mmc_davinci_irq, 0, mmc_hostname(mmc), host);
+ ret = devm_request_irq(&pdev->dev, irq, mmc_davinci_irq, 0,
+ mmc_hostname(mmc), host);
if (ret)
- goto out;
+ goto err_request_irq;
if (host->sdio_irq >= 0) {
- ret = request_irq(host->sdio_irq, mmc_davinci_sdio_irq, 0,
- mmc_hostname(mmc), host);
+ ret = devm_request_irq(&pdev->dev, host->sdio_irq,
+ mmc_davinci_sdio_irq, 0,
+ mmc_hostname(mmc), host);
if (!ret)
mmc->caps |= MMC_CAP_SDIO_IRQ;
}
@@ -1376,27 +1377,15 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
return 0;
-out:
+err_request_irq:
+ mmc_remove_host(mmc);
+err_mmc_add:
mmc_davinci_cpufreq_deregister(host);
-cpu_freq_fail:
- if (host) {
- davinci_release_dma_channels(host);
-
- if (host->clk) {
- clk_disable(host->clk);
- clk_put(host->clk);
- }
-
- if (host->base)
- iounmap(host->base);
- }
-
- if (mmc)
- mmc_free_host(mmc);
-
- if (mem)
- release_resource(mem);
-
+err_cpu_freq:
+ davinci_release_dma_channels(host);
+ clk_disable(host->clk);
+err_ioremap:
+ mmc_free_host(mmc);
dev_dbg(&pdev->dev, "probe err %d\n", ret);
return ret;
@@ -1406,25 +1395,11 @@ static int __exit davinci_mmcsd_remove(struct platform_device *pdev)
{
struct mmc_davinci_host *host = platform_get_drvdata(pdev);
- if (host) {
- mmc_davinci_cpufreq_deregister(host);
-
- mmc_remove_host(host->mmc);
- free_irq(host->mmc_irq, host);
- if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
- free_irq(host->sdio_irq, host);
-
- davinci_release_dma_channels(host);
-
- clk_disable(host->clk);
- clk_put(host->clk);
-
- iounmap(host->base);
-
- release_resource(host->mem_res);
-
- mmc_free_host(host->mmc);
- }
+ mmc_remove_host(host->mmc);
+ mmc_davinci_cpufreq_deregister(host);
+ davinci_release_dma_channels(host);
+ clk_disable(host->clk);
+ mmc_free_host(host->mmc);
return 0;
}
--
1.7.9.5
On 30 October 2014 08:46, Pramod Gurav <[email protected]> wrote:
> The sequence of resource release in probe failure path in this
> driver was wrong and needed fixes to cleanly unload the driver.
> This changes does the same by switching to managed resources and
> fixes return path to release resource in proper sequence.
>
> Cc: Chris Ball <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: [email protected]
Please remove these Ccs above from the commit message. It's not needed
when you anyway need to send the patches directly to these addresses.
> Signed-off-by: Pramod Gurav <[email protected]>
> ---
> Changes since v1:
> - Dropped IS_ERR check on devm_ioremap() return.
> - Fixed sequence on mmc_remove_host in fail as well as in remove function
>
> drivers/mmc/host/davinci_mmc.c | 91 +++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
> index 5d4c5e0..dc49b22 100644
> --- a/drivers/mmc/host/davinci_mmc.c
> +++ b/drivers/mmc/host/davinci_mmc.c
> @@ -1242,22 +1242,20 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
> return -ENOENT;
> }
>
> - ret = -ENODEV;
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(pdev, 0);
> if (!r || irq == NO_IRQ)
> - goto out;
> + return -ENODEV;
>
> - ret = -EBUSY;
> mem_size = resource_size(r);
> - mem = request_mem_region(r->start, mem_size, pdev->name);
> + mem = devm_request_mem_region(&pdev->dev, r->start, mem_size,
> + pdev->name);
> if (!mem)
> - goto out;
> + return -EBUSY;
>
> - ret = -ENOMEM;
> mmc = mmc_alloc_host(sizeof(struct mmc_davinci_host), &pdev->dev);
> if (!mmc)
> - goto out;
> + return -ENOMEM;
>
> host = mmc_priv(mmc);
> host->mmc = mmc; /* Important */
> @@ -1275,15 +1273,16 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
> host->txdma = r->start;
>
> host->mem_res = mem;
> - host->base = ioremap(mem->start, mem_size);
> - if (!host->base)
> - goto out;
> + host->base = devm_ioremap(&pdev->dev, mem->start, mem_size);
I realized that you should use devm_ioremap_resource() instead. That
would simplify the code even more.
[...]
Kind regards
Uffe
Thanks Ulf,
On Thu, Oct 30, 2014 at 4:38 PM, Ulf Hansson <[email protected]> wrote:
> On 30 October 2014 08:46, Pramod Gurav <[email protected]> wrote:
>> The sequence of resource release in probe failure path in this
>> driver was wrong and needed fixes to cleanly unload the driver.
>> This changes does the same by switching to managed resources and
>> fixes return path to release resource in proper sequence.
>>
>> Cc: Chris Ball <[email protected]>
>> Cc: Ulf Hansson <[email protected]>
>> Cc: [email protected]
>
> Please remove these Ccs above from the commit message. It's not needed
> when you anyway need to send the patches directly to these addresses.
Will remove Cc.
>
>> Signed-off-by: Pramod Gurav <[email protected]>
>> ---
..
>>
>> host = mmc_priv(mmc);
>> host->mmc = mmc; /* Important */
>> @@ -1275,15 +1273,16 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
>> host->txdma = r->start;
>>
>> host->mem_res = mem;
>> - host->base = ioremap(mem->start, mem_size);
>> - if (!host->base)
>> - goto out;
>> + host->base = devm_ioremap(&pdev->dev, mem->start, mem_size);
>
> I realized that you should use devm_ioremap_resource() instead. That
> would simplify the code even more.
>
Yes, we could have used devm_ioremap_resource() but this driver stores
the return from devm_request_mem_region() in host->mem_res and uses
the same in the other part of drivers. Is there a way to work this
around?
> [...]
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks and Regards
Pramod
On 30 October 2014 12:45, Pramod Gurav <[email protected]> wrote:
> Thanks Ulf,
>
> On Thu, Oct 30, 2014 at 4:38 PM, Ulf Hansson <[email protected]> wrote:
>> On 30 October 2014 08:46, Pramod Gurav <[email protected]> wrote:
>>> The sequence of resource release in probe failure path in this
>>> driver was wrong and needed fixes to cleanly unload the driver.
>>> This changes does the same by switching to managed resources and
>>> fixes return path to release resource in proper sequence.
>>>
>>> Cc: Chris Ball <[email protected]>
>>> Cc: Ulf Hansson <[email protected]>
>>> Cc: [email protected]
>>
>> Please remove these Ccs above from the commit message. It's not needed
>> when you anyway need to send the patches directly to these addresses.
> Will remove Cc.
>
>>
>>> Signed-off-by: Pramod Gurav <[email protected]>
>>> ---
> ..
>
>>>
>>> host = mmc_priv(mmc);
>>> host->mmc = mmc; /* Important */
>>> @@ -1275,15 +1273,16 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
>>> host->txdma = r->start;
>>>
>>> host->mem_res = mem;
>>> - host->base = ioremap(mem->start, mem_size);
>>> - if (!host->base)
>>> - goto out;
>>> + host->base = devm_ioremap(&pdev->dev, mem->start, mem_size);
>>
>> I realized that you should use devm_ioremap_resource() instead. That
>> would simplify the code even more.
>>
> Yes, we could have used devm_ioremap_resource() but this driver stores
> the return from devm_request_mem_region() in host->mem_res and uses
> the same in the other part of drivers. Is there a way to work this
> around?
Isn't that the same value as "mem->start" ?
Kind regards
Uffe