| Message ID | 1495107722-13444-1-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18 May 2017 at 13:42, Adrian Hunter <adrian.hunter@intel.com> wrote: > Drivers core will runtime suspend a device with no driver. That means the > SDIO card will be runtime suspended as soon as it is added. It is then > runtime resumed to add each function. That is entirely pointless, so add > pm runtime get/put to keep the SDIO card runtime resumed until the function > devices have been added. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> Adrian, apologize for the delay! > --- > drivers/mmc/core/sdio.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > index fae732c870a9..97bedde0610c 100644 > --- a/drivers/mmc/core/sdio.c > +++ b/drivers/mmc/core/sdio.c > @@ -1110,6 +1110,8 @@ int mmc_attach_sdio(struct mmc_host *host) > if (err) > goto remove; > > + pm_runtime_get_noresume(&card->dev); Please move this above pm_runtime_set_active(), just to be really sure the device remains active. Thus you also need to update the error path. > + > /* > * Enable runtime PM for this card > */ > @@ -1129,7 +1131,7 @@ int mmc_attach_sdio(struct mmc_host *host) > for (i = 0; i < funcs; i++, card->sdio_funcs++) { > err = sdio_init_func(host->card, i + 1); > if (err) > - goto remove; > + goto remove_get; > > /* > * Enable Runtime PM for this func (if supported) > @@ -1156,6 +1158,10 @@ int mmc_attach_sdio(struct mmc_host *host) > } > > mmc_claim_host(host); > + > + if (host->caps & MMC_CAP_POWER_OFF_CARD) > + pm_runtime_put(&card->dev); > + > return 0; > > > @@ -1163,6 +1169,9 @@ int mmc_attach_sdio(struct mmc_host *host) > /* Remove without lock if the device has been added. */ > mmc_sdio_remove(host); > mmc_claim_host(host); > +remove_get: > + if (host->caps & MMC_CAP_POWER_OFF_CARD) > + pm_runtime_put(&card->dev); Looking in the error path, I also notice that there is a pm_runtime_disable missing. Would you mind fixing that (as a separate change of course) as well? > remove: > /* And with lock if it hasn't been added. */ > mmc_release_host(host); > -- > 1.9.1 > 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
On 07/06/17 17:45, Ulf Hansson wrote: > On 18 May 2017 at 13:42, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Drivers core will runtime suspend a device with no driver. That means the >> SDIO card will be runtime suspended as soon as it is added. It is then >> runtime resumed to add each function. That is entirely pointless, so add >> pm runtime get/put to keep the SDIO card runtime resumed until the function >> devices have been added. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > Adrian, apologize for the delay! > >> --- >> drivers/mmc/core/sdio.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c >> index fae732c870a9..97bedde0610c 100644 >> --- a/drivers/mmc/core/sdio.c >> +++ b/drivers/mmc/core/sdio.c >> @@ -1110,6 +1110,8 @@ int mmc_attach_sdio(struct mmc_host *host) >> if (err) >> goto remove; >> >> + pm_runtime_get_noresume(&card->dev); > > Please move this above pm_runtime_set_active(), just to be really sure > the device remains active. Thus you also need to update the error > path. As you wish, but even if there was somehow another code path that could get a reference to this device, it couldn't runtime suspend it because it is not runtime enabled. > >> + >> /* >> * Enable runtime PM for this card >> */ >> @@ -1129,7 +1131,7 @@ int mmc_attach_sdio(struct mmc_host *host) >> for (i = 0; i < funcs; i++, card->sdio_funcs++) { >> err = sdio_init_func(host->card, i + 1); >> if (err) >> - goto remove; >> + goto remove_get; >> >> /* >> * Enable Runtime PM for this func (if supported) >> @@ -1156,6 +1158,10 @@ int mmc_attach_sdio(struct mmc_host *host) >> } >> >> mmc_claim_host(host); >> + >> + if (host->caps & MMC_CAP_POWER_OFF_CARD) >> + pm_runtime_put(&card->dev); >> + >> return 0; >> >> >> @@ -1163,6 +1169,9 @@ int mmc_attach_sdio(struct mmc_host *host) >> /* Remove without lock if the device has been added. */ >> mmc_sdio_remove(host); >> mmc_claim_host(host); >> +remove_get: >> + if (host->caps & MMC_CAP_POWER_OFF_CARD) >> + pm_runtime_put(&card->dev); I notice this is wrong here - it needs to be before mmc_sdio_remove() because mmc_sdio_remove() deletes the devices. > > Looking in the error path, I also notice that there is a > pm_runtime_disable missing. Would you mind fixing that (as a separate > change of course) as well? We can't call pm_runtime_disable() after mmc_sdio_remove() because mmc_sdio_remove() deletes the devices. device_del() anyway takes care of runtime pm (i.e. ends up calling __pm_runtime_disable()). If we call pm_runtime_disable() before mmc_sdio_remove() then it results in the devices staying in the pm state they were in at that point. The ->remove() callbacks expect that to be "active" but AFAICT they in turn leave the devices "suspended", so we would end up changing the current behaviour. So it looks to me like we don't need to call pm_runtime_disable() and we can't call it here without side-effects. > >> remove: >> /* And with lock if it hasn't been added. */ >> mmc_release_host(host); >> -- >> 1.9.1 >> > > 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
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index fae732c870a9..97bedde0610c 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -1110,6 +1110,8 @@ int mmc_attach_sdio(struct mmc_host *host) if (err) goto remove; + pm_runtime_get_noresume(&card->dev); + /* * Enable runtime PM for this card */ @@ -1129,7 +1131,7 @@ int mmc_attach_sdio(struct mmc_host *host) for (i = 0; i < funcs; i++, card->sdio_funcs++) { err = sdio_init_func(host->card, i + 1); if (err) - goto remove; + goto remove_get; /* * Enable Runtime PM for this func (if supported) @@ -1156,6 +1158,10 @@ int mmc_attach_sdio(struct mmc_host *host) } mmc_claim_host(host); + + if (host->caps & MMC_CAP_POWER_OFF_CARD) + pm_runtime_put(&card->dev); + return 0; @@ -1163,6 +1169,9 @@ int mmc_attach_sdio(struct mmc_host *host) /* Remove without lock if the device has been added. */ mmc_sdio_remove(host); mmc_claim_host(host); +remove_get: + if (host->caps & MMC_CAP_POWER_OFF_CARD) + pm_runtime_put(&card->dev); remove: /* And with lock if it hasn't been added. */ mmc_release_host(host);
Drivers core will runtime suspend a device with no driver. That means the SDIO card will be runtime suspended as soon as it is added. It is then runtime resumed to add each function. That is entirely pointless, so add pm runtime get/put to keep the SDIO card runtime resumed until the function devices have been added. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/sdio.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)