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

[Khalid Aziz <khalid@kernel.org>]

On 10/10/25 3:59 AM, Uwe Kleine-König wrote:
> Hello Rakuram,
> 
> On Thu, Oct 09, 2025 at 08:57:38PM +0530, Rakuram Eswaran wrote:
>> Your suggestion makes perfect sense — since host is devm-managed,
>> explicitly assigning its members to NULL has no effect.
>> I’ll remove those two redundant lines in v2 as you suggested.
>>
>> I had one small clarification regarding the remaining host->dma_chan_tx = NULL;
>> in the TX DMA error path. Since that branch uses goto out,
>> the cleanup section below may call dma_release_channel() on both RX
>> and TX pointers. Setting TX to NULL there seems like a defensive step
>> to avoid accidentally passing an ERR_PTR() to dma_release_channel()
>> — is that understanding correct?
> 
> Ah right, so either keep host->dma_chan_tx = NULL or improve the error
> handling like:
> 
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..e5068cc55fb2 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -715,7 +715,7 @@ static int pxamci_probe(struct platform_device *pdev)
>   		dev_err(dev, "unable to request tx dma channel\n");
>   		ret = PTR_ERR(host->dma_chan_tx);
>   		host->dma_chan_tx = NULL;
> -		goto out;
> +		goto out_dma_tx;
>   	}
>   
>   	if (host->pdata) {
> @@ -765,10 +765,11 @@ static int pxamci_probe(struct platform_device *pdev)
>   	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);
> +out_dma_tx:
> +	if (host->dma_chan_rx)
> +		dma_release_channel(host->dma_chan_rx);
>   	return ret;
>   }

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.

--
Khalid


>   
>> Also, I noticed that in the build configuration downloaded from the LKP report
>> link (CONFIG_DMA_ENGINE isn’t defined), the kernel uses the stub inline
>> version of dma_release_channel() from include/linux/dmaengine.h,
>> which becomes a no-op.
>>
>>  From what I understand, when the DMA engine framework isn’t enabled,
>> these APIs compile as no-ops through their inline stubs.
>> Please correct me if I’m misunderstanding how this works.
>>
>> Please let me know if this reasoning aligns with what you had in mind.
> 
> Sounds right.
> 
> Best regards
> Uwe