ARM: pxa: fix error handling in pxa2xx_drv_pcmcia_probe

PATCH RESEND Applied Axel Lin 5 participants 7 messages Dec 27, 2011 – Feb 2, 2012

Subsystems (2)

PXA2xx/PXA3xx support Pcmcia

lore

Share & tools

lore

Axel Lin

author

2011-12-27 08:25 UTC #0 lore raw

PATCH RESEND 1/1 ARM: pxa: fix error handling in pxa2xx_drv_pcmcia_probe

If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.

Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.

Signed-off-by Axel Lin <[email protected]>

---
 drivers/pcmcia/pxa2xx_base.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/pcmcia/pxa2xx_base.c +3 -9

index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 			goto err1;
 	}
 
-	if (ret) {
-		while (--i >= 0)
-			soc_pcmcia_remove_one(&sinfo->skt[i]);
-		kfree(sinfo);
-		clk_put(clk);
-	} else {
-		pxa2xx_configure_sockets(&dev->dev);
-		dev_set_drvdata(&dev->dev, sinfo);
-	}
+	pxa2xx_configure_sockets(&dev->dev);
+	dev_set_drvdata(&dev->dev, sinfo);
 
 	return 0;
 
 err1:
 	while (--i >= 0)
 		soc_pcmcia_remove_one(&sinfo->skt[i]);
+	clk_put(clk);
 	kfree(sinfo);
 err0:
 	return ret;

Signature · 1 line

1.7.5.4

Eric Miao

Acked-by on v1 1/1

2011-12-27 09:11 UTC #1 lore raw

Quoted 7 lines · Axel Lin

If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.
Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.
Signed-off-by Axel Lin <[email protected]>

Acked-by Eric Miao <[email protected]>

Quoted 38 lines

---
?drivers/pcmcia/pxa2xx_base.c | ? 12 +++---------
?1 files changed, 3 insertions(+), 9 deletions(-)
drivers/pcmcia/pxa2xx_base.c +2 -9
index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 ? ? ? ? ? ? ? ? ? ? ? ?goto err1;
 ? ? ? ?}
 
- ? ? ? if (ret) {
- ? ? ? ? ? ? ? while (--i >= 0)
- ? ? ? ? ? ? ? ? ? ? ? soc_pcmcia_remove_one(&sinfo->skt[i]);
- ? ? ? ? ? ? ? kfree(sinfo);
- ? ? ? ? ? ? ? clk_put(clk);
- ? ? ? } else {
- ? ? ? ? ? ? ? pxa2xx_configure_sockets(&dev->dev);
- ? ? ? ? ? ? ? dev_set_drvdata(&dev->dev, sinfo);
- ? ? ? }
+ ? ? ? pxa2xx_configure_sockets(&dev->dev);
+ ? ? ? dev_set_drvdata(&dev->dev, sinfo);
 
 ? ? ? ?return 0;
 
?err1:
? ? ? ?while (--i >= 0)
? ? ? ? ? ? ? ?soc_pcmcia_remove_one(&sinfo->skt[i]);
+ ? ? ? clk_put(clk);
? ? ? ?kfree(sinfo);
?err0:
? ? ? ?return ret;
--
1.7.5.4

Haojian Zhuang

Acked-by Eric Miao

2011-12-27 09:18 UTC #2 lore raw

________________________________________
From: Eric Miao [eric.y.miao at gmail.com]
Sent: Tuesday, December 27, 2011 5:11 PM
To: Axel Lin
Cc: linux-kernel at vger.kernel.org; Haojian Zhuang; Russell King; linux-arm-kernel at lists.infradead.org; linux-pcmcia at lists.infradead.org
Subject: Re: [PATCH RESEND] ARM: pxa: fix error handling in pxa2xx_drv_pcmcia_probe

Quoted 7 lines · Axel Lin

If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.
Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.
Signed-off-by Axel Lin <[email protected]>

Acked-by Eric Miao <[email protected]>

Quoted 34 lines

---
 drivers/pcmcia/pxa2xx_base.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/pcmcia/pxa2xx_base.c +3 ~~-10~~

