| 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: <8afff048-4fe1-440a-9739-e5a5ea43d6eb@kernel.org>
Date: Mon, 13 Oct 2025 16:54:13 -0600
From: Khalid Aziz <khalid@...nel.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>,
Rakuram Eswaran <rakuram.e96@...il.com>
Cc: chenhuacai@...nel.org, dan.carpenter@...aro.org,
david.hunter.linux@...il.com, 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()
On 10/13/25 2:45 AM, Uwe Kleine-König wrote:
> 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");
Hi Uwe,
I agree using dev_err_probe() is better since it leads to better logging
and troubleshooting.
>
> + /*
> + * 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");
We should still release DMA rx channel before returning here.
>
> 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");
Don't we need to release DMA Rx and Tx channels before we return from here?
> }
>
> /* 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");
Same here
>
> 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");
and here.
Looking at Documentation/driver-api/driver-model/devres.rst,
dma_request_chan() is not devres managed interface and thus will not be
released automatically. Do you agree?
--
Khalid
> +
> 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
Powered by blists - more mailing lists