| Message ID | 1441975315-30387-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Ulf, On Friday 11 September 2015 06:11 PM, Ulf Hansson wrote: > As mmc_claim_host() invokes pm_runtime_get_sync() for the mmc host device, > it's important that the host is kept claimed for *all* accesses to it via > the host_ops callbacks. > > In some code paths for SDIO, particularly related to the PM support, > mmc_power_off|up() is invoked without keeping the host claimed. Let's fix > these. > > Moreover, mmc_start|stop_host() also invokes mmc_power_off|up() without > claiming the host, let's fix these as well. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/core/core.c | 6 ++++++ > drivers/mmc/core/sdio.c | 20 +++++++++++++++----- > 2 files changed, 21 insertions(+), 5 deletions(-) > . . <snip> . . > int mmc_power_save_host(struct mmc_host *host) > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > index b91abed..95bc101 100644 > --- a/drivers/mmc/core/sdio.c > +++ b/drivers/mmc/core/sdio.c > @@ -897,11 +897,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host) > */ > static int mmc_sdio_suspend(struct mmc_host *host) > { > - if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) { > - mmc_claim_host(host); > + mmc_claim_host(host); > + > + if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) > sdio_disable_wide(host->card); > - mmc_release_host(host); > - } > > if (!mmc_card_keep_power(host)) { > mmc_power_off(host); > @@ -910,6 +909,8 @@ static int mmc_sdio_suspend(struct mmc_host *host) > mmc_retune_needed(host); > } > > + mmc_release_host(host); > + > return 0; > } > > @@ -1018,15 +1019,24 @@ out: > static int mmc_sdio_runtime_suspend(struct mmc_host *host) > { > /* No references to the card, cut the power to it. */ > + mmc_claim_host(host); > mmc_power_off(host); > + mmc_release_host(host); > + > return 0; > } > > static int mmc_sdio_runtime_resume(struct mmc_host *host) > { > + int ret; > + > /* Restore power and re-initialize. */ > + mmc_claim_host(host); > mmc_power_up(host, host->card->ocr); > - return mmc_sdio_power_restore(host); > + ret = mmc_sdio_power_restore(host); IIUC, this will cause mmc_claim_host to be invoked twice since mmc_sdio_power_restore also invokes mmc_claim_host. I'm not sure if this will have undesired side effects. Cheers Kishon -- 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
On 11 September 2015 at 15:22, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi Ulf, > > On Friday 11 September 2015 06:11 PM, Ulf Hansson wrote: >> As mmc_claim_host() invokes pm_runtime_get_sync() for the mmc host device, >> it's important that the host is kept claimed for *all* accesses to it via >> the host_ops callbacks. >> >> In some code paths for SDIO, particularly related to the PM support, >> mmc_power_off|up() is invoked without keeping the host claimed. Let's fix >> these. >> >> Moreover, mmc_start|stop_host() also invokes mmc_power_off|up() without >> claiming the host, let's fix these as well. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/mmc/core/core.c | 6 ++++++ >> drivers/mmc/core/sdio.c | 20 +++++++++++++++----- >> 2 files changed, 21 insertions(+), 5 deletions(-) >> > . > . > <snip> > . > . >> int mmc_power_save_host(struct mmc_host *host) >> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c >> index b91abed..95bc101 100644 >> --- a/drivers/mmc/core/sdio.c >> +++ b/drivers/mmc/core/sdio.c >> @@ -897,11 +897,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host) >> */ >> static int mmc_sdio_suspend(struct mmc_host *host) >> { >> - if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) { >> - mmc_claim_host(host); >> + mmc_claim_host(host); >> + >> + if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) >> sdio_disable_wide(host->card); >> - mmc_release_host(host); >> - } >> >> if (!mmc_card_keep_power(host)) { >> mmc_power_off(host); >> @@ -910,6 +909,8 @@ static int mmc_sdio_suspend(struct mmc_host *host) >> mmc_retune_needed(host); >> } >> >> + mmc_release_host(host); >> + >> return 0; >> } >> >> @@ -1018,15 +1019,24 @@ out: >> static int mmc_sdio_runtime_suspend(struct mmc_host *host) >> { >> /* No references to the card, cut the power to it. */ >> + mmc_claim_host(host); >> mmc_power_off(host); >> + mmc_release_host(host); >> + >> return 0; >> } >> >> static int mmc_sdio_runtime_resume(struct mmc_host *host) >> { >> + int ret; >> + >> /* Restore power and re-initialize. */ >> + mmc_claim_host(host); >> mmc_power_up(host, host->card->ocr); >> - return mmc_sdio_power_restore(host); >> + ret = mmc_sdio_power_restore(host); > > IIUC, this will cause mmc_claim_host to be invoked twice since > mmc_sdio_power_restore also invokes mmc_claim_host. I'm not sure if this > will have undesired side effects. That's not a problem, mmc_claim_host() handles that. As long it's the same running context that tries to claim the host it will allow it. Kind regards Uffe -- 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
Hi, On Friday 11 September 2015 07:29 PM, Ulf Hansson wrote: > On 11 September 2015 at 15:22, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> Hi Ulf, >> >> On Friday 11 September 2015 06:11 PM, Ulf Hansson wrote: >>> As mmc_claim_host() invokes pm_runtime_get_sync() for the mmc host device, >>> it's important that the host is kept claimed for *all* accesses to it via >>> the host_ops callbacks. >>> >>> In some code paths for SDIO, particularly related to the PM support, >>> mmc_power_off|up() is invoked without keeping the host claimed. Let's fix >>> these. >>> >>> Moreover, mmc_start|stop_host() also invokes mmc_power_off|up() without >>> claiming the host, let's fix these as well. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/mmc/core/core.c | 6 ++++++ >>> drivers/mmc/core/sdio.c | 20 +++++++++++++++----- >>> 2 files changed, 21 insertions(+), 5 deletions(-) >>> >> . >> . >> <snip> >> . >> . >>> int mmc_power_save_host(struct mmc_host *host) >>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c >>> index b91abed..95bc101 100644 >>> --- a/drivers/mmc/core/sdio.c >>> +++ b/drivers/mmc/core/sdio.c >>> @@ -897,11 +897,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host) >>> */ >>> static int mmc_sdio_suspend(struct mmc_host *host) >>> { >>> - if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) { >>> - mmc_claim_host(host); >>> + mmc_claim_host(host); >>> + >>> + if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) >>> sdio_disable_wide(host->card); >>> - mmc_release_host(host); >>> - } >>> >>> if (!mmc_card_keep_power(host)) { >>> mmc_power_off(host); >>> @@ -910,6 +909,8 @@ static int mmc_sdio_suspend(struct mmc_host *host) >>> mmc_retune_needed(host); >>> } >>> >>> + mmc_release_host(host); >>> + >>> return 0; >>> } >>> >>> @@ -1018,15 +1019,24 @@ out: >>> static int mmc_sdio_runtime_suspend(struct mmc_host *host) >>> { >>> /* No references to the card, cut the power to it. */ >>> + mmc_claim_host(host); >>> mmc_power_off(host); >>> + mmc_release_host(host); >>> + >>> return 0; >>> } >>> >>> static int mmc_sdio_runtime_resume(struct mmc_host *host) >>> { >>> + int ret; >>> + >>> /* Restore power and re-initialize. */ >>> + mmc_claim_host(host); >>> mmc_power_up(host, host->card->ocr); >>> - return mmc_sdio_power_restore(host); >>> + ret = mmc_sdio_power_restore(host); >> >> IIUC, this will cause mmc_claim_host to be invoked twice since >> mmc_sdio_power_restore also invokes mmc_claim_host. I'm not sure if this >> will have undesired side effects. > > That's not a problem, mmc_claim_host() handles that. As long it's the > same running context that tries to claim the host it will allow it. Alright then. You can add my Acked-by: Kishon Vijay Abraham I <kishon@ti.com> Cheers Kishon -- 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/core/core.c b/drivers/mmc/core/core.c index 0520064..ed5f4ad 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2631,10 +2631,14 @@ void mmc_start_host(struct mmc_host *host) host->f_init = max(freqs[0], host->f_min); host->rescan_disable = 0; host->ios.power_mode = MMC_POWER_UNDEFINED; + + mmc_claim_host(host); if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP) mmc_power_off(host); else mmc_power_up(host, host->ocr_avail); + mmc_release_host(host); + mmc_gpiod_request_cd_irq(host); _mmc_detect_change(host, 0, false); } @@ -2672,7 +2676,9 @@ void mmc_stop_host(struct mmc_host *host) BUG_ON(host->card); + mmc_claim_host(host); mmc_power_off(host); + mmc_release_host(host); } int mmc_power_save_host(struct mmc_host *host) diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index b91abed..95bc101 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -897,11 +897,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host) */ static int mmc_sdio_suspend(struct mmc_host *host) { - if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) { - mmc_claim_host(host); + mmc_claim_host(host); + + if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) sdio_disable_wide(host->card); - mmc_release_host(host); - } if (!mmc_card_keep_power(host)) { mmc_power_off(host); @@ -910,6 +909,8 @@ static int mmc_sdio_suspend(struct mmc_host *host) mmc_retune_needed(host); } + mmc_release_host(host); + return 0; } @@ -1018,15 +1019,24 @@ out: static int mmc_sdio_runtime_suspend(struct mmc_host *host) { /* No references to the card, cut the power to it. */ + mmc_claim_host(host); mmc_power_off(host); + mmc_release_host(host); + return 0; } static int mmc_sdio_runtime_resume(struct mmc_host *host) { + int ret; + /* Restore power and re-initialize. */ + mmc_claim_host(host); mmc_power_up(host, host->card->ocr); - return mmc_sdio_power_restore(host); + ret = mmc_sdio_power_restore(host); + mmc_release_host(host); + + return ret; } static int mmc_sdio_reset(struct mmc_host *host)
As mmc_claim_host() invokes pm_runtime_get_sync() for the mmc host device, it's important that the host is kept claimed for *all* accesses to it via the host_ops callbacks. In some code paths for SDIO, particularly related to the PM support, mmc_power_off|up() is invoked without keeping the host claimed. Let's fix these. Moreover, mmc_start|stop_host() also invokes mmc_power_off|up() without claiming the host, let's fix these as well. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/core.c | 6 ++++++ drivers/mmc/core/sdio.c | 20 +++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)