[PATCH RFC 0/4] mmc: sdio: To support hibernation add capability to skip SDIO reset at scan

[PATCH RFC 0/4] mmc: sdio: To support hibernation add capability to skip SDIO reset at scan

[Adrian Hunter]

Hi

Before a hibernation image can be restored, the system must boot which
might result in the mmc core performing a SDIO reset on any SDIO cards.
That is a problem if the state of the SDIO function is to be preserved.
The patches here contrive to avoid that SDIO reset.

The first patch is a general improvement which avoids unnecessary runtime
suspend / resume, and therefore also SDIO reset.

The second patch adds a new capability to void SDIO reset at rescan time.

The final patches select the new capability based on whether there is an
ACPI child node with a _S4W method which is used to infer that the device
(SDIO function) is capable of waking the system from S4 (hibernation).

This patch set is sent as RFC and any feedback on this approach would be
appreciated.


Adrian Hunter (4):
      mmc: sdio: Keep card runtime resumed while adding function devices
      mmc: sdio: Add capability to skip SDIO reset at scan
      mmc: sdhci-acpi: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)
      mmc: sdhci-pci: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

 drivers/mmc/core/core.c           |  6 +++++-
 drivers/mmc/core/sdio.c           | 11 ++++++++++-
 drivers/mmc/host/sdhci-acpi.c     | 17 ++++++++++++++++-
 drivers/mmc/host/sdhci-pci-core.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h          |  1 +
 5 files changed, 66 insertions(+), 3 deletions(-)


Regards
Adrian

[PATCH RFC 1/4] 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.

This also benefits the hibernation use-case whereby we want to avoid the
SDIO reset that is done as part of runtime resume. In that case, the SDIO
card state is preserved at boot time by avoiding loading any function
drivers before restoring the hibernation image.

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

[PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

The SDIO card state might be being preserved during hibernation, for
example a SDIO wifi card supporting WOWLAN. That state will be lost if an
SDIO reset is done. One way to avoid that would be to build mmc core as a
module and simply not load it until after attempting to restore the
hibernation image. However that won't work if the hibernation image is
stored on eMMC which, of course, requires mmc core.

It is assumed on such systems that the platform will power cycle the SDIO
card or not as necessary so that the SDIO reset is not needed. Add a
capability flag to reflect that and use it to skip the SDIO reset at scan
time.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c  | 6 +++++-
 include/linux/mmc/host.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6987976252ad..178e23bf0c30 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 	 * if the card is being re-initialized, just send it.  CMD52
 	 * should be ignored by SD/eMMC cards.
 	 * Skip it if we already know that we do not support SDIO commands
+	 * Also skip it if we know this host controller has a SDIO card that
+	 * needs to be able to restore from hibernation without losing the card
+	 * state e.g. an SDIO wifi card supporting WOWLAN.
 	 */
-	if (!(host->caps2 & MMC_CAP2_NO_SDIO))
+	if (!(host->caps2 & MMC_CAP2_NO_SDIO) &&
+	    !(host->caps2 & MMC_CAP2_NO_SDIO_RESET))
 		sdio_reset(host);
 
 	mmc_go_idle(host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 78c544e296cd..187a7ba41364 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -281,6 +281,7 @@ struct mmc_host {
 	u32			caps2;		/* More host capabilities */
 
 #define MMC_CAP2_BOOTPART_NOACC	(1 << 0)	/* Boot partition no access */
+#define MMC_CAP2_NO_SDIO_RESET	(1 << 1)	/* Do not SDIO reset at scan */
 #define MMC_CAP2_FULL_PWR_CYCLE	(1 << 2)	/* Can do full power cycle */
 #define MMC_CAP2_HS200_1_8V_SDR	(1 << 5)        /* can support */
 #define MMC_CAP2_HS200_1_2V_SDR	(1 << 6)        /* can support */
-- 
1.9.1

[PATCH RFC 3/4] mmc: sdhci-acpi: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

[Adrian Hunter]

SDIO reset interferes with a SDIO function driver's restore from
hibernation. Set MMC_CAP2_NO_SDIO_RESET if a child node has _S4W method
which indicates a capability to wake from S4 (hibernate).

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index c6a9a1bfaa22..e053a45db6b1 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -374,6 +374,14 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
 	return NULL;
 }
 
+static bool sdhci_acpi_child_has_s4w(struct acpi_device *child)
+{
+	acpi_handle handle = child->handle;
+	unsigned long long ret;
+
+	return ACPI_SUCCESS(acpi_evaluate_integer(handle, "_S4W", NULL, &ret));
+}
+
 static int sdhci_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -382,6 +390,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
+	bool child_has_s4w = false;
 	resource_size_t len;
 	const char *hid;
 	const char *uid;
@@ -393,8 +402,11 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	/* Power on the SDHCI controller and its children */
 	acpi_device_fix_up_power(device);
 	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
+		if (child->status.present && child->status.enabled) {
 			acpi_device_fix_up_power(child);
+			if (sdhci_acpi_child_has_s4w(child))
+				child_has_s4w = true;
+		}
 
 	if (acpi_bus_get_status(device) || !device->status.present)
 		return -ENODEV;
@@ -439,6 +451,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	if (child_has_s4w)
+		host->mmc->caps2 |= MMC_CAP2_NO_SDIO_RESET;
+
 	if (c->slot) {
 		if (c->slot->probe_slot) {
 			err = c->slot->probe_slot(pdev, hid, uid);
-- 
1.9.1

[PATCH RFC 4/4] mmc: sdhci-pci: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

[Adrian Hunter]

SDIO reset interferes with a SDIO function driver's restore from
hibernation. Set MMC_CAP2_NO_SDIO_RESET if a child node has _S4W method
which indicates a capability to wake from S4 (hibernate).

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-pci-core.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 57d6f50b73dc..cc9fc7d54570 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -597,11 +597,42 @@ static int ni_set_max_freq(struct sdhci_pci_slot *slot)
 
 	return 0;
 }
+
+static bool __sdhci_pci_child_has_s4w(struct acpi_device *child)
+{
+	acpi_handle handle = child->handle;
+	unsigned long long ret;
+
+	return ACPI_SUCCESS(acpi_evaluate_integer(handle, "_S4W", NULL, &ret));
+}
+
+static bool sdhci_pci_child_has_s4w(struct sdhci_pci_slot *slot)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&slot->chip->pdev->dev);
+	struct acpi_device *child;
+	bool child_has_s4w = false;
+
+	if (!adev)
+		return false;
+
+	list_for_each_entry(child, &adev->children, node)
+		if (child->status.present && child->status.enabled) {
+			if (__sdhci_pci_child_has_s4w(child))
+				child_has_s4w = true;
+		}
+
+	return child_has_s4w;
+}
 #else
 static inline int ni_set_max_freq(struct sdhci_pci_slot *slot)
 {
 	return 0;
 }
+
+static bool sdhci_pci_child_has_s4w(struct sdhci_pci_slot *slot)
+{
+	return false;
+}
 #endif
 
 static int ni_byt_sdio_probe_slot(struct sdhci_pci_slot *slot)
@@ -1960,6 +1991,9 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
 
 	host->ioaddr = pcim_iomap_table(pdev)[bar];
 
+	if (sdhci_pci_child_has_s4w(slot))
+		host->mmc->caps2 |= MMC_CAP2_NO_SDIO_RESET;
+
 	if (chip->fixes && chip->fixes->probe_slot) {
 		ret = chip->fixes->probe_slot(slot);
 		if (ret)
-- 
1.9.1

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Ulf Hansson]

On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> The SDIO card state might be being preserved during hibernation, for
> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
> SDIO reset is done. One way to avoid that would be to build mmc core as a
> module and simply not load it until after attempting to restore the
> hibernation image. However that won't work if the hibernation image is
> stored on eMMC which, of course, requires mmc core.

I don't follow here. Are you saying the SDIO card is kept powered in
hibernation, as to be able to support WOWLAN, right?

Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
That should never happen, unless something is broken of course.

Kind regards
Uffe

>
> It is assumed on such systems that the platform will power cycle the SDIO
> card or not as necessary so that the SDIO reset is not needed. Add a
> capability flag to reflect that and use it to skip the SDIO reset at scan
> time.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/core.c  | 6 +++++-
>  include/linux/mmc/host.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6987976252ad..178e23bf0c30 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>          * if the card is being re-initialized, just send it.  CMD52
>          * should be ignored by SD/eMMC cards.
>          * Skip it if we already know that we do not support SDIO commands
> +        * Also skip it if we know this host controller has a SDIO card that
> +        * needs to be able to restore from hibernation without losing the card
> +        * state e.g. an SDIO wifi card supporting WOWLAN.
>          */
> -       if (!(host->caps2 & MMC_CAP2_NO_SDIO))
> +       if (!(host->caps2 & MMC_CAP2_NO_SDIO) &&
> +           !(host->caps2 & MMC_CAP2_NO_SDIO_RESET))
>                 sdio_reset(host);
>
>         mmc_go_idle(host);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 78c544e296cd..187a7ba41364 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -281,6 +281,7 @@ struct mmc_host {
>         u32                     caps2;          /* More host capabilities */
>
>  #define MMC_CAP2_BOOTPART_NOACC        (1 << 0)        /* Boot partition no access */
> +#define MMC_CAP2_NO_SDIO_RESET (1 << 1)        /* Do not SDIO reset at scan */
>  #define MMC_CAP2_FULL_PWR_CYCLE        (1 << 2)        /* Can do full power cycle */
>  #define MMC_CAP2_HS200_1_8V_SDR        (1 << 5)        /* can support */
>  #define MMC_CAP2_HS200_1_2V_SDR        (1 << 6)        /* can support */
> --
> 1.9.1
>

Re: [PATCH RFC 1/4] mmc: sdio: Keep card runtime resumed while adding function devices

[Ulf Hansson]

On 21 April 2017 at 12:08, 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

I totally agree! You are touching the part of the PM code for SDIO
that is severely broken in my view.

The mmc core shouldn't mess with runtime PM for func device at all,
except setting up the parent-child relationship. Instead runtime PM
for the func device should be solely handled by the func driver.

> pm runtime get/put to keep the SDIO card runtime resumed until the function
> devices have been added.
>
> This also benefits the hibernation use-case whereby we want to avoid the
> SDIO reset that is done as part of runtime resume. In that case, the SDIO
> card state is preserved at boot time by avoiding loading any function
> drivers before restoring the hibernation image.

This seems like a different issue. I guess this is because the func
driver don't implement the PM callbacks, which makes the mmc core to
remove the sdio card at mmc_pm_notify().

The proper solution here is to make the sdio func driver to implement
the PM callbacks.

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

Overall this looks okay to me. Except for the changelog that is a bit
miss-leading.

Kind regards
Uffe

Re: [PATCH RFC 3/4] mmc: sdhci-acpi: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

[Rafael J. Wysocki]

On Fri, Apr 21, 2017 at 12:08 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> SDIO reset interferes with a SDIO function driver's restore from
> hibernation. Set MMC_CAP2_NO_SDIO_RESET if a child node has _S4W method
> which indicates a capability to wake from S4 (hibernate).
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci-acpi.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index c6a9a1bfaa22..e053a45db6b1 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -374,6 +374,14 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>         return NULL;
>  }
>
> +static bool sdhci_acpi_child_has_s4w(struct acpi_device *child)
> +{
> +       acpi_handle handle = child->handle;
> +       unsigned long long ret;
> +
> +       return ACPI_SUCCESS(acpi_evaluate_integer(handle, "_S4W", NULL, &ret));

First off, there is acpi_has_method().

Second, checking child->wakeup.sleep_state is a better indicator of S4
wakeup support.

> +}
> +
>  static int sdhci_acpi_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;

Thanks,
Rafael

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

On 24/04/17 23:33, Ulf Hansson wrote:
> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> The SDIO card state might be being preserved during hibernation, for
>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>> module and simply not load it until after attempting to restore the
>> hibernation image. However that won't work if the hibernation image is
>> stored on eMMC which, of course, requires mmc core.
> 
> I don't follow here. Are you saying the SDIO card is kept powered in
> hibernation, as to be able to support WOWLAN, right?

Yes

> 
> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
> That should never happen, unless something is broken of course.

The thing to note about hibernation is that there is a regular boot in
between saving the hibernation image and restoring it again.  At boot time,
the kernel knows almost nothing about whether there is a hibernation image
and whether or not it will be restored.  Consequently it becomes difficult
to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
initialize the eMMC so that the hibernation image can be read.

> 
> Kind regards
> Uffe
> 
>>
>> It is assumed on such systems that the platform will power cycle the SDIO
>> card or not as necessary so that the SDIO reset is not needed. Add a
>> capability flag to reflect that and use it to skip the SDIO reset at scan
>> time.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/core.c  | 6 +++++-
>>  include/linux/mmc/host.h | 1 +
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 6987976252ad..178e23bf0c30 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>>          * if the card is being re-initialized, just send it.  CMD52
>>          * should be ignored by SD/eMMC cards.
>>          * Skip it if we already know that we do not support SDIO commands
>> +        * Also skip it if we know this host controller has a SDIO card that
>> +        * needs to be able to restore from hibernation without losing the card
>> +        * state e.g. an SDIO wifi card supporting WOWLAN.
>>          */
>> -       if (!(host->caps2 & MMC_CAP2_NO_SDIO))
>> +       if (!(host->caps2 & MMC_CAP2_NO_SDIO) &&
>> +           !(host->caps2 & MMC_CAP2_NO_SDIO_RESET))
>>                 sdio_reset(host);
>>
>>         mmc_go_idle(host);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 78c544e296cd..187a7ba41364 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -281,6 +281,7 @@ struct mmc_host {
>>         u32                     caps2;          /* More host capabilities */
>>
>>  #define MMC_CAP2_BOOTPART_NOACC        (1 << 0)        /* Boot partition no access */
>> +#define MMC_CAP2_NO_SDIO_RESET (1 << 1)        /* Do not SDIO reset at scan */
>>  #define MMC_CAP2_FULL_PWR_CYCLE        (1 << 2)        /* Can do full power cycle */
>>  #define MMC_CAP2_HS200_1_8V_SDR        (1 << 5)        /* can support */
>>  #define MMC_CAP2_HS200_1_2V_SDR        (1 << 6)        /* can support */
>> --
>> 1.9.1
>>
> 

Re: [PATCH RFC 1/4] mmc: sdio: Keep card runtime resumed while adding function devices

[Adrian Hunter]

On 25/04/17 00:03, Ulf Hansson wrote:
> On 21 April 2017 at 12:08, 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
> 
> I totally agree! You are touching the part of the PM code for SDIO
> that is severely broken in my view.
> 
> The mmc core shouldn't mess with runtime PM for func device at all,
> except setting up the parent-child relationship. Instead runtime PM
> for the func device should be solely handled by the func driver.

This addresses the runtime pm of the SDIO card device, not the function
devices.  SDIO cards do not have drivers, only the functions do.

> 
>> pm runtime get/put to keep the SDIO card runtime resumed until the function
>> devices have been added.
>>
>> This also benefits the hibernation use-case whereby we want to avoid the
>> SDIO reset that is done as part of runtime resume. In that case, the SDIO
>> card state is preserved at boot time by avoiding loading any function
>> drivers before restoring the hibernation image.
> 
> This seems like a different issue. I guess this is because the func
> driver don't implement the PM callbacks, which makes the mmc core to
> remove the sdio card at mmc_pm_notify().
> 
> The proper solution here is to make the sdio func driver to implement
> the PM callbacks.

This is before the SDIO function devices have been added and consequently
before the SDIO function drivers have been probed.

Since none of the functions have drivers, mmc_pm_notify() doesn't remove the
card anyway.

> 
>>
>> 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
>>
> 
> Overall this looks okay to me. Except for the changelog that is a bit
> miss-leading.
> 
> Kind regards
> Uffe
> 

Re: [PATCH RFC 3/4] mmc: sdhci-acpi: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

[Adrian Hunter]

On 25/04/17 00:52, Rafael J. Wysocki wrote:
> On Fri, Apr 21, 2017 at 12:08 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> SDIO reset interferes with a SDIO function driver's restore from
>> hibernation. Set MMC_CAP2_NO_SDIO_RESET if a child node has _S4W method
>> which indicates a capability to wake from S4 (hibernate).
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/host/sdhci-acpi.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index c6a9a1bfaa22..e053a45db6b1 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -374,6 +374,14 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>>         return NULL;
>>  }
>>
>> +static bool sdhci_acpi_child_has_s4w(struct acpi_device *child)
>> +{
>> +       acpi_handle handle = child->handle;
>> +       unsigned long long ret;
>> +
>> +       return ACPI_SUCCESS(acpi_evaluate_integer(handle, "_S4W", NULL, &ret));
> 
> First off, there is acpi_has_method().
> 
> Second, checking child->wakeup.sleep_state is a better indicator of S4
> wakeup support.

So, like this?

	child->wakeup.sleep_state >= ACPI_STATE_S4

That comes just from _PRW right?

> 
>> +}
>> +
>>  static int sdhci_acpi_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
> 
> Thanks,
> Rafael
> 

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Ulf Hansson]

