| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6j7ix5yof7qmrp6cgxhqver7yimvmgj7dujqu4l7cnzbpjksfd@5sp7am47gigw>
Date: Mon, 13 Oct 2025 10:45:03 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Rakuram Eswaran <rakuram.e96@...il.com>
Cc: chenhuacai@...nel.org, dan.carpenter@...aro.org,
david.hunter.linux@...il.com, khalid@...nel.org, linux-kernel-mentees@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org, lkp@...el.com,
skhan@...uxfoundation.org, ulf.hansson@...aro.org, zhoubinbin@...ngson.cn
Subject: Re: [PATCH] mmc: pxamci: Fix passing NULL to PTR_ERR() in
pxamci_probe()
Hello Rakuram,
On Mon, Oct 13, 2025 at 12:07:52AM +0530, Rakuram Eswaran wrote:
> > >
> > > I do not see the need for this code change. "if (host->dma_chan_tx)" will
> > > skip "dma_release_channel(host->dma_chan_tx)" since dma_chan_tx is already
> > > NULL. This code change does not add anything.
> >
> > Yes, stand alone this change doesn't make sense, but if we want to drop
> >
> > host->dma_chan_tx = NULL
> >
> > in the error path above, this change is needed. Maybe then even
> >
> > if (host->dma_chan_rx)
> >
> > and
> >
> > if (host->dma_chan_rx)
> >
> > can be dropped.
>
> Hello Uwe,
>
> I had one quick follow-up before sending v2.
>
> Regarding the devm_clk_get() error path —
> you mentioned that setting host->clk = NULL; is redundant since host is
> devm-managed and the function returns immediately afterward.
>
> > I am not sure that sounds right. Looking at the code for
> > __devm_clk_get(), if devres_alloc() fails, it returns -ENOMEM. If any of
> > the other steps after a successful devres_alloc() fail, code goes
> > through possibly clk_put() if needed and then devres_free(). So the
> > resources are already freed at this point before the return to
> > pxamci_probe(). The only thing left to do is to set host->clk to NULL
> > since it would be set to an error pointer at this point.
>
> Khalid pointed out that when __devm_clk_get() fails after allocating a
> devres entry, the internal cleanup (clk_put() + devres_free()) ensures
> resources are released, but host->clk would still hold an ERR_PTR()
> value at that point.
>
> His suggestion was that setting it to NULL might be a harmless defensive
> step to avoid any accidental later dereference.
Why is NULL better than an error pointer? (Spoiler: It isn't.)
> For now, I have dropped the redundant NULL assignment from
> host->dma_chan_rx = NULL and directly returning the ERR_PTR instead of
> storing in a return variable.
>
> Below I have appended proposed changes for v2.
>
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..eb46a4861dbe 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -653,8 +653,9 @@ static int pxamci_probe(struct platform_device *pdev)
>
> host->clk = devm_clk_get(dev, NULL);
> if (IS_ERR(host->clk)) {
> + ret = PTR_ERR(host->clk);
> host->clk = NULL;
> - return PTR_ERR(host->clk);
> + return ret;
> }
>
> host->clkrate = clk_get_rate(host->clk);
> @@ -705,7 +706,6 @@ static int pxamci_probe(struct platform_device *pdev)
>
> host->dma_chan_rx = dma_request_chan(dev, "rx");
> if (IS_ERR(host->dma_chan_rx)) {
> - host->dma_chan_rx = NULL;
> return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
> "unable to request rx dma channel\n");
> }
>
> Would you prefer that I:
>
> 1. Remove the host->clk = NULL; assignment for consistency (as you initially
> suggested), or
>
> 2. Keep it in v2 for defensive clarity, as Khalid reasoned?
>
> I just wanted to confirm your preference before resending, to keep v2 aligned.
Note that in the end it's not me who decides, but Ulf (= mmc
maintainer).
If you ask me however, I'd say the right thing to do there is like the
following:
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 26d03352af63..ce896b3f697b 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -652,11 +652,13 @@ static int pxamci_probe(struct platform_device *pdev)
host->clkrt = CLKRT_OFF;
host->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(host->clk)) {
- host->clk = NULL;
- return PTR_ERR(host->clk);
- }
+ if (IS_ERR(host->clk))
+ return dev_err_probe(dev, PTR_ERR(host->clk), "Failed to aquire clock\n");
+ /*
+ * XXX: Note that the return value of clk_get_rate() is only valid if
+ * the clock is enabled.
+ */
host->clkrate = clk_get_rate(host->clk);
/*
@@ -703,20 +705,15 @@ static int pxamci_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, mmc);
- host->dma_chan_rx = dma_request_chan(dev, "rx");
- if (IS_ERR(host->dma_chan_rx)) {
- host->dma_chan_rx = NULL;
+ host->dma_chan_rx = devm_dma_request_chan(dev, "rx");
+ if (IS_ERR(host->dma_chan_rx))
return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
"unable to request rx dma channel\n");
- }
- host->dma_chan_tx = dma_request_chan(dev, "tx");
- if (IS_ERR(host->dma_chan_tx)) {
- dev_err(dev, "unable to request tx dma channel\n");
- ret = PTR_ERR(host->dma_chan_tx);
- host->dma_chan_tx = NULL;
- goto out;
- }
+ host->dma_chan_tx = devm_dma_request_chan(dev, "tx");
+ if (IS_ERR(host->dma_chan_tx))
+ return dev_err_probe(dev, PTR_ERR(host->dma_chan_tx),
+ "unable to request tx dma channel\n");
if (host->pdata) {
host->detect_delay_ms = host->pdata->detect_delay_ms;
@@ -724,25 +721,21 @@ static int pxamci_probe(struct platform_device *pdev)
host->power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
if (IS_ERR(host->power)) {
ret = PTR_ERR(host->power);
- dev_err(dev, "Failed requesting gpio_power\n");
- goto out;
+ return dev_err_probe(dev, ret, "Failed requesting gpio_power\n");
}
/* FIXME: should we pass detection delay to debounce? */
ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
- if (ret && ret != -ENOENT) {
- dev_err(dev, "Failed requesting gpio_cd\n");
- goto out;
- }
+ if (ret && ret != -ENOENT)
+ return dev_err_probe(dev, ret, "Failed requesting gpio_cd\n");
if (!host->pdata->gpio_card_ro_invert)
mmc->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
ret = mmc_gpiod_request_ro(mmc, "wp", 0, 0);
- if (ret && ret != -ENOENT) {
- dev_err(dev, "Failed requesting gpio_ro\n");
- goto out;
- }
+ if (ret && ret != -ENOENT)
+ return dev_err_probe(dev, ret, "Failed requesting gpio_ro\n");
+
if (!ret)
host->use_ro_gpio = true;
@@ -759,16 +752,8 @@ static int pxamci_probe(struct platform_device *pdev)
if (ret) {
if (host->pdata && host->pdata->exit)
host->pdata->exit(dev, mmc);
- goto out;
}
- return 0;
-
-out:
- if (host->dma_chan_rx)
- dma_release_channel(host->dma_chan_rx);
- if (host->dma_chan_tx)
- dma_release_channel(host->dma_chan_tx);
return ret;
}
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists