Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20230801052321.1328225-1-harshit.m.mogalapalli@oracle.com>
In-Reply-To: <20230801052321.1328225-1-harshit.m.mogalapalli@oracle.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Tue, 8 Aug 2023 11:00:57 +0200
Message-ID: <CAPDyKFr9inG4dgAUYhrQOEND1Fu1CDajEPoO3W=dBRRXB=kceQ@mail.gmail.com>
Subject: Re: [PATCH next] mmc: sunplus: Fix error handling in spmmc_drv_probe()
To:     Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Cc:     Tony Huang <tonyhuang.sunplus@gmail.com>,
        Li-hao Kuo <lhjeff911@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, dan.carpenter@linaro.org,
        kernel-janitors@vger.kernel.org, error27@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>