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

Uwe Kleine-König
2025-10-12T14:15:48Z

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

On Fri, Oct 10, 2025 at 11:59:57AM -0600, Khalid Aziz wrote:
> 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.

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.

Best regards
Uwe

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