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

[Khalid Aziz <khalid@kernel.org>]

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