[next] mmc: sunplus: Fix error handling in spmmc_drv_probe()
Commit Message
There are few issues in spmmc_drv_probe():
1. When mmc allocation fails, goto is a no-op.
2. When mmc allocation succeeds, the error paths should use goto instead
of direct return.
3. platform_get_irq() doesn't return zero, so '<' is sufficient.
Fix the above issues by adding goto instead of direct return, also
remove NULL check in 'probe_free_host' as we changed the goto to return
when mmc_alloc_host() fails.
Fixes: 4e268fed8b18 ("mmc: Add mmc driver for Sunplus SP7021")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/a3829ed3-d827-4b9d-827e-9cc24a3ec3bc@moroto.mountain/
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
This is based on static analysis with Smatch. Only compile tested.
---
drivers/mmc/host/sunplus-mmc.c | 41 ++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 17 deletions(-)
Comments
On Tue, 1 Aug 2023 at 07:23, Harshit Mogalapalli
<harshit.m.mogalapalli@oracle.com> wrote:
>
> There are few issues in spmmc_drv_probe():
>
> 1. When mmc allocation fails, goto is a no-op.
> 2. When mmc allocation succeeds, the error paths should use goto instead
> of direct return.
Rather than adding a bunch of new "gotos", how about converting into
using devm_mmc_alloc_host()?
> 3. platform_get_irq() doesn't return zero, so '<' is sufficient.
>
> Fix the above issues by adding goto instead of direct return, also
> remove NULL check in 'probe_free_host' as we changed the goto to return
> when mmc_alloc_host() fails.
>
> Fixes: 4e268fed8b18 ("mmc: Add mmc driver for Sunplus SP7021")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/a3829ed3-d827-4b9d-827e-9cc24a3ec3bc@moroto.mountain/
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Other than the above, this looks good to me!
Kind regards
Uffe
> ---
> This is based on static analysis with Smatch. Only compile tested.
> ---
> drivers/mmc/host/sunplus-mmc.c | 41 ++++++++++++++++++++--------------
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mmc/host/sunplus-mmc.c b/drivers/mmc/host/sunplus-mmc.c
> index a55a87f64d2a..21cd49be08c0 100644
> --- a/drivers/mmc/host/sunplus-mmc.c
> +++ b/drivers/mmc/host/sunplus-mmc.c
> @@ -864,10 +864,8 @@ static int spmmc_drv_probe(struct platform_device *pdev)
> int ret = 0;
>
> mmc = mmc_alloc_host(sizeof(*host), &pdev->dev);
> - if (!mmc) {
> - ret = -ENOMEM;
> - goto probe_free_host;
> - }
> + if (!mmc)
> + return -ENOMEM;
>
> host = mmc_priv(mmc);
> host->mmc = mmc;
> @@ -875,30 +873,40 @@ static int spmmc_drv_probe(struct platform_device *pdev)
> host->dma_int_threshold = 1024;
>
> host->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> - if (IS_ERR(host->base))
> - return PTR_ERR(host->base);
> + if (IS_ERR(host->base)) {
> + ret = PTR_ERR(host->base);
> + goto probe_free_host;
> + }
>
> host->clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(host->clk))
> - return dev_err_probe(&pdev->dev, PTR_ERR(host->clk), "clk get fail\n");
> + if (IS_ERR(host->clk)) {
> + ret = dev_err_probe(&pdev->dev, PTR_ERR(host->clk), "clk get fail\n");
> + goto probe_free_host;
> + }
>
> host->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> - if (IS_ERR(host->rstc))
> - return dev_err_probe(&pdev->dev, PTR_ERR(host->rstc), "rst get fail\n");
> + if (IS_ERR(host->rstc)) {
> + ret = dev_err_probe(&pdev->dev, PTR_ERR(host->rstc), "rst get fail\n");
> + goto probe_free_host;
> + }
>
> host->irq = platform_get_irq(pdev, 0);
> - if (host->irq <= 0)
> - return host->irq;
> + if (host->irq < 0) {
> + ret = host->irq;
> + goto probe_free_host;
> + }
>
> ret = devm_request_threaded_irq(&pdev->dev, host->irq,
> spmmc_irq, spmmc_func_finish_req, IRQF_SHARED,
> NULL, host);
> if (ret)
> - return ret;
> + goto probe_free_host;
>
> ret = clk_prepare_enable(host->clk);
> - if (ret)
> - return dev_err_probe(&pdev->dev, ret, "failed to enable clk\n");
> + if (ret) {
> + ret = dev_err_probe(&pdev->dev, ret, "failed to enable clk\n");
> + goto probe_free_host;
> + }
>
> ret = mmc_of_parse(mmc);
> if (ret)
> @@ -940,8 +948,7 @@ static int spmmc_drv_probe(struct platform_device *pdev)
> clk_disable_unprepare(host->clk);
>
> probe_free_host:
> - if (mmc)
> - mmc_free_host(mmc);
> + mmc_free_host(mmc);
>
> return ret;
> }
> --
> 2.39.3
>
Hi,
On 08/08/23 2:30 pm, Ulf Hansson wrote:
> On Tue, 1 Aug 2023 at 07:23, Harshit Mogalapalli
> <harshit.m.mogalapalli@oracle.com> wrote:
>>
>> There are few issues in spmmc_drv_probe():
>>
>> 1. When mmc allocation fails, goto is a no-op.
>> 2. When mmc allocation succeeds, the error paths should use goto instead
>> of direct return.
>
> Rather than adding a bunch of new "gotos", how about converting into
> using devm_mmc_alloc_host()?
>
Thanks for the suggestion. Yes it would reduce many "gotos".
Will send a V2 with the changes.
>> 3. platform_get_irq() doesn't return zero, so '<' is sufficient.
>>
(Also will split this into different patch as it's fixing a different
thing than above.)
Thanks,
Harshit
>> Fix the above issues by adding goto instead of direct return, also
>> remove NULL check in 'probe_free_host' as we changed the goto to return
>> when mmc_alloc_host() fails.
>>
>> Fixes: 4e268fed8b18 ("mmc: Add mmc driver for Sunplus SP7021")
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/all/a3829ed3-d827-4b9d-827e-9cc24a3ec3bc@moroto.mountain/
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>
> Other than the above, this looks good to me!
>
> Kind regards
> Uffe
@@ -864,10 +864,8 @@ static int spmmc_drv_probe(struct platform_device *pdev)
int ret = 0;
mmc = mmc_alloc_host(sizeof(*host), &pdev->dev);
- if (!mmc) {
- ret = -ENOMEM;
- goto probe_free_host;
- }
+ if (!mmc)
+ return -ENOMEM;
host = mmc_priv(mmc);
host->mmc = mmc;
@@ -875,30 +873,40 @@ static int spmmc_drv_probe(struct platform_device *pdev)
host->dma_int_threshold = 1024;
host->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
- if (IS_ERR(host->base))
- return PTR_ERR(host->base);
+ if (IS_ERR(host->base)) {
+ ret = PTR_ERR(host->base);
+ goto probe_free_host;
+ }
host->clk = devm_clk_get(&pdev->dev, NULL);
- if (IS_ERR(host->clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(host->clk), "clk get fail\n");
+ if (IS_ERR(host->clk)) {
+ ret = dev_err_probe(&pdev->dev, PTR_ERR(host->clk), "clk get fail\n");
+ goto probe_free_host;
+ }
host->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
- if (IS_ERR(host->rstc))
- return dev_err_probe(&pdev->dev, PTR_ERR(host->rstc), "rst get fail\n");
+ if (IS_ERR(host->rstc)) {
+ ret = dev_err_probe(&pdev->dev, PTR_ERR(host->rstc), "rst get fail\n");
+ goto probe_free_host;
+ }
host->irq = platform_get_irq(pdev, 0);
- if (host->irq <= 0)
- return host->irq;
+ if (host->irq < 0) {
+ ret = host->irq;
+ goto probe_free_host;
+ }
ret = devm_request_threaded_irq(&pdev->dev, host->irq,
spmmc_irq, spmmc_func_finish_req, IRQF_SHARED,
NULL, host);
if (ret)
- return ret;
+ goto probe_free_host;
ret = clk_prepare_enable(host->clk);
- if (ret)
- return dev_err_probe(&pdev->dev, ret, "failed to enable clk\n");
+ if (ret) {
+ ret = dev_err_probe(&pdev->dev, ret, "failed to enable clk\n");
+ goto probe_free_host;
+ }
ret = mmc_of_parse(mmc);
if (ret)
@@ -940,8 +948,7 @@ static int spmmc_drv_probe(struct platform_device *pdev)
clk_disable_unprepare(host->clk);
probe_free_host:
- if (mmc)
- mmc_free_host(mmc);
+ mmc_free_host(mmc);
return ret;
}