Re: [PATCH] mmc: pxamci: Fix passing NULL to PTR_ERR() in pxamci_probe()

Uwe Kleine-König
2025-10-13T08:45:07Z

[-- Attachment #1: Type: text/plain, Size: 6466 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]