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

Khalid Aziz
2025-10-10T17:59:57Z

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