| Message ID | dfc7e01a6134e421ae3aa8da3221f67d59706d0d.1747792905.git.zhoubinbin@loongson.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mmc: Cleanup sdhci_pltfm_free()/sdhci_free_host() usage | expand |
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
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
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
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 = {
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(-)