| Message ID | 20230801052321.1328225-1-harshit.m.mogalapalli@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [next] mmc: sunplus: Fix error handling in spmmc_drv_probe() | expand |
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
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; }
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(-)