+Grygorii Strashko

On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 24/04/17 23:33, Ulf Hansson wrote:
>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> The SDIO card state might be being preserved during hibernation, for
>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>> module and simply not load it until after attempting to restore the
>>> hibernation image. However that won't work if the hibernation image is
>>> stored on eMMC which, of course, requires mmc core.
>>
>> I don't follow here. Are you saying the SDIO card is kept powered in
>> hibernation, as to be able to support WOWLAN, right?
>
> Yes

Okay, makes sense!

>
>>
>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>> That should never happen, unless something is broken of course.
>
> The thing to note about hibernation is that there is a regular boot in
> between saving the hibernation image and restoring it again.  At boot time,
> the kernel knows almost nothing about whether there is a hibernation image
> and whether or not it will be restored.  Consequently it becomes difficult
> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
> initialize the eMMC so that the hibernation image can be read.

What's wrong with using the hibernation callbacks in the struct
dev_pm_ops? We already do this.

To manage the eMMC/SD/SDIO card device, we do this in /drivers/mmc/core/bus.c:

static const struct dev_pm_ops mmc_bus_pm_ops = {
        SET_RUNTIME_PM_OPS(mmc_runtime_suspend, mmc_runtime_resume, NULL)
        SET_SYSTEM_SLEEP_PM_OPS(mmc_bus_suspend, mmc_bus_resume)
};

