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

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

[Rakuram Eswaran]

Smatch reported:
drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'

Case 1:
When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
This mistakenly returns 0 instead of the real error code.

Case 2:
When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
issue like case 1. 

Store the error code before nullifying the pointers in both the cases.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
---

Build and Analysis:
This patch was compiled against the configuration file reported by
0day CI in the above link (config: s390-randconfig-r071-20251004) using
`s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`. 

Static analysis was performed with Smatch to ensure the reported warning 
no longer reproduces after applying this fix.

Command used for verification:
  ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
  ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c

 drivers/mmc/host/pxamci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 26d03352af63..4fab693d3b32 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,8 +706,9 @@ static int pxamci_probe(struct platform_device *pdev)
 
 	host->dma_chan_rx = dma_request_chan(dev, "rx");
 	if (IS_ERR(host->dma_chan_rx)) {
+		ret = PTR_ERR(host->dma_chan_rx);
 		host->dma_chan_rx = NULL;
-		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
+		return dev_err_probe(dev, ret,
 				     "unable to request rx dma channel\n");
 	}
 
-- 
2.48.1

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

[Dan Carpenter]

On Tue, Oct 07, 2025 at 09:47:44PM +0530, Rakuram Eswaran wrote:
> Smatch reported:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> 
> Case 1:
> When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
> but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
> This mistakenly returns 0 instead of the real error code.
> 
> Case 2:
> When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
> issue like case 1. 
> 
> Store the error code before nullifying the pointers in both the cases.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> ---

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

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

[Khalid Aziz]

On 10/7/25 10:17 AM, Rakuram Eswaran wrote:
> Smatch reported:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> 
> Case 1:
> When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
> but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
> This mistakenly returns 0 instead of the real error code.
> 
> Case 2:
> When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
> issue like case 1.
> 
> Store the error code before nullifying the pointers in both the cases.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> ---
> 
> Build and Analysis:
> This patch was compiled against the configuration file reported by
> 0day CI in the above link (config: s390-randconfig-r071-20251004) using
> `s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`.
> 
> Static analysis was performed with Smatch to ensure the reported warning
> no longer reproduces after applying this fix.
> 
> Command used for verification:
>    ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
>    ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c
> 
>   drivers/mmc/host/pxamci.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..4fab693d3b32 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,8 +706,9 @@ static int pxamci_probe(struct platform_device *pdev)
>   
>   	host->dma_chan_rx = dma_request_chan(dev, "rx");
>   	if (IS_ERR(host->dma_chan_rx)) {
> +		ret = PTR_ERR(host->dma_chan_rx);
>   		host->dma_chan_rx = NULL;
> -		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
> +		return dev_err_probe(dev, ret,
>   				     "unable to request rx dma channel\n");
>   	}
>   

This looks good to me.

Reviewed-by: Khalid Aziz <khalid@kernel.org>

--
Khalid

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

[Binbin Zhou]

On 2025/10/8 00:17, Rakuram Eswaran wrote:
> Smatch reported:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
>
> Case 1:
> When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
> but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
> This mistakenly returns 0 instead of the real error code.
>
> Case 2:
> When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
> issue like case 1.
>
> Store the error code before nullifying the pointers in both the cases.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202510041841.pRlunIfl-lkp@intel.com/
> Fixes: 58c40f3faf742c ("mmc: pxamci: Use devm_mmc_alloc_host() helper")
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
LGTM.

Reviewed-by: Binbin Zhou <zhoubinbin@loongson.cn>

> ---
>
> Build and Analysis:
> This patch was compiled against the configuration file reported by
> 0day CI in the above link (config: s390-randconfig-r071-20251004) using
> `s390x-linux-gnu-gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0`.
>
> Static analysis was performed with Smatch to ensure the reported warning
> no longer reproduces after applying this fix.
>
> Command used for verification:
>    ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- \
>    ~/project/smatch/smatch_scripts/kchecker ./drivers/mmc/host/pxamci.c
>
>   drivers/mmc/host/pxamci.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..4fab693d3b32 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,8 +706,9 @@ static int pxamci_probe(struct platform_device *pdev)
>   
>   	host->dma_chan_rx = dma_request_chan(dev, "rx");
>   	if (IS_ERR(host->dma_chan_rx)) {
> +		ret = PTR_ERR(host->dma_chan_rx);
>   		host->dma_chan_rx = NULL;
> -		return dev_err_probe(dev, PTR_ERR(host->dma_chan_rx),
> +		return dev_err_probe(dev, ret,
>   				     "unable to request rx dma channel\n");
>   	}
>   
Thanks.
Binbin

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

[Uwe Kleine-König]

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

On Tue, Oct 07, 2025 at 09:47:44PM +0530, Rakuram Eswaran wrote:
> Smatch reported:
> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
> 
> Case 1:
> When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
> but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
> This mistakenly returns 0 instead of the real error code.
> 
> Case 2:
> When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
> issue like case 1. 
> 
> Store the error code before nullifying the pointers in both the cases.

Why is the pointer set to NULL at all? This is in both cases memory that
is freed directly afterwards (as `host` is devm managed). So I'd claim

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 26d03352af63..404f78198252 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -652,10 +652,8 @@ 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;
+	if (IS_ERR(host->clk))
 		return PTR_ERR(host->clk);
-	}
 
 	host->clkrate = clk_get_rate(host->clk);
 
