Re: [PATCH next] mmc: sunplus: Fix error handling in spmmc_drv_probe()

[Ulf Hansson]

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
>