mmc: sdhci: add error checking for mmc_add_host

PATCH In review Jaehoon Chung 3 participants 3 messages Apr 5, 2011 – Apr 9, 2011

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Jaehoon Chung

author

2011-04-05 08:03 UTC #0 lore raw

PATCH v3 1/2 mmc: sdhci: add error checking for mmc_add_host

Sometimes we can't add the device,but we didn't check any error status. Need to check error status for mmc_add_host.

Signed-off-by Jaehoon Chung <[email protected]>

Signed-off-by Kyungin Park <[email protected]>

---
 drivers/mmc/host/sdhci.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

drivers/mmc/host/sdhci.c +18 -2

index 9e15f41..1768ffb 100644
@@ -2025,7 +2025,9 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	mmiowb();
 
-	mmc_add_host(mmc);
+	ret = mmc_add_host(mmc);
+	if (unlikely(ret))
+		goto err_free_mmc;
 
 	printk(KERN_INFO "%s: SDHCI controller on %s [%s] using %s\n",
 		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),

@@ -2036,15 +2038,29 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	return 0;
 
+err_free_mmc:
+	mmc_remove_host(host);
+
 #ifdef SDHCI_USE_LEDS_CLASS
+	led_classdev_ungregister(&host->led);
 reset:
+#endif
 	sdhci_reset(host, SDHCI_RESET_ALL);
 	free_irq(host->irq, host);
-#endif
+	del_timer_sync(&host->timer);
+
 untasklet:
 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
 
+	if (host->flags & SDHCI_USE_ADMA) {
+		kfree(host->adma_desc);
+		kfree(host->align_buffer);
+
+		host->adma_desc = NULL;
+		host->align_buffer = NULL;
+	}
+
 	return ret;
 }

Wolfram Sang

2011-04-08 18:32 UTC #1 lore raw

Hi,

Quoted 25 lines · Jaehoon Chung

Sometimes we can't add the device,but we didn't check any error status. Need to check error status for mmc_add_host.
Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungin Park <[email protected]>
---
 drivers/mmc/host/sdhci.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -1
index 9e15f41..1768ffb 100644
@@ -2025,7 +2025,9 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	mmiowb();
 
-	mmc_add_host(mmc);
+	ret = mmc_add_host(mmc);
+	if (unlikely(ret))
+		goto err_free_mmc;
 
 	printk(KERN_INFO "%s: SDHCI controller on %s [%s] using %s\n",
 		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2042

Quoted 5 lines

drivers/mmc/host/sdhci.c +2 -0

@@ -2036,15 +2038,29 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	return 0;
 
+err_free_mmc:
+	mmc_remove_host(host);

As I asked in my last review: Why do you remove the host when add_host failed? Doesn't that crash?

Quoted 11 lines

drivers/mmc/host/sdhci.c +5 -1

+
 #ifdef SDHCI_USE_LEDS_CLASS
+	led_classdev_ungregister(&host->led);
 reset:
+#endif
 	sdhci_reset(host, SDHCI_RESET_ALL);
 	free_irq(host->irq, host);
-#endif
+	del_timer_sync(&host->timer);
+
 untasklet:

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2062

Quoted 10 lines

drivers/mmc/host/sdhci.c +7 -0

 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
 
+	if (host->flags & SDHCI_USE_ADMA) {
+		kfree(host->adma_desc);
+		kfree(host->align_buffer);
+
+		host->adma_desc = NULL;
+		host->align_buffer = NULL;
+	}

The if-condition is not needed, kfree is NULL-safe.

Quoted 4 lines

drivers/mmc/host/sdhci.c +1 -0
+
 	return ret;
 }
 

Signature · 2 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Jae hoon Chung

2011-04-09 11:49 UTC #2 lore raw

Hi Wolfram..

2011/4/9 Wolfram Sang <[email protected]>:

Quoted 51 lines

Hi,
Quoted 25 lines · Jaehoon Chung
Sometimes we can't add the device,but we didn't check any error status. Need to check error status for mmc_add_host.
Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungin Park <[email protected]>
---
drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +3 -1
index 9e15f41..1768ffb 100644
@@ -2025,7 +2025,9 @@ int sdhci_add_host(struct sdhci_host *host)
 
       mmiowb();
 
-     mmc_add_host(mmc);
+     ret = mmc_add_host(mmc);
+     if (unlikely(ret))
+             goto err_free_mmc;
 
       printk(KERN_INFO "%s: SDHCI controller on %s [%s] using %s\n",
               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2042
Quoted 5 lines
drivers/mmc/host/sdhci.c +2 -0
@@ -2036,15 +2038,29 @@ int sdhci_add_host(struct sdhci_host *host)
 
       return 0;
 
+err_free_mmc:
+     mmc_remove_host(host);
As I asked in my last review: Why do you remove the host when add_host failed? Doesn't that crash?
Quoted 14 lines
drivers/mmc/host/sdhci.c +2 -0
+
  #ifdef SDHCI_USE_LEDS_CLASS
+     led_classdev_ungregister(&host->led);
reset:
+#endif
sdhci_reset(host, SDHCI_RESET_ALL);
free_irq(host->irq, host);
drivers/mmc/host/sdhci.c +2 -1
-#endif
+     del_timer_sync(&host->timer);
+
untasklet:
tasklet_kill(&host->card_tasklet);
tasklet_kill(&host->finish_tasklet);

Quoted 9 lines

Quoted 7 lines
+ if (host->flags & SDHCI_USE_ADMA) {
+ kfree(host->adma_desc);
+ kfree(host->align_buffer);
+
+ host->adma_desc = NULL;
+ host->align_buffer = NULL;
+ }
The if-condition is not needed, kfree is NULL-safe.

I'll remove that..Thanks for your reviewed

Regards,
Jaehoon Chung

Quoted 19 lines

Quoted 5 lines
+
return ret;
}
--
Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAk2fVJ0ACgkQD27XaX1/VRsZFwCgsgZ567Z5EP981NF4/cEXG9z/ HvAAoMFOud43MnfeKNDX2/HCBKBdVnJu
=RTub
-----END PGP SIGNATURE-----