@@ -704,11 +702,9 @@ 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;
+	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)) {

is a superior patch.

Best regards
Uwe

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

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

[Rakuram Eswaran]

Hi Uwe,

Thanks a lot for the review and the clear explanation.

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?

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.

Thanks again for the insightful feedback — 
I’ll send v2 with those two cleanups shortly.

Best regards,
Rakuram

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

[Uwe Kleine-König]

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

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;
 }
 
> 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

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

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

[Khalid Aziz]

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

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

[Uwe Kleine-König]

[-- 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 --]

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

[Rakuram Eswaran]

> >
> > 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.

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.

Thanks again for your time and detailed feedback!

Best regards,
Rakuram

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

[Uwe Kleine-König]

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

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");
 
+	/*
+	 * 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");
 
 	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");
 		}
 
 		/* 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");
 
 		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");
+
 		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

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

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

[Khalid Aziz]

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

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

[Dan Carpenter]

On Mon, Oct 13, 2025 at 04:54:13PM -0600, Khalid Aziz wrote:
> > @@ -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?
> 

The patch changes dma_request_chan() to devm_dma_request_chan().

regards,
dan carpenter

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

[Khalid Aziz]

On 10/14/25 6:27 AM, Dan Carpenter wrote:
> On Mon, Oct 13, 2025 at 04:54:13PM -0600, Khalid Aziz wrote:
>>> @@ -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?
>>
> 
> The patch changes dma_request_chan() to devm_dma_request_chan().
> 

Ah, yes. It does. That works then.

Thanks,
Khalid

> regards,
> dan carpenter
> 

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

[Rakuram Eswaran]

Hi Uwe, Khalid, Dan, and all,

Thank you all for the detailed review and clarifications.

I’ve just sent [PATCH v2] with the suggested changes — adopting devm-managed
resource handling, removing redundant NULL assignments, and improving the
error paths using dev_err_probe(), as discussed. I’ve also updated the patch
title to better reflect the nature of the change.

Appreciate everyone’s feedback and guidance!

Best Regards,
Rakuram

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

[Khalid Aziz]

On 10/9/25 2:57 AM, Uwe Kleine-König wrote:
> On Tue, Oct 07, 2025 at 09:47:44PM +0530, Rakuram Eswaran wrote:
>> Smatch reported:
>> drivers/mmc/host/pxamci.c:709 pxamci_probe() warn: passing zero to 'PTR_ERR'
>>
>> Case 1:
>> When dma_request_chan() fails, host->dma_chan_rx is an ERR_PTR(),
>> but it is reset to NULL before using PTR_ERR(), resulting in PTR_ERR(0).
>> This mistakenly returns 0 instead of the real error code.
>>
>> Case 2:
>> When devm_clk_get() fails, host->clk is an ERR_PTR() resulting in the similar
>> issue like case 1.
>>
>> Store the error code before nullifying the pointers in both the cases.
> 
> Why is the pointer set to NULL at all? This is in both cases memory that
> is freed directly afterwards (as `host` is devm managed). So I'd claim

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.

Am I missing something?

Thanks,
Khalid


> 
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 26d03352af63..404f78198252 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -652,10 +652,8 @@ 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;
> +	if (IS_ERR(host->clk))
>   		return PTR_ERR(host->clk);
> -	}
>   
>   	host->clkrate = clk_get_rate(host->clk);
>   
> @@ -704,11 +702,9 @@ 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;
> +	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)) {
> 
> is a superior patch.
> 
> Best regards
> Uwe