[PATCH] mmc: sdio: Keep card runtime resumed while adding function devices

[PATCH] mmc: sdio: Keep card runtime resumed while adding function devices

[Adrian Hunter]

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(-)

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);
-- 
1.9.1

Re: [PATCH] mmc: sdio: Keep card runtime resumed while adding function devices

[Ulf Hansson]

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

Re: [PATCH] mmc: sdio: Keep card runtime resumed while adding function devices

[Adrian Hunter]

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
>