To mange the sdio func device, we do this in /drivers/mmc/core/sdio_bus.c

static const struct dev_pm_ops sdio_bus_pm_ops = {
        SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
        SET_RUNTIME_PM_OPS(
                pm_generic_runtime_suspend,
                pm_generic_runtime_resume,
                NULL
        )
};

In other words, we are re-using the same callbacks for system PM
suspend as for system PM hibernation.

I do imagine there are something broken for SDIO in this path, mainly
because I haven't heard much from people using it.

However, for (e)MMC/SD card point of view, restoring from hibernation
should work. I remember that Grygorii Strashko has been working on
this, perhaps he can confirm some of his observations.

Kind regards
Uffe

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

On 25/04/17 10:52, Ulf Hansson wrote:
> +Grygorii Strashko
> 
> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 24/04/17 23:33, Ulf Hansson wrote:
>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> The SDIO card state might be being preserved during hibernation, for
>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>> module and simply not load it until after attempting to restore the
>>>> hibernation image. However that won't work if the hibernation image is
>>>> stored on eMMC which, of course, requires mmc core.
>>>
>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>> hibernation, as to be able to support WOWLAN, right?
>>
>> Yes
> 
> Okay, makes sense!
> 
>>
>>>
>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>> That should never happen, unless something is broken of course.
>>
>> The thing to note about hibernation is that there is a regular boot in
>> between saving the hibernation image and restoring it again.  At boot time,
>> the kernel knows almost nothing about whether there is a hibernation image
>> and whether or not it will be restored.  Consequently it becomes difficult
>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>> initialize the eMMC so that the hibernation image can be read.
> 
> What's wrong with using the hibernation callbacks in the struct
> dev_pm_ops? We already do this.

