[PATCH 18/34] mmc: sdhci-of-aspeed: Drop the use of sdhci_pltfm_free()

[PATCH 18/34] mmc: sdhci-of-aspeed: Drop the use of sdhci_pltfm_free()

[Binbin Zhou]

Since the devm_mmc_alloc_host() helper is already in
use, sdhci_pltfm_free() is no longer needed.

Cc: Andrew Jeffery <andrew@codeconstruct.com.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: linux-aspeed@lists.ozlabs.org
Cc: openbmc@lists.ozlabs.org
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d6de010551b9..ca97b01996b1 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -425,10 +425,8 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		return PTR_ERR(pltfm_host->clk);
 
 	ret = clk_prepare_enable(pltfm_host->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Unable to enable SDIO clock\n");
-		goto err_pltfm_free;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Unable to enable SDIO clock\n");
 
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
@@ -445,8 +443,6 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 
 err_sdhci_add:
 	clk_disable_unprepare(pltfm_host->clk);
-err_pltfm_free:
-	sdhci_pltfm_free(pdev);
 	return ret;
 }
 
@@ -461,8 +457,6 @@ static void aspeed_sdhci_remove(struct platform_device *pdev)
 	sdhci_remove_host(host, 0);
 
 	clk_disable_unprepare(pltfm_host->clk);
-
-	sdhci_pltfm_free(pdev);
 }
 
 static const struct aspeed_sdhci_pdata ast2400_sdhci_pdata = {
-- 
2.47.1

Re: [PATCH 18/34] mmc: sdhci-of-aspeed: Drop the use of sdhci_pltfm_free()

[Andrew Jeffery]

On Mon, 2025-05-26 at 14:06 +0800, Binbin Zhou wrote:
> Since the devm_mmc_alloc_host() helper is already in
> use, 

This doesn't appear to be true? aspeed_sdhci_probe() calls
sdhci_pltfm_init(), and following that through eventually we call
mmc_alloc_host() (the non-devm_ variant).

That said, there are some error paths in aspeed_sdhci_probe() where we
leak the object. Improving the code would be helpful, but my
understanding is this patch isn't right.

Andrew

Re: [PATCH 18/34] mmc: sdhci-of-aspeed: Drop the use of sdhci_pltfm_free()

[Binbin Zhou]

Hi Andrew:

Thanks for your reply.

On Wed, May 28, 2025 at 8:44 AM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> On Mon, 2025-05-26 at 14:06 +0800, Binbin Zhou wrote:
> > Since the devm_mmc_alloc_host() helper is already in
> > use,
>
> This doesn't appear to be true? aspeed_sdhci_probe() calls
> sdhci_pltfm_init(), and following that through eventually we call
> mmc_alloc_host() (the non-devm_ variant).
>
> That said, there are some error paths in aspeed_sdhci_probe() where we
> leak the object. Improving the code would be helpful, but my
> understanding is this patch isn't right.

Sorry, I have a different thought.
In the original code, there does seem to be a leakage of objects, for example:

pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
        if (IS_ERR(pltfm_host->clk))
                return PTR_ERR(pltfm_host->clk);

With this error path, we should goto err_pltfm_free;

Now, I have replaced mmc_alloc_host() with devm_mmc_alloc_host() in
Patch-1[1], so I don't think there will be any error path leakage of
objects.

[1]: https://lore.kernel.org/all/6fd5afb003982bb5edbf95f76b605686223730e0.1747792905.git.zhoubinbin@loongson.cn/
>
> Andrew

-- 
Thanks.
Binbin

Re: [PATCH 18/34] mmc: sdhci-of-aspeed: Drop the use of sdhci_pltfm_free()

[Andrew Jeffery]

On Wed, 2025-05-28 at 12:42 +0800, Binbin Zhou wrote:
> Hi Andrew:
> 
> Thanks for your reply.
> 
> On Wed, May 28, 2025 at 8:44 AM Andrew Jeffery
> <andrew@codeconstruct.com.au> wrote:
> > 
> > On Mon, 2025-05-26 at 14:06 +0800, Binbin Zhou wrote:
> > > Since the devm_mmc_alloc_host() helper is already in
> > > use,
> > 
> > This doesn't appear to be true? aspeed_sdhci_probe() calls
> > sdhci_pltfm_init(), and following that through eventually we call
> > mmc_alloc_host() (the non-devm_ variant).
> > 
> > That said, there are some error paths in aspeed_sdhci_probe() where we
> > leak the object. Improving the code would be helpful, but my
> > understanding is this patch isn't right.
> 
> Sorry, I have a different thought.
> In the original code, there does seem to be a leakage of objects, for example:
> 
> pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(pltfm_host->clk))
>                 return PTR_ERR(pltfm_host->clk);
> 
> With this error path, we should goto err_pltfm_free;
> 
> Now, I have replaced mmc_alloc_host() with devm_mmc_alloc_host() in
> Patch-1[1], so I don't think there will be any error path leakage of
> objects.
> 
> [1]: https://lore.kernel.org/all/6fd5afb003982bb5edbf95f76b605686223730e0.1747792905.git.zhoubinbin@loongson.cn/

Okay, I didn't track down the rest of the series on lore. I don't
personally mind if you send me the whole series, that would certainly
have helped with the context in this case.

Andrew