On Tue, 1 Aug 2023 at 07:23, Harshit Mogalapalli
<harshit.m.mogalapalli@xxxxxxxxxx> 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@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/a3829ed3-d827-4b9d-827e-9cc24a3ec3bc@moroto.mountain/
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@xxxxxxxxxx>
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
>