index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
                       goto err1;
       }
 
-       if (ret) {
-               while (--i >= 0)
-                       soc_pcmcia_remove_one(&sinfo->skt[i]);
-               kfree(sinfo);
-               clk_put(clk);
-       } else {
-               pxa2xx_configure_sockets(&dev->dev);
-               dev_set_drvdata(&dev->dev, sinfo);
-       }
+       pxa2xx_configure_sockets(&dev->dev);
+       dev_set_drvdata(&dev->dev, sinfo);
 
       return 0;
 
 err1:
       while (--i >= 0)
               soc_pcmcia_remove_one(&sinfo->skt[i]);
+       clk_put(clk);
       kfree(sinfo);
 err0:
       return ret;
--

Quoted 4 lines

1.7.5.4

Wait a minute.

The original code will check whether pxa2xx_drv_pcmcia_add_one() successful. If it fails, we'll remove all devices that we registered.

But Axel removed these code. Why?

Thanks
Haojian

Eric Miao

2011-12-27 09:25 UTC #3 lore raw

Quoted 59 lines · Haojian Zhuang

________________________________________
From: Eric Miao [eric.y.miao at gmail.com]
Sent: Tuesday, December 27, 2011 5:11 PM
To: Axel Lin
Cc: linux-kernel at vger.kernel.org; Haojian Zhuang; Russell King; linux-arm-kernel at lists.infradead.org; linux-pcmcia at lists.infradead.org
Subject: Re: [PATCH RESEND] ARM: pxa: fix error handling in pxa2xx_drv_pcmcia_probe
Quoted 7 lines · Axel Lin
If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.
Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.
Signed-off-by Axel Lin <[email protected]>
Acked-by Eric Miao <[email protected]>
Quoted 27 lines
---
?drivers/pcmcia/pxa2xx_base.c | ? 12 +++---------
?1 files changed, 3 insertions(+), 9 deletions(-)
drivers/pcmcia/pxa2xx_base.c +2 -9
index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 ? ? ? ? ? ? ? ? ? ? ? ?goto err1;
 ? ? ? ?}
 
- ? ? ? if (ret) {
- ? ? ? ? ? ? ? while (--i >= 0)
- ? ? ? ? ? ? ? ? ? ? ? soc_pcmcia_remove_one(&sinfo->skt[i]);
- ? ? ? ? ? ? ? kfree(sinfo);
- ? ? ? ? ? ? ? clk_put(clk);
- ? ? ? } else {
- ? ? ? ? ? ? ? pxa2xx_configure_sockets(&dev->dev);
- ? ? ? ? ? ? ? dev_set_drvdata(&dev->dev, sinfo);
- ? ? ? }
+ ? ? ? pxa2xx_configure_sockets(&dev->dev);
+ ? ? ? dev_set_drvdata(&dev->dev, sinfo);
 
 ? ? ? ?return 0;
 
?err1:
Quoted 11 lines
? ? ? ?while (--i >= 0)
? ? ? ? ? ? ? ?soc_pcmcia_remove_one(&sinfo->skt[i]);
+ ? ? ? clk_put(clk);
? ? ? ?kfree(sinfo);
?err0:
? ? ? ?return ret;
--
1.7.5.4
Wait a minute.

Quoted 3 lines

The original code will check whether pxa2xx_drv_pcmcia_add_one() successful. If it fails, we'll remove all devices that we registered.
But Axel removed these code. Why?

If it fails, it will jump to err1 anyway. The check there is redundant.

Haojian Zhuang

maintainer Applied

2011-12-28 01:04 UTC #4 lore raw

Quoted 60 lines · Eric Miao