Here is the scenario.  The kernel has just booted.  User space wants to try
to restore a hibernation image, if there is one.  So user space loads the
mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
reset on the SDIO card and the state is lost.  It has little to do with pm
callbacks AFAICS.

> 
> To manage the eMMC/SD/SDIO card device, we do this in /drivers/mmc/core/bus.c:
> 
> static const struct dev_pm_ops mmc_bus_pm_ops = {
>         SET_RUNTIME_PM_OPS(mmc_runtime_suspend, mmc_runtime_resume, NULL)
>         SET_SYSTEM_SLEEP_PM_OPS(mmc_bus_suspend, mmc_bus_resume)
> };
> 
> To mange the sdio func device, we do this in /drivers/mmc/core/sdio_bus.c
> 
> static const struct dev_pm_ops sdio_bus_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
>                 NULL
>         )
> };
> 
> In other words, we are re-using the same callbacks for system PM
> suspend as for system PM hibernation.
> 
> I do imagine there are something broken for SDIO in this path, mainly
> because I haven't heard much from people using it.
> 
> However, for (e)MMC/SD card point of view, restoring from hibernation
> should work. I remember that Grygorii Strashko has been working on
> this, perhaps he can confirm some of his observations.
> 
> Kind regards
> Uffe
> 

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Ulf Hansson]

On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 25/04/17 10:52, Ulf Hansson wrote:
>> +Grygorii Strashko
>>
>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>> module and simply not load it until after attempting to restore the
>>>>> hibernation image. However that won't work if the hibernation image is
>>>>> stored on eMMC which, of course, requires mmc core.
>>>>
>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>> hibernation, as to be able to support WOWLAN, right?
>>>
>>> Yes
>>
>> Okay, makes sense!
>>
>>>
>>>>
>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>> That should never happen, unless something is broken of course.
>>>
>>> The thing to note about hibernation is that there is a regular boot in
>>> between saving the hibernation image and restoring it again.  At boot time,
>>> the kernel knows almost nothing about whether there is a hibernation image
>>> and whether or not it will be restored.  Consequently it becomes difficult
>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>> initialize the eMMC so that the hibernation image can be read.
>>
>> What's wrong with using the hibernation callbacks in the struct
>> dev_pm_ops? We already do this.
>
> Here is the scenario.  The kernel has just booted.  User space wants to try
> to restore a hibernation image, if there is one.  So user space loads the
> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
> reset on the SDIO card and the state is lost.  It has little to do with pm
> callbacks AFAICS.

Ah, now I see what you mean. I thought the problem was during the
actual restoring of the hibernation image.

Alright, when a boot is triggered by WOWLAN , you want to avoid
sending the reset command for the SDIO card before the
re-initialization of the SDIO card starts.

The problem with this approach is that you can't differentiate between
a cold boot and a boot triggered by WOWLAN, right!? In other words, in
some cases the reset command may be needed while in other it won't.

Maybe you can elaborate more on what exactly what the problem is with
sending the reset command when the boot is triggered from WOWLAN? Yes,
the SDIO card loses its context, but how is that a problem?

[...]

Kind regards
Uffe

Re: [PATCH RFC 3/4] mmc: sdhci-acpi: Set MMC_CAP2_NO_SDIO_RESET if child has wake from S4 (hibernate)

[Rafael J. Wysocki]

