| Message ID | 4D9ACC8B.8070407@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Apr 05, 2011 at 05:02:19PM +0900, Jaehoon Chung wrote: > 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 <jh80.chung@samsung.com> > Signed-off-by: Kyungin Park <kyungmin.park@samsung.com> > --- > drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++-- > 1 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 9e15f41..1768ffb 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -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); As I asked in my last review: Why do you remove the host when add_host failed? Doesn't that crash? > + > #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; > + } The if-condition is not needed, kfree is NULL-safe. > + > return ret; > } >
Hi Wolfram.. 2011/4/9 Wolfram Sang <w.sang@pengutronix.de>: > Hi, > > On Tue, Apr 05, 2011 at 05:02:19PM +0900, Jaehoon Chung wrote: >> 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 <jh80.chung@samsung.com> >> Signed-off-by: Kyungin Park <kyungmin.park@samsung.com> >> --- >> drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++-- >> 1 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 9e15f41..1768ffb 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -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); > > As I asked in my last review: Why do you remove the host when add_host failed? > Doesn't that crash? > >> + >> #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; >> + } > > The if-condition is not needed, kfree is NULL-safe. I'll remove that..Thanks for your reviewed Regards, Jaehoon Chung > >> + >> 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----- > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 9e15f41..1768ffb 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -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; }