Quoted 59 lines · Haojian Zhuang
________________________________________
From: Eric Miao [eric.y.miao at gmail.com]
Sent: Tuesday, December 27, 2011 5:11 PM
To: Axel Lin
Cc: linux-kernel at vger.kernel.org; Haojian Zhuang; Russell King; linux-arm-kernel at lists.infradead.org; linux-pcmcia at lists.infradead.org
Subject: Re: [PATCH RESEND] ARM: pxa: fix error handling in pxa2xx_drv_pcmcia_probe
Quoted 7 lines · Axel Lin
If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.
Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.
Signed-off-by Axel Lin <[email protected]>
Acked-by Eric Miao <[email protected]>
Quoted 27 lines
---
?drivers/pcmcia/pxa2xx_base.c | ? 12 +++---------
?1 files changed, 3 insertions(+), 9 deletions(-)
drivers/pcmcia/pxa2xx_base.c +2 -9
index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct platform_device *dev)
 ? ? ? ? ? ? ? ? ? ? ? ?goto err1;
 ? ? ? ?}
 
- ? ? ? if (ret) {
- ? ? ? ? ? ? ? while (--i >= 0)
- ? ? ? ? ? ? ? ? ? ? ? soc_pcmcia_remove_one(&sinfo->skt[i]);
- ? ? ? ? ? ? ? kfree(sinfo);
- ? ? ? ? ? ? ? clk_put(clk);
- ? ? ? } else {
- ? ? ? ? ? ? ? pxa2xx_configure_sockets(&dev->dev);
- ? ? ? ? ? ? ? dev_set_drvdata(&dev->dev, sinfo);
- ? ? ? }
+ ? ? ? pxa2xx_configure_sockets(&dev->dev);
+ ? ? ? dev_set_drvdata(&dev->dev, sinfo);
 
 ? ? ? ?return 0;
 
?err1:
Quoted 11 lines
? ? ? ?while (--i >= 0)
? ? ? ? ? ? ? ?soc_pcmcia_remove_one(&sinfo->skt[i]);
+ ? ? ? clk_put(clk);
? ? ? ?kfree(sinfo);
?err0:
? ? ? ?return ret;
--
1.7.5.4
Wait a minute.

Quoted 6 lines

Quoted 3 lines
The original code will check whether pxa2xx_drv_pcmcia_add_one() successful. If it fails, we'll remove all devices that we registered.
But Axel removed these code. Why?
If it fails, it will jump to err1 anyway. The check there is redundant.

Applied

Axel Lin

author

2012-02-02 01:59 UTC #5 lore raw

Quoted 4 lines

Quoted 3 lines
If it fails, it will jump to err1 anyway. The check there is redundant.
Applied

Hi Haojian,
This patch is not (yet) upstream.
Could you help to check it again.

Thanks,
Axel

Marek Vasut

Acked-by on v1 1/1

2011-12-27 15:24 UTC #6 lore raw

In reply to Axel Lin

Quoted 30 lines

If pxa2xx_drv_pcmcia_add_one fails, it will go to err1 error path. Add a missing clk_put in the error path.
Checking the ret value after the for loop is redundant, it is always false. Thus remove the redundant checking.
Signed-off-by Axel Lin <[email protected]>
---
 drivers/pcmcia/pxa2xx_base.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)
drivers/pcmcia/pxa2xx_base.c +2 -9
index a87e272..64d433e 100644
@@ -328,21 +328,15 @@ static int pxa2xx_drv_pcmcia_probe(struct
 platform_device *dev) goto err1;
 	}
 
-	if (ret) {
-		while (--i >= 0)
-			soc_pcmcia_remove_one(&sinfo->skt[i]);
-		kfree(sinfo);
-		clk_put(clk);
-	} else {
-		pxa2xx_configure_sockets(&dev->dev);
-		dev_set_drvdata(&dev->dev, sinfo);
-	}
+	pxa2xx_configure_sockets(&dev->dev);
+	dev_set_drvdata(&dev->dev, sinfo);

drivers/pcmcia/pxa2xx_base.c static int pxa2xx_drv_pcmcia_probe(struct · line 342

Quoted 10 lines

drivers/pcmcia/pxa2xx_base.c +1 -0

 
 	return 0;
 
 err1:
 	while (--i >= 0)
 		soc_pcmcia_remove_one(&sinfo->skt[i]);
+	clk_put(clk);
 	kfree(sinfo);
 err0:
 	return ret;

Makes sense

Acked-by Marek Vasut <[email protected]>