On Tue, Apr 25, 2017 at 9:28 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 25/04/17 00:52, Rafael J. Wysocki wrote:
>> On Fri, Apr 21, 2017 at 12:08 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> SDIO reset interferes with a SDIO function driver's restore from
>>> hibernation. Set MMC_CAP2_NO_SDIO_RESET if a child node has _S4W method
>>> which indicates a capability to wake from S4 (hibernate).
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/host/sdhci-acpi.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index c6a9a1bfaa22..e053a45db6b1 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -374,6 +374,14 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>>>         return NULL;
>>>  }
>>>
>>> +static bool sdhci_acpi_child_has_s4w(struct acpi_device *child)
>>> +{
>>> +       acpi_handle handle = child->handle;
>>> +       unsigned long long ret;
>>> +
>>> +       return ACPI_SUCCESS(acpi_evaluate_integer(handle, "_S4W", NULL, &ret));
>>
>> First off, there is acpi_has_method().
>>
>> Second, checking child->wakeup.sleep_state is a better indicator of S4
>> wakeup support.
>
> So, like this?
>
>         child->wakeup.sleep_state >= ACPI_STATE_S4
>
> That comes just from _PRW right?

It may be fixed up if the power resources in there don't match what
_PRW itself returns, but generally yes.

In any case _S4W is optional even for devices that can wake up from S4 IIRC.

Thanks,
Rafael

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

On 25/04/17 13:46, Ulf Hansson wrote:
> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 25/04/17 10:52, Ulf Hansson wrote:
>>> +Grygorii Strashko
>>>
>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>> module and simply not load it until after attempting to restore the
>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>
>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>
>>>> Yes
>>>
>>> Okay, makes sense!
>>>
>>>>
>>>>>
>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>> That should never happen, unless something is broken of course.
>>>>
>>>> The thing to note about hibernation is that there is a regular boot in
>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>> initialize the eMMC so that the hibernation image can be read.
>>>
>>> What's wrong with using the hibernation callbacks in the struct
>>> dev_pm_ops? We already do this.
>>
>> Here is the scenario.  The kernel has just booted.  User space wants to try
>> to restore a hibernation image, if there is one.  So user space loads the
>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>> reset on the SDIO card and the state is lost.  It has little to do with pm
>> callbacks AFAICS.
> 
> Ah, now I see what you mean. I thought the problem was during the
> actual restoring of the hibernation image.
> 
> Alright, when a boot is triggered by WOWLAN , you want to avoid
> sending the reset command for the SDIO card before the
> re-initialization of the SDIO card starts.
> 
> The problem with this approach is that you can't differentiate between
> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
> some cases the reset command may be needed while in other it won't.

SDIO reset is only needed if the card has not been power-cycled.  The
assumption is that the platform takes care of that when needed. e.g. when
rebooting instead of going to S4.

> 
> Maybe you can elaborate more on what exactly what the problem is with
> sending the reset command when the boot is triggered from WOWLAN? Yes,
> the SDIO card loses its context, but how is that a problem?

The wifi driver expects to find the function in an initialized state.
Otherwise it would have to re-enable the function and re-do the
function-specific initialization.  I don't know if there are other
consequences.  Presumably it will have lost any information about the nature
of the wake-up trigger.

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Ulf Hansson]

On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 25/04/17 13:46, Ulf Hansson wrote:
>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 25/04/17 10:52, Ulf Hansson wrote:
>>>> +Grygorii Strashko
>>>>
>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>>> module and simply not load it until after attempting to restore the
>>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>>
>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>>
>>>>> Yes
>>>>
>>>> Okay, makes sense!
>>>>
>>>>>
>>>>>>
>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>>> That should never happen, unless something is broken of course.
>>>>>
>>>>> The thing to note about hibernation is that there is a regular boot in
>>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>>> initialize the eMMC so that the hibernation image can be read.
>>>>
>>>> What's wrong with using the hibernation callbacks in the struct
>>>> dev_pm_ops? We already do this.
>>>
>>> Here is the scenario.  The kernel has just booted.  User space wants to try
>>> to restore a hibernation image, if there is one.  So user space loads the
>>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>>> reset on the SDIO card and the state is lost.  It has little to do with pm
>>> callbacks AFAICS.
>>
>> Ah, now I see what you mean. I thought the problem was during the
>> actual restoring of the hibernation image.
>>
>> Alright, when a boot is triggered by WOWLAN , you want to avoid
>> sending the reset command for the SDIO card before the
>> re-initialization of the SDIO card starts.
>>
>> The problem with this approach is that you can't differentiate between
>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
>> some cases the reset command may be needed while in other it won't.
>
> SDIO reset is only needed if the card has not been power-cycled.  The
> assumption is that the platform takes care of that when needed. e.g. when
> rebooting instead of going to S4.
>
>>
>> Maybe you can elaborate more on what exactly what the problem is with
>> sending the reset command when the boot is triggered from WOWLAN? Yes,
>> the SDIO card loses its context, but how is that a problem?
>
> The wifi driver expects to find the function in an initialized state.

So how does the the wifi driver distinguish this a boot caused by
WOWLAN - and not a cold boot?

> Otherwise it would have to re-enable the function and re-do the
> function-specific initialization.  I don't know if there are other

At boot the SDIO func driver becomes probed, when the mmc core finds
and SDIO card and then registers a func device for it. Are you saying
that the SDIO func driver can take shortcuts when enabling the func
device, when the boot is trigger from WOWLAN?

> consequences.  Presumably it will have lost any information about the nature
> of the wake-up trigger.

How does that matter?

Kind regards
Uffe

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

On 25/04/17 15:24, Ulf Hansson wrote:
> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 25/04/17 13:46, Ulf Hansson wrote:
>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 25/04/17 10:52, Ulf Hansson wrote:
>>>>> +Grygorii Strashko
>>>>>
>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>>>> module and simply not load it until after attempting to restore the
>>>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>>>
>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>>>
>>>>>> Yes
>>>>>
>>>>> Okay, makes sense!
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>>>> That should never happen, unless something is broken of course.
>>>>>>
>>>>>> The thing to note about hibernation is that there is a regular boot in
>>>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>>>> initialize the eMMC so that the hibernation image can be read.
>>>>>
>>>>> What's wrong with using the hibernation callbacks in the struct
>>>>> dev_pm_ops? We already do this.
>>>>
>>>> Here is the scenario.  The kernel has just booted.  User space wants to try
>>>> to restore a hibernation image, if there is one.  So user space loads the
>>>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>>>> reset on the SDIO card and the state is lost.  It has little to do with pm
>>>> callbacks AFAICS.
>>>
>>> Ah, now I see what you mean. I thought the problem was during the
>>> actual restoring of the hibernation image.
>>>
>>> Alright, when a boot is triggered by WOWLAN , you want to avoid
>>> sending the reset command for the SDIO card before the
>>> re-initialization of the SDIO card starts.
>>>
>>> The problem with this approach is that you can't differentiate between
>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
>>> some cases the reset command may be needed while in other it won't.
>>
>> SDIO reset is only needed if the card has not been power-cycled.  The
>> assumption is that the platform takes care of that when needed. e.g. when
>> rebooting instead of going to S4.
>>
>>>
>>> Maybe you can elaborate more on what exactly what the problem is with
>>> sending the reset command when the boot is triggered from WOWLAN? Yes,
>>> the SDIO card loses its context, but how is that a problem?
>>
>> The wifi driver expects to find the function in an initialized state.
> 
> So how does the the wifi driver distinguish this a boot caused by
> WOWLAN - and not a cold boot?

It doesn't have to.  It doesn't get loaded until after the attempt to
restore the hibernation image.  So in the hibernation case it has its
->restore() callback.  If there is no hibernation image, it gets loaded and
probed.

AFAICT that is the normal way to stop drivers interfering with hibernation
i.e. don't load them until after the attempt is made to restore the
hibernation image.  We could do that with mmc core too, but for the fact
that the hibernation image is on eMMC.

> 
>> Otherwise it would have to re-enable the function and re-do the
>> function-specific initialization.  I don't know if there are other
> 
> At boot the SDIO func driver becomes probed, when the mmc core finds
> and SDIO card and then registers a func device for it. Are you saying
> that the SDIO func driver can take shortcuts when enabling the func
> device, when the boot is trigger from WOWLAN?

No.

> 
>> consequences.  Presumably it will have lost any information about the nature
>> of the wake-up trigger.
> 
> How does that matter?

I don't know if it matters.

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Grygorii Strashko]

On 04/25/2017 07:45 AM, Adrian Hunter wrote:
> On 25/04/17 15:24, Ulf Hansson wrote:
>> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 25/04/17 13:46, Ulf Hansson wrote:
>>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 25/04/17 10:52, Ulf Hansson wrote:
>>>>>> +Grygorii Strashko
>>>>>>
>>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>>>>> module and simply not load it until after attempting to restore the
>>>>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>>>>
>>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>>>>
>>>>>>> Yes
>>>>>>
>>>>>> Okay, makes sense!
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>>>>> That should never happen, unless something is broken of course.
>>>>>>>
>>>>>>> The thing to note about hibernation is that there is a regular boot in
>>>>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>>>>> initialize the eMMC so that the hibernation image can be read.
>>>>>>
>>>>>> What's wrong with using the hibernation callbacks in the struct
>>>>>> dev_pm_ops? We already do this.
>>>>>
>>>>> Here is the scenario.  The kernel has just booted.  User space wants to try
>>>>> to restore a hibernation image, if there is one.  So user space loads the
>>>>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>>>>> reset on the SDIO card and the state is lost.  It has little to do with pm
>>>>> callbacks AFAICS.
>>>>
>>>> Ah, now I see what you mean. I thought the problem was during the
>>>> actual restoring of the hibernation image.
>>>>
>>>> Alright, when a boot is triggered by WOWLAN , you want to avoid
>>>> sending the reset command for the SDIO card before the
>>>> re-initialization of the SDIO card starts.
>>>>
>>>> The problem with this approach is that you can't differentiate between
>>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
>>>> some cases the reset command may be needed while in other it won't.
>>>
>>> SDIO reset is only needed if the card has not been power-cycled.  The
>>> assumption is that the platform takes care of that when needed. e.g. when
>>> rebooting instead of going to S4.
>>>
>>>>
>>>> Maybe you can elaborate more on what exactly what the problem is with
>>>> sending the reset command when the boot is triggered from WOWLAN? Yes,
>>>> the SDIO card loses its context, but how is that a problem?
>>>
>>> The wifi driver expects to find the function in an initialized state.
>>
>> So how does the the wifi driver distinguish this a boot caused by
>> WOWLAN - and not a cold boot?
> 
> It doesn't have to.  It doesn't get loaded until after the attempt to
> restore the hibernation image.  So in the hibernation case it has its
> ->restore() callback.  

For this case is a good question of how Kernel wifi card driver state will be
synchronized HW state after restore from Hibernation (first of all wireless state -
scan results, connection state and parameters)? Most probably wifi driver will
need to perform mostly full re-initialization on restore, so...

>If there is no hibernation image, it gets loaded and
> probed.

But in this case, as per you patch, there are will be no SDIO reset so
do you expect that wifi driver will still work? 

> 
> AFAICT that is the normal way to stop drivers interfering with hibernation
> i.e. don't load them until after the attempt is made to restore the
> hibernation image.  We could do that with mmc core too, but for the fact
> that the hibernation image is on eMMC.

>From my experience, it pretty hard to restore HW state which runs by FW, so as
W/A we just unloaded remoteproc, wireless and graphic drivers before hibernation and
reloaded them right after. As I know the same approach often used in x86 world also.


> 
>>
>>> Otherwise it would have to re-enable the function and re-do the
>>> function-specific initialization.  I don't know if there are other
>>
>> At boot the SDIO func driver becomes probed, when the mmc core finds
>> and SDIO card and then registers a func device for it. Are you saying
>> that the SDIO func driver can take shortcuts when enabling the func
>> device, when the boot is trigger from WOWLAN?
> 
> No.
> 
>>
>>> consequences.  Presumably it will have lost any information about the nature
>>> of the wake-up trigger.
>>
>> How does that matter?
> 
> I don't know if it matters.
> 

-- 
regards,
-grygorii

Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Adrian Hunter]

On 04/25/2017 09:51 PM, Grygorii Strashko wrote:
> 
> 
> On 04/25/2017 07:45 AM, Adrian Hunter wrote:
>> On 25/04/17 15:24, Ulf Hansson wrote:
>>> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 25/04/17 13:46, Ulf Hansson wrote:
>>>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 25/04/17 10:52, Ulf Hansson wrote:
>>>>>>> +Grygorii Strashko
>>>>>>>
>>>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>>>>>> module and simply not load it until after attempting to restore the
>>>>>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>>>>>
>>>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>>>>>
>>>>>>>> Yes
>>>>>>>
>>>>>>> Okay, makes sense!
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>>>>>> That should never happen, unless something is broken of course.
>>>>>>>>
>>>>>>>> The thing to note about hibernation is that there is a regular boot in
>>>>>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>>>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>>>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>>>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>>>>>> initialize the eMMC so that the hibernation image can be read.
>>>>>>>
>>>>>>> What's wrong with using the hibernation callbacks in the struct
>>>>>>> dev_pm_ops? We already do this.
>>>>>>
>>>>>> Here is the scenario.  The kernel has just booted.  User space wants to try
>>>>>> to restore a hibernation image, if there is one.  So user space loads the
>>>>>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>>>>>> reset on the SDIO card and the state is lost.  It has little to do with pm
>>>>>> callbacks AFAICS.
>>>>>
>>>>> Ah, now I see what you mean. I thought the problem was during the
>>>>> actual restoring of the hibernation image.
>>>>>
>>>>> Alright, when a boot is triggered by WOWLAN , you want to avoid
>>>>> sending the reset command for the SDIO card before the
>>>>> re-initialization of the SDIO card starts.
>>>>>
>>>>> The problem with this approach is that you can't differentiate between
>>>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
>>>>> some cases the reset command may be needed while in other it won't.
>>>>
>>>> SDIO reset is only needed if the card has not been power-cycled.  The
>>>> assumption is that the platform takes care of that when needed. e.g. when
>>>> rebooting instead of going to S4.
>>>>
>>>>>
>>>>> Maybe you can elaborate more on what exactly what the problem is with
>>>>> sending the reset command when the boot is triggered from WOWLAN? Yes,
>>>>> the SDIO card loses its context, but how is that a problem?
>>>>
>>>> The wifi driver expects to find the function in an initialized state.
>>>
>>> So how does the the wifi driver distinguish this a boot caused by
>>> WOWLAN - and not a cold boot?
>>
>> It doesn't have to.  It doesn't get loaded until after the attempt to
>> restore the hibernation image.  So in the hibernation case it has its
>> ->restore() callback.  
> 
> For this case is a good question of how Kernel wifi card driver state will be
> synchronized HW state after restore from Hibernation (first of all wireless state -
> scan results, connection state and parameters)? Most probably wifi driver will
> need to perform mostly full re-initialization on restore, so...
> 
>> If there is no hibernation image, it gets loaded and
>> probed.
> 
> But in this case, as per you patch, there are will be no SDIO reset so
> do you expect that wifi driver will still work? 

It does still work yes.

> 
>>
>> AFAICT that is the normal way to stop drivers interfering with hibernation
>> i.e. don't load them until after the attempt is made to restore the
>> hibernation image.  We could do that with mmc core too, but for the fact
>> that the hibernation image is on eMMC.
> 
>>From my experience, it pretty hard to restore HW state which runs by FW, so as
> W/A we just unloaded remoteproc, wireless and graphic drivers before hibernation and
> reloaded them right after. As I know the same approach often used in x86 world also.

Not sure it would make sense to try and configure WOWLAN and then remove the
driver?

> 
> 
>>
>>>
>>>> Otherwise it would have to re-enable the function and re-do the
>>>> function-specific initialization.  I don't know if there are other
>>>
>>> At boot the SDIO func driver becomes probed, when the mmc core finds
>>> and SDIO card and then registers a func device for it. Are you saying
>>> that the SDIO func driver can take shortcuts when enabling the func
>>> device, when the boot is trigger from WOWLAN?
>>
>> No.
>>
>>>
>>>> consequences.  Presumably it will have lost any information about the nature
>>>> of the wake-up trigger.
>>>
>>> How does that matter?
>>
>> I don't know if it matters.
>>
>