[PATCH 0/9] mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup

[PATCH 0/9] mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup

[Adrian Hunter]

Hi

Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback.  Patch 9 adds support for that in sdhci-pci, which is the
only driver presently using the MMC_CAP_CD_WAKE flag.  Patches 1 - 7 tidy
up aspects of sdhci and sdhci-pci wakeup handling, and patch 8 adds a
helper function to make it easy for drivers.


Adrian Hunter (9):
      mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()
      mmc: sdhci-pci: Use device wakeup capability to determine MMC_PM_WAKE_SDIO_IRQ capability
      mmc: sdhci: Stop exporting sdhci_enable_irq_wakeups()
      mmc: sdhci: Handle failure of enable_irq_wake()
      mmc: sdhci: Rework sdhci_enable_irq_wakeups()
      mmc: sdhci: Do not unnecessarily enable wakeup for card detect interrupt
      mmc: sdhci: Do not unnecessarily enable wakeup for SDIO card interrupt
      mmc: slot-gpio: Add a function to enable/disable card detect IRQ wakeup
      mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup

 drivers/mmc/core/core.c           |  3 +-
 drivers/mmc/core/slot-gpio.c      | 25 ++++++++++++--
 drivers/mmc/host/sdhci-pci-core.c | 73 +++++++++++++++++++--------------------
 drivers/mmc/host/sdhci.c          | 67 ++++++++++++++++++++++++-----------
 drivers/mmc/host/sdhci.h          |  2 +-
 include/linux/mmc/slot-gpio.h     |  1 +
 6 files changed, 108 insertions(+), 63 deletions(-)


Regards
Adrian

[PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Adrian Hunter]

sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
sdhci-pci should not need to call it. However sdhci_suspend_host() only
calls it if wakeups are enabled, and sdhci-pci does not enable them until
after calling sdhci_suspend_host(). So move the calls to
sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
stop calling sdhci_enable_irq_wakeups(). That results in some
simplification because sdhci_pci_suspend_host() and
__sdhci_pci_suspend_host() no longer need to be separate functions.

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

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 110c634cfb43..2ffc09f088ee 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -39,10 +39,29 @@
 static void sdhci_pci_hw_reset(struct sdhci_host *host);
 
 #ifdef CONFIG_PM_SLEEP
-static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
+static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
+{
+	mmc_pm_flag_t pm_flags = 0;
+	int i;
+
+	for (i = 0; i < chip->num_slots; i++) {
+		struct sdhci_pci_slot *slot = chip->slots[i];
+
+		if (slot)
+			pm_flags |= slot->host->mmc->pm_flags;
+	}
+
+	return device_init_wakeup(&chip->pdev->dev,
+				  (pm_flags & MMC_PM_KEEP_POWER) &&
+				  (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+}
+
+static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
 {
 	int i, ret;
 
+	sdhci_pci_init_wakeup(chip);
+
 	for (i = 0; i < chip->num_slots; i++) {
 		struct sdhci_pci_slot *slot = chip->slots[i];
 		struct sdhci_host *host;
@@ -58,9 +77,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
 		ret = sdhci_suspend_host(host);
 		if (ret)
 			goto err_pci_suspend;
-
-		if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
-			sdhci_enable_irq_wakeups(host);
 	}
 
 	return 0;
@@ -71,36 +87,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
 	return ret;
 }
 
-static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
-{
-	mmc_pm_flag_t pm_flags = 0;
-	int i;
-
-	for (i = 0; i < chip->num_slots; i++) {
-		struct sdhci_pci_slot *slot = chip->slots[i];
-
-		if (slot)
-			pm_flags |= slot->host->mmc->pm_flags;
-	}
-
-	return device_init_wakeup(&chip->pdev->dev,
-				  (pm_flags & MMC_PM_KEEP_POWER) &&
-				  (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
-}
-
-static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
-{
-	int ret;
-
-	ret = __sdhci_pci_suspend_host(chip);
-	if (ret)
-		return ret;
-
-	sdhci_pci_init_wakeup(chip);
-
-	return 0;
-}
-
 int sdhci_pci_resume_host(struct sdhci_pci_chip *chip)
 {
 	struct sdhci_pci_slot *slot;
@@ -1108,7 +1094,7 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
 {
 	int i, ret;
 
-	ret = __sdhci_pci_suspend_host(chip);
+	ret = sdhci_pci_suspend_host(chip);
 	if (ret)
 		return ret;
 
@@ -1118,8 +1104,6 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
 			jmicron_enable_mmc(chip->slots[i]->host, 0);
 	}
 
-	sdhci_pci_init_wakeup(chip);
-
 	return 0;
 }
 
-- 
1.9.1

[PATCH 2/9] mmc: sdhci-pci: Use device wakeup capability to determine MMC_PM_WAKE_SDIO_IRQ capability

[Adrian Hunter]

PCI and ACPI determine if a device is wakeup capable, so use that to
determine the MMC_PM_WAKE_SDIO_IRQ capability correctly.

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

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 2ffc09f088ee..96ec4aa7cb27 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -51,9 +51,9 @@ static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
 			pm_flags |= slot->host->mmc->pm_flags;
 	}
 
-	return device_init_wakeup(&chip->pdev->dev,
-				  (pm_flags & MMC_PM_KEEP_POWER) &&
-				  (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+	return device_set_wakeup_enable(&chip->pdev->dev,
+					(pm_flags & MMC_PM_KEEP_POWER) &&
+					(pm_flags & MMC_PM_WAKE_SDIO_IRQ));
 }
 
 static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
@@ -1680,10 +1680,13 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
 		}
 	}
 
-	host->mmc->pm_caps = MMC_PM_KEEP_POWER | MMC_PM_WAKE_SDIO_IRQ;
+	host->mmc->pm_caps = MMC_PM_KEEP_POWER;
 	host->mmc->slotno = slotno;
 	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
 
+	if (device_can_wakeup(&pdev->dev))
+		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+
 	if (slot->cd_idx >= 0) {
 		ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx,
 					   slot->cd_override_level, 0, NULL);
-- 
1.9.1

[PATCH 3/9] mmc: sdhci: Stop exporting sdhci_enable_irq_wakeups()

[Adrian Hunter]

Now that it is not being used by any drivers, stop exporting it.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 3 +--
 drivers/mmc/host/sdhci.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e9290a3439d5..8b2ccf7795ec 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2821,7 +2821,7 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
  * sdhci_disable_irq_wakeups() since it will be set by
  * sdhci_enable_card_detection() or sdhci_init().
  */
-void sdhci_enable_irq_wakeups(struct sdhci_host *host)
+static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
 {
 	u8 val;
 	u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
@@ -2839,7 +2839,6 @@ void sdhci_enable_irq_wakeups(struct sdhci_host *host)
 	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
 	sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
 }
-EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups);
 
 static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
 {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 54bc444c317f..7393b3a54772 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -718,7 +718,6 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 #ifdef CONFIG_PM
 int sdhci_suspend_host(struct sdhci_host *host);
 int sdhci_resume_host(struct sdhci_host *host);
-void sdhci_enable_irq_wakeups(struct sdhci_host *host);
 int sdhci_runtime_suspend_host(struct sdhci_host *host);
 int sdhci_runtime_resume_host(struct sdhci_host *host);
 #endif
-- 
1.9.1

[PATCH 4/9] mmc: sdhci: Handle failure of enable_irq_wake()

[Adrian Hunter]

Now that sdhci_enable_irq_wakeups() is a local function, change it to
return whether the IRQ wakeup was successfully enabled. This is in
preparation for adding more conditions for whether IRQ wakeup is enabled.

Note it is assumed, for SDHCI devices, that suspend is more important than
wakeup, so we continue to suspend regardless.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 24 +++++++++++++++---------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8b2ccf7795ec..a8129d091207 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2821,7 +2821,7 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
  * sdhci_disable_irq_wakeups() since it will be set by
  * sdhci_enable_card_detection() or sdhci_init().
  */
-static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
+static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
 {
 	u8 val;
 	u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
@@ -2838,6 +2838,10 @@ static void sdhci_enable_irq_wakeups(struct sdhci_host *host)
 	}
 	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
 	sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
+
+	host->irq_wake_enabled = !enable_irq_wake(host->irq);
+
+	return host->irq_wake_enabled;
 }
 
 static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
@@ -2849,6 +2853,10 @@ static void sdhci_disable_irq_wakeups(struct sdhci_host *host)
 	val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
 	val &= ~mask;
 	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
+
+	disable_irq_wake(host->irq);
+
+	host->irq_wake_enabled = false;
 }
 
 int sdhci_suspend_host(struct sdhci_host *host)
@@ -2857,15 +2865,14 @@ int sdhci_suspend_host(struct sdhci_host *host)
 
 	mmc_retune_timer_stop(host->mmc);
 
-	if (!device_may_wakeup(mmc_dev(host->mmc))) {
+	if (!device_may_wakeup(mmc_dev(host->mmc)) ||
+	    !sdhci_enable_irq_wakeups(host)) {
 		host->ier = 0;
 		sdhci_writel(host, 0, SDHCI_INT_ENABLE);
 		sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
 		free_irq(host->irq, host);
-	} else {
-		sdhci_enable_irq_wakeups(host);
-		enable_irq_wake(host->irq);
 	}
+
 	return 0;
 }
 
@@ -2893,15 +2900,14 @@ int sdhci_resume_host(struct sdhci_host *host)
 		mmiowb();
 	}
 
-	if (!device_may_wakeup(mmc_dev(host->mmc))) {
+	if (host->irq_wake_enabled) {
+		sdhci_disable_irq_wakeups(host);
+	} else {
 		ret = request_threaded_irq(host->irq, sdhci_irq,
 					   sdhci_thread_irq, IRQF_SHARED,
 					   mmc_hostname(host->mmc), host);
 		if (ret)
 			return ret;
-	} else {
-		sdhci_disable_irq_wakeups(host);
-		disable_irq_wake(host->irq);
 	}
 
 	sdhci_enable_card_detection(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7393b3a54772..afab26fd70e6 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -484,6 +484,7 @@ struct sdhci_host {
 	bool bus_on;		/* Bus power prevents runtime suspend */
 	bool preset_enabled;	/* Preset is enabled */
 	bool pending_reset;	/* Cmd/data reset is pending */
+	bool irq_wake_enabled;	/* IRQ wakeup is enabled */
 
 	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
 	struct mmc_command *cmd;	/* Current command */
-- 
1.9.1

[PATCH 5/9] mmc: sdhci: Rework sdhci_enable_irq_wakeups()

[Adrian Hunter]

In preparation for adding more conditions for whether IRQ wakeup is
enabled, rework sdhci_enable_irq_wakeups() so that needed bits are added
instead of adding them all and then removing the unneeded bits.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a8129d091207..dd922923019a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2823,20 +2823,25 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
  */
 static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
 {
+	u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE |
+		  SDHCI_WAKE_ON_INT;
+	u32 irq_val = 0;
+	u8 wake_val = 0;
 	u8 val;
-	u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE
-			| SDHCI_WAKE_ON_INT;
-	u32 irq_val = SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE |
-		      SDHCI_INT_CARD_INT;
 
-	val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
-	val |= mask ;
-	/* Avoid fake wake up */
-	if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
-		val &= ~(SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE);
-		irq_val &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
+	if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)) {
+		wake_val |= SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
+		irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
 	}
+
+	wake_val |= SDHCI_WAKE_ON_INT;
+	irq_val |= SDHCI_INT_CARD_INT;
+
+	val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
+	val &= ~mask;
+	val |= wake_val;
 	sdhci_writeb(host, val, SDHCI_WAKE_UP_CONTROL);
+
 	sdhci_writel(host, irq_val, SDHCI_INT_ENABLE);
 
 	host->irq_wake_enabled = !enable_irq_wake(host->irq);
-- 
1.9.1

[PATCH 6/9] mmc: sdhci: Do not unnecessarily enable wakeup for card detect interrupt

[Adrian Hunter]

Do not unnecessarily enable card detect wakeup in the cases that the card
is not removable or a GPIO is used for card detect.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dd922923019a..f16df253cc59 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2813,6 +2813,14 @@ static irqreturn_t sdhci_thread_irq(int irq, void *dev_id)
 \*****************************************************************************/
 
 #ifdef CONFIG_PM
+
+static bool sdhci_cd_irq_can_wakeup(struct sdhci_host *host)
+{
+	return mmc_card_is_removable(host->mmc) &&
+	       !(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
+	       host->mmc->slot.cd_irq < 0;
+}
+
 /*
  * To enable wakeup events, the corresponding events have to be enabled in
  * the Interrupt Status Enable register too. See 'Table 1-6: Wakeup Signal
@@ -2829,7 +2837,7 @@ static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
 	u8 wake_val = 0;
 	u8 val;
 
-	if (!(host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)) {
+	if (sdhci_cd_irq_can_wakeup(host)) {
 		wake_val |= SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE;
 		irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
 	}
-- 
1.9.1

[PATCH 7/9] mmc: sdhci: Do not unnecessarily enable wakeup for SDIO card interrupt

[Adrian Hunter]

Do not enable wakeup for SDIO card interrupt unless the SDIO function
driver has requested it which is indicated by mmc_card_wake_sdio_irq().

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f16df253cc59..c82fd16dd14e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2842,8 +2842,13 @@ static bool sdhci_enable_irq_wakeups(struct sdhci_host *host)
 		irq_val |= SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE;
 	}
 
-	wake_val |= SDHCI_WAKE_ON_INT;
-	irq_val |= SDHCI_INT_CARD_INT;
+	if (mmc_card_wake_sdio_irq(host->mmc)) {
+		wake_val |= SDHCI_WAKE_ON_INT;
+		irq_val |= SDHCI_INT_CARD_INT;
+	}
+
+	if (!irq_val)
+		return false;
 
 	val = sdhci_readb(host, SDHCI_WAKE_UP_CONTROL);
 	val &= ~mask;
-- 
1.9.1

[PATCH 8/9] mmc: slot-gpio: Add a function to enable/disable card detect IRQ wakeup

[Adrian Hunter]

Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback. Add helper function mmc_gpio_set_cd_wake() to make it
easy for drivers to do that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c       |  3 +--
 drivers/mmc/core/slot-gpio.c  | 23 +++++++++++++++++++++++
 include/linux/mmc/slot-gpio.h |  1 +
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c0ba6d8823b7..7bed23c877de 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2655,8 +2655,7 @@ void mmc_start_host(struct mmc_host *host)
 void mmc_stop_host(struct mmc_host *host)
 {
 	if (host->slot.cd_irq >= 0) {
-		if (host->slot.cd_wake_enabled)
-			disable_irq_wake(host->slot.cd_irq);
+		mmc_gpio_set_cd_wake(host, false);
 		disable_irq(host->slot.cd_irq);
 	}
 
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index f7c6e0542de7..3a30535c2f31 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -154,6 +154,29 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_gpiod_request_cd_irq);
 
+int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on)
+{
+	int ret = 0;
+
+	if (!(host->caps & MMC_CAP_CD_WAKE) ||
+	    host->slot.cd_irq < 0 ||
+	    on == host->slot.cd_wake_enabled)
+		return 0;
+
+	if (on) {
+		if (device_may_wakeup(mmc_dev(host))) {
+			ret = enable_irq_wake(host->slot.cd_irq);
+			host->slot.cd_wake_enabled = !ret;
+		}
+	} else {
+		disable_irq_wake(host->slot.cd_irq);
+		host->slot.cd_wake_enabled = false;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(mmc_gpio_set_cd_wake);
+
 /* Register an alternate interrupt service routine for
  * the card-detect GPIO.
  */
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index 82f0d289f110..1ee9f15bf2c0 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -33,5 +33,6 @@ void mmc_gpio_set_cd_isr(struct mmc_host *host,
 			 irqreturn_t (*isr)(int irq, void *dev_id));
 void mmc_gpiod_request_cd_irq(struct mmc_host *host);
 bool mmc_can_gpio_cd(struct mmc_host *host);
+int mmc_gpio_set_cd_wake(struct mmc_host *host, bool on);
 
 #endif
-- 
1.9.1

[PATCH 9/9] mmc: sdhci-pci: Respect PM flags when enabling card detect GPIO IRQ wakeup

[Adrian Hunter]

Commit 03dbaa04a2e5 ("mmc: slot-gpio: Add support to enable irq wake on
cd_irq") enabled wakeup irrespective of the host controller's PM flags.
However, users also want to control it from sysfs power/wakeup attribute.
That means the driver needs to check the PM flags before enabling it in the
suspend callback. Add support for that in sdhci-pci, which is the only
driver presently using the MMC_CAP_CD_WAKE flag, and remove the enabling in
mmc_gpiod_request_cd_irq().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/slot-gpio.c      |  2 --
 drivers/mmc/host/sdhci-pci-core.c | 18 ++++++++++++++----
 drivers/mmc/host/sdhci.c          |  4 ++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 3a30535c2f31..74dd534905f7 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -149,8 +149,6 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host)
 
 	if (irq < 0)
 		host->caps |= MMC_CAP_NEEDS_POLL;
-	else if ((host->caps & MMC_CAP_CD_WAKE) && !enable_irq_wake(irq))
-		host->slot.cd_wake_enabled = true;
 }
 EXPORT_SYMBOL(mmc_gpiod_request_cd_irq);
 
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 96ec4aa7cb27..eeb4bd5797a0 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -42,18 +42,25 @@
 static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
 {
 	mmc_pm_flag_t pm_flags = 0;
+	bool cap_cd_wake = false;
 	int i;
 
 	for (i = 0; i < chip->num_slots; i++) {
 		struct sdhci_pci_slot *slot = chip->slots[i];
 
-		if (slot)
+		if (slot) {
 			pm_flags |= slot->host->mmc->pm_flags;
+			if (slot->host->mmc->caps & MMC_CAP_CD_WAKE)
+				cap_cd_wake = true;
+		}
 	}
 
-	return device_set_wakeup_enable(&chip->pdev->dev,
-					(pm_flags & MMC_PM_KEEP_POWER) &&
-					(pm_flags & MMC_PM_WAKE_SDIO_IRQ));
+	if ((pm_flags & MMC_PM_KEEP_POWER) && (pm_flags & MMC_PM_WAKE_SDIO_IRQ))
+		return device_wakeup_enable(&chip->pdev->dev);
+	else if (!cap_cd_wake)
+		return device_wakeup_disable(&chip->pdev->dev);
+
+	return 0;
 }
 
 static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
@@ -1687,6 +1694,9 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
 	if (device_can_wakeup(&pdev->dev))
 		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
 
+	if (host->mmc->caps & MMC_CAP_CD_WAKE)
+		device_init_wakeup(&pdev->dev, true);
+
 	if (slot->cd_idx >= 0) {
 		ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx,
 					   slot->cd_override_level, 0, NULL);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c82fd16dd14e..883b55077231 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2891,6 +2891,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
 		free_irq(host->irq, host);
 	}
 
+	mmc_gpio_set_cd_wake(host->mmc, true);
+
 	return 0;
 }
 
@@ -2918,6 +2920,8 @@ int sdhci_resume_host(struct sdhci_host *host)
 		mmiowb();
 	}
 
+	mmc_gpio_set_cd_wake(host->mmc, false);
+
 	if (host->irq_wake_enabled) {
 		sdhci_disable_irq_wakeups(host);
 	} else {
-- 
1.9.1

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Ulf Hansson]

On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
> sdhci-pci should not need to call it. However sdhci_suspend_host() only
> calls it if wakeups are enabled, and sdhci-pci does not enable them until
> after calling sdhci_suspend_host(). So move the calls to
> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
> stop calling sdhci_enable_irq_wakeups(). That results in some
> simplification because sdhci_pci_suspend_host() and
> __sdhci_pci_suspend_host() no longer need to be separate functions.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 110c634cfb43..2ffc09f088ee 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -39,10 +39,29 @@
>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>
>  #ifdef CONFIG_PM_SLEEP
> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> +{
> +       mmc_pm_flag_t pm_flags = 0;
> +       int i;
> +
> +       for (i = 0; i < chip->num_slots; i++) {
> +               struct sdhci_pci_slot *slot = chip->slots[i];
> +
> +               if (slot)
> +                       pm_flags |= slot->host->mmc->pm_flags;
> +       }
> +
> +       return device_init_wakeup(&chip->pdev->dev,
> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));

A couple of comments here.

Wakeup settings shouldn't be changed during system suspend. That means
we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
for that matter) here.

Instead, device_init_wakeup() should be called during ->probe(), while
device_set_wakeup_enable() should normally *not* have to be called at
all by drivers. There are a exceptions for device_set_wakeup_enable(),
however it should not have to be called during system suspend, at
least.

Of course, I realize that you are not changing the behavior in this
regards in this patch, so I am fine this anyway.

My point is, that the end result while doing this improvements in this
series, should strive to that device_init_wakeup() and
device_set_wakeup_enable() becomes used correctly. I don't think that
is the case, or is it?

> +}
> +
> +static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>  {
>         int i, ret;
>
> +       sdhci_pci_init_wakeup(chip);
> +
>         for (i = 0; i < chip->num_slots; i++) {
>                 struct sdhci_pci_slot *slot = chip->slots[i];
>                 struct sdhci_host *host;
> @@ -58,9 +77,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>                 ret = sdhci_suspend_host(host);
>                 if (ret)
>                         goto err_pci_suspend;
> -
> -               if (host->mmc->pm_flags & MMC_PM_WAKE_SDIO_IRQ)
> -                       sdhci_enable_irq_wakeups(host);
>         }
>
>         return 0;
> @@ -71,36 +87,6 @@ static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>         return ret;
>  }
>
> -static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> -{
> -       mmc_pm_flag_t pm_flags = 0;
> -       int i;
> -
> -       for (i = 0; i < chip->num_slots; i++) {
> -               struct sdhci_pci_slot *slot = chip->slots[i];
> -
> -               if (slot)
> -                       pm_flags |= slot->host->mmc->pm_flags;
> -       }
> -
> -       return device_init_wakeup(&chip->pdev->dev,
> -                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> -                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> -}
> -
> -static int sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> -{
> -       int ret;
> -
> -       ret = __sdhci_pci_suspend_host(chip);
> -       if (ret)
> -               return ret;
> -
> -       sdhci_pci_init_wakeup(chip);
> -
> -       return 0;
> -}
> -
>  int sdhci_pci_resume_host(struct sdhci_pci_chip *chip)
>  {
>         struct sdhci_pci_slot *slot;
> @@ -1108,7 +1094,7 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
>  {
>         int i, ret;
>
> -       ret = __sdhci_pci_suspend_host(chip);
> +       ret = sdhci_pci_suspend_host(chip);
>         if (ret)
>                 return ret;
>
> @@ -1118,8 +1104,6 @@ static int jmicron_suspend(struct sdhci_pci_chip *chip)
>                         jmicron_enable_mmc(chip->slots[i]->host, 0);
>         }
>
> -       sdhci_pci_init_wakeup(chip);
> -
>         return 0;
>  }
>
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Adrian Hunter]

On 21/12/17 16:15, Ulf Hansson wrote:
> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>> after calling sdhci_suspend_host(). So move the calls to
>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>> stop calling sdhci_enable_irq_wakeups(). That results in some
>> simplification because sdhci_pci_suspend_host() and
>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>>  1 file changed, 21 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index 110c634cfb43..2ffc09f088ee 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -39,10 +39,29 @@
>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>>
>>  #ifdef CONFIG_PM_SLEEP
>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>> +{
>> +       mmc_pm_flag_t pm_flags = 0;
>> +       int i;
>> +
>> +       for (i = 0; i < chip->num_slots; i++) {
>> +               struct sdhci_pci_slot *slot = chip->slots[i];
>> +
>> +               if (slot)
>> +                       pm_flags |= slot->host->mmc->pm_flags;
>> +       }
>> +
>> +       return device_init_wakeup(&chip->pdev->dev,
>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> 
> A couple of comments here.
> 
> Wakeup settings shouldn't be changed during system suspend. That means
> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
> for that matter) here.
> 
> Instead, device_init_wakeup() should be called during ->probe(), while
> device_set_wakeup_enable() should normally *not* have to be called at
> all by drivers. There are a exceptions for device_set_wakeup_enable(),
> however it should not have to be called during system suspend, at
> least.
> 
> Of course, I realize that you are not changing the behavior in this
> regards in this patch, so I am fine this anyway.
> 
> My point is, that the end result while doing this improvements in this
> series, should strive to that device_init_wakeup() and
> device_set_wakeup_enable() becomes used correctly. I don't think that
> is the case, or is it?

Unfortunately we don't find out what the SDIO driver wants to do (i.e
pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.

I considered enabling wakeups by default but wasn't sure that would not
increase power consumption in devices not expecting it.

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Ulf Hansson]

+ linux-pm, Rafael, Geert

On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 21/12/17 16:15, Ulf Hansson wrote:
>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>>> after calling sdhci_suspend_host(). So move the calls to
>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>>> stop calling sdhci_enable_irq_wakeups(). That results in some
>>> simplification because sdhci_pci_suspend_host() and
>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>>>  1 file changed, 21 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>> index 110c634cfb43..2ffc09f088ee 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -39,10 +39,29 @@
>>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>>>
>>>  #ifdef CONFIG_PM_SLEEP
>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>>> +{
>>> +       mmc_pm_flag_t pm_flags = 0;
>>> +       int i;
>>> +
>>> +       for (i = 0; i < chip->num_slots; i++) {
>>> +               struct sdhci_pci_slot *slot = chip->slots[i];
>>> +
>>> +               if (slot)
>>> +                       pm_flags |= slot->host->mmc->pm_flags;
>>> +       }
>>> +
>>> +       return device_init_wakeup(&chip->pdev->dev,
>>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
>>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
>>
>> A couple of comments here.
>>
>> Wakeup settings shouldn't be changed during system suspend. That means
>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
>> for that matter) here.
>>
>> Instead, device_init_wakeup() should be called during ->probe(), while
>> device_set_wakeup_enable() should normally *not* have to be called at
>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
>> however it should not have to be called during system suspend, at
>> least.
>>
>> Of course, I realize that you are not changing the behavior in this
>> regards in this patch, so I am fine this anyway.
>>
>> My point is, that the end result while doing this improvements in this
>> series, should strive to that device_init_wakeup() and
>> device_set_wakeup_enable() becomes used correctly. I don't think that
>> is the case, or is it?
>
> Unfortunately we don't find out what the SDIO driver wants to do (i.e
> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.

That's true! Of course, we need to be able to act on this, somehow.

>
> I considered enabling wakeups by default but wasn't sure that would not
> increase power consumption in devices not expecting it.

I understand the problem, believe me. However, I would rather like to
try to find a generic solution to these problems, else we will keep
abusing existing wakeups APIs.

For your information, I have been working on several issues on how to
handle the "wakeup path" correctly, which is linked to this problem.
See a few quite small series for this below [1], [2].

I think the general problems can be described liked this:

1) The dev->power.wakeup_path flag doesn't get set for the device by
the PM core, in case device_set_wakeup_enable() is called from a
->suspend() callback. That also means, that the parent device don't
get its >power.wakeup_path flag set in __device_suspend() by the PM
core, while this may be expected by upper layers (PM domains, middle
layers).

2) device_may_wakeup() doesn't tell us the hole story about the
wakeup. Only that is *may* be configured. :-)
So in cases when device_may_wakeup() returns true, we still have these
three conditions to consider, which we currently can't distinguish
between:

*) The wakeup is configured and enabled, so the device should be
enabled in the wakeup path.

**) The wakeup is configured and enabled, but as an out-band-wakeup
signal (like a GPIO IRQ). This means the device shouldn't be enabled
in the wakeup path.

***) The wakeup isn't configured, thus disabled, because there are no
consumers of the wakeup IRQ registered (as may be the case for SDIO
IRQ). This also means the device shouldn't be enabled in the wakeup
path.

Potentially, one could consider **) and ***) being treated in the same
way, via using an opt-out method, avoiding the wakeup path to be
enabled. Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
driver PM flag, to deal with this, however there may be other
preferred options.

Kind regards
Uffe

[1]
https://www.spinics.net/lists/linux-renesas-soc/msg21421.html
[2]
https://www.spinics.net/lists/linux-renesas-soc/msg20122.html
(Actually the discussions there concluded that we may need an
"OUT_BAND_WAKEUP" flag instead)

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Adrian Hunter]

On 21/12/17 17:33, Ulf Hansson wrote:
> + linux-pm, Rafael, Geert
> 
> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 21/12/17 16:15, Ulf Hansson wrote:
>>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>>>> after calling sdhci_suspend_host(). So move the calls to
>>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>>>> stop calling sdhci_enable_irq_wakeups(). That results in some
>>>> simplification because sdhci_pci_suspend_host() and
>>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>>>>  1 file changed, 21 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>>> index 110c634cfb43..2ffc09f088ee 100644
>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>> @@ -39,10 +39,29 @@
>>>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>>>>
>>>>  #ifdef CONFIG_PM_SLEEP
>>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>>>> +{
>>>> +       mmc_pm_flag_t pm_flags = 0;
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < chip->num_slots; i++) {
>>>> +               struct sdhci_pci_slot *slot = chip->slots[i];
>>>> +
>>>> +               if (slot)
>>>> +                       pm_flags |= slot->host->mmc->pm_flags;
>>>> +       }
>>>> +
>>>> +       return device_init_wakeup(&chip->pdev->dev,
>>>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
>>>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
>>>
>>> A couple of comments here.
>>>
>>> Wakeup settings shouldn't be changed during system suspend. That means
>>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
>>> for that matter) here.
>>>
>>> Instead, device_init_wakeup() should be called during ->probe(), while
>>> device_set_wakeup_enable() should normally *not* have to be called at
>>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
>>> however it should not have to be called during system suspend, at
>>> least.
>>>
>>> Of course, I realize that you are not changing the behavior in this
>>> regards in this patch, so I am fine this anyway.
>>>
>>> My point is, that the end result while doing this improvements in this
>>> series, should strive to that device_init_wakeup() and
>>> device_set_wakeup_enable() becomes used correctly. I don't think that
>>> is the case, or is it?
>>
>> Unfortunately we don't find out what the SDIO driver wants to do (i.e
>> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
> 
> That's true! Of course, we need to be able to act on this, somehow.
> 
>>
>> I considered enabling wakeups by default but wasn't sure that would not
>> increase power consumption in devices not expecting it.
> 
> I understand the problem, believe me. However, I would rather like to
> try to find a generic solution to these problems, else we will keep
> abusing existing wakeups APIs.
> 
> For your information, I have been working on several issues on how to
> handle the "wakeup path" correctly, which is linked to this problem.
> See a few quite small series for this below [1], [2].
> 
> I think the general problems can be described liked this:
> 
> 1) The dev->power.wakeup_path flag doesn't get set for the device by
> the PM core, in case device_set_wakeup_enable() is called from a
> ->suspend() callback. That also means, that the parent device don't
> get its >power.wakeup_path flag set in __device_suspend() by the PM
> core, while this may be expected by upper layers (PM domains, middle
> layers).
> 
> 2) device_may_wakeup() doesn't tell us the hole story about the
> wakeup. Only that is *may* be configured. :-)
> So in cases when device_may_wakeup() returns true, we still have these
> three conditions to consider, which we currently can't distinguish
> between:
> 
> *) The wakeup is configured and enabled, so the device should be
> enabled in the wakeup path.
> 
> **) The wakeup is configured and enabled, but as an out-band-wakeup
> signal (like a GPIO IRQ). This means the device shouldn't be enabled
> in the wakeup path.

So what about just adding can_oob_wakeup and oob_wakeup i.e.


diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 0f651efc58a1..162a511286b7 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -323,23 +323,32 @@ static ssize_t pm_qos_no_power_off_store(struct device *dev,
 static ssize_t wakeup_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
-	return sprintf(buf, "%s\n", device_can_wakeup(dev)
-		? (device_may_wakeup(dev) ? _enabled : _disabled)
-		: "");
+	return sprintf(buf, "%s\n", device_may_wakeup(dev) ||
+				    device_may_oob_wakeup(dev) ? _enabled :
+				    device_can_wakeup(dev) ||
+				    device_can_oob_wakeup(dev) ? _disabled :
+				    "");
 }
 
 static ssize_t wakeup_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t n)
 {
-	if (!device_can_wakeup(dev))
+	bool enable = false;
+
+	if (!device_can_wakeup(dev) && !device_can_oob_wakeup(dev))
 		return -EINVAL;
 
 	if (sysfs_streq(buf, _enabled))
-		device_set_wakeup_enable(dev, 1);
-	else if (sysfs_streq(buf, _disabled))
-		device_set_wakeup_enable(dev, 0);
-	else
+		enable = true;
+	else if (!sysfs_streq(buf, _disabled))
 		return -EINVAL;
+
+	if (device_can_wakeup(dev))
+		device_set_wakeup_enable(dev, enable);
+
+	if (device_can_oob_wakeup(dev))
+		device_set_oob_wakeup_enable(dev, enable);
+
 	return n;
 }
 
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index e73a081c6397..13e176edc3d7 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -392,6 +392,20 @@ int device_wakeup_disable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_wakeup_disable);
 
+static void wakeup_sysfs_add_remove(struct device *dev, bool capable)
+{
+	if (device_is_registered(dev) && !list_empty(&dev->power.entry)) {
+		if (capable) {
+			int ret = wakeup_sysfs_add(dev);
+
+			if (ret)
+				dev_info(dev, "Wakeup sysfs attributes not added\n");
+		} else {
+			wakeup_sysfs_remove(dev);
+		}
+	}
+}
+
 /**
  * device_set_wakeup_capable - Set/reset device wakeup capability flag.
  * @dev: Device to handle.
@@ -410,20 +424,35 @@ void device_set_wakeup_capable(struct device *dev, bool capable)
 		return;
 
 	dev->power.can_wakeup = capable;
-	if (device_is_registered(dev) && !list_empty(&dev->power.entry)) {
-		if (capable) {
-			int ret = wakeup_sysfs_add(dev);
-
-			if (ret)
-				dev_info(dev, "Wakeup sysfs attributes not added\n");
-		} else {
-			wakeup_sysfs_remove(dev);
-		}
-	}
+	wakeup_sysfs_add_remove(dev, capable);
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_capable);
 
 /**
+ * device_set_oob_wakeup_capable - Set/reset device oob wakeup capability flag.
+ * @dev: Device to handle.
+ * @capable: Whether or not @dev is capable of out-of-band wake up of the system
+ * from sleep.
+ *
+ * If @capable is set, set the @dev's power.can_oob_wakeup flag and add its
+ * wakeup-related attributes to sysfs.  Otherwise, unset the @dev's
+ * power.can_oob_wakeup flag and remove its wakeup-related attributes from
+ * sysfs.
+ *
+ * This function may sleep and it can't be called from any context where
+ * sleeping is not allowed.
+ */
+void device_set_oob_wakeup_capable(struct device *dev, bool capable)
+{
+	if (!!dev->power.can_oob_wakeup == !!capable)
+		return;
+
+	dev->power.can_oob_wakeup = capable;
+	wakeup_sysfs_add_remove(dev, capable);
+}
+EXPORT_SYMBOL_GPL(device_set_oob_wakeup_capable);
+
+/**
  * device_init_wakeup - Device wakeup initialization.
  * @dev: Device to handle.
  * @enable: Whether or not to enable @dev as a wakeup device.
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d8357..22cfcacc0095 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -585,6 +585,8 @@ struct pm_subsys_data {
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
+	unsigned int		can_oob_wakeup:1;
+	unsigned int		oob_wakeup:1;
 	unsigned int		async_suspend:1;
 	bool			in_dpm_list:1;	/* Owned by the PM core */
 	bool			is_prepared:1;	/* Owned by the PM core */
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 4238dde0aaf0..64252cb0f97a 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -83,11 +83,30 @@ static inline bool device_can_wakeup(struct device *dev)
 	return dev->power.can_wakeup;
 }
 
+static inline bool device_can_oob_wakeup(struct device *dev)
+{
+	return dev->power.can_oob_wakeup;
+}
+
 static inline bool device_may_wakeup(struct device *dev)
 {
 	return dev->power.can_wakeup && !!dev->power.wakeup;
 }
 
+static inline bool device_may_oob_wakeup(struct device *dev)
+{
+	return dev->power.can_oob_wakeup && dev->power.oob_wakeup;
+}
+
+static inline int device_set_oob_wakeup_enable(struct device *dev, bool enable)
+{
+	if (!dev->power.can_oob_wakeup)
+		return -EINVAL;
+
+	dev->power.oob_wakeup = enable;
+	return 0;
+}
+
 static inline void device_set_wakeup_path(struct device *dev)
 {
 	dev->power.wakeup_path = true;


> 
> ***) The wakeup isn't configured, thus disabled, because there are no
> consumers of the wakeup IRQ registered (as may be the case for SDIO
> IRQ). This also means the device shouldn't be enabled in the wakeup
> path.
> 
> Potentially, one could consider **) and ***) being treated in the same
> way, via using an opt-out method, avoiding the wakeup path to be
> enabled. Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
> driver PM flag, to deal with this, however there may be other
> preferred options.
> 
> Kind regards
> Uffe
> 
> [1]
> https://www.spinics.net/lists/linux-renesas-soc/msg21421.html
> [2]
> https://www.spinics.net/lists/linux-renesas-soc/msg20122.html
> (Actually the discussions there concluded that we may need an
> "OUT_BAND_WAKEUP" flag instead)
> 

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Rafael J. Wysocki]

On Monday, January 8, 2018 3:49:26 PM CET Adrian Hunter wrote:
> On 21/12/17 17:33, Ulf Hansson wrote:
> > + linux-pm, Rafael, Geert
> > 
> > On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> On 21/12/17 16:15, Ulf Hansson wrote:
> >>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
> >>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
> >>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
> >>>> after calling sdhci_suspend_host(). So move the calls to
> >>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
> >>>> stop calling sdhci_enable_irq_wakeups(). That results in some
> >>>> simplification because sdhci_pci_suspend_host() and
> >>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
> >>>>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>> ---
> >>>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
> >>>>  1 file changed, 21 insertions(+), 37 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >>>> index 110c634cfb43..2ffc09f088ee 100644
> >>>> --- a/drivers/mmc/host/sdhci-pci-core.c
> >>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >>>> @@ -39,10 +39,29 @@
> >>>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
> >>>>
> >>>>  #ifdef CONFIG_PM_SLEEP
> >>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> >>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> >>>> +{
> >>>> +       mmc_pm_flag_t pm_flags = 0;
> >>>> +       int i;
> >>>> +
> >>>> +       for (i = 0; i < chip->num_slots; i++) {
> >>>> +               struct sdhci_pci_slot *slot = chip->slots[i];
> >>>> +
> >>>> +               if (slot)
> >>>> +                       pm_flags |= slot->host->mmc->pm_flags;
> >>>> +       }
> >>>> +
> >>>> +       return device_init_wakeup(&chip->pdev->dev,
> >>>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> >>>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> >>>
> >>> A couple of comments here.
> >>>
> >>> Wakeup settings shouldn't be changed during system suspend. That means
> >>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
> >>> for that matter) here.
> >>>
> >>> Instead, device_init_wakeup() should be called during ->probe(), while
> >>> device_set_wakeup_enable() should normally *not* have to be called at
> >>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
> >>> however it should not have to be called during system suspend, at
> >>> least.
> >>>
> >>> Of course, I realize that you are not changing the behavior in this
> >>> regards in this patch, so I am fine this anyway.
> >>>
> >>> My point is, that the end result while doing this improvements in this
> >>> series, should strive to that device_init_wakeup() and
> >>> device_set_wakeup_enable() becomes used correctly. I don't think that
> >>> is the case, or is it?
> >>
> >> Unfortunately we don't find out what the SDIO driver wants to do (i.e
> >> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
> > 
> > That's true! Of course, we need to be able to act on this, somehow.
> > 
> >>
> >> I considered enabling wakeups by default but wasn't sure that would not
> >> increase power consumption in devices not expecting it.
> > 
> > I understand the problem, believe me. However, I would rather like to
> > try to find a generic solution to these problems, else we will keep
> > abusing existing wakeups APIs.
> > 
> > For your information, I have been working on several issues on how to
> > handle the "wakeup path" correctly, which is linked to this problem.
> > See a few quite small series for this below [1], [2].
> > 
> > I think the general problems can be described liked this:
> > 
> > 1) The dev->power.wakeup_path flag doesn't get set for the device by
> > the PM core, in case device_set_wakeup_enable() is called from a
> > ->suspend() callback. That also means, that the parent device don't
> > get its >power.wakeup_path flag set in __device_suspend() by the PM
> > core, while this may be expected by upper layers (PM domains, middle
> > layers).
> > 
> > 2) device_may_wakeup() doesn't tell us the hole story about the
> > wakeup. Only that is *may* be configured. :-)
> > So in cases when device_may_wakeup() returns true, we still have these
> > three conditions to consider, which we currently can't distinguish
> > between:
> > 
> > *) The wakeup is configured and enabled, so the device should be
> > enabled in the wakeup path.
> > 
> > **) The wakeup is configured and enabled, but as an out-band-wakeup
> > signal (like a GPIO IRQ). This means the device shouldn't be enabled
> > in the wakeup path.
> 
> So what about just adding can_oob_wakeup and oob_wakeup i.e.

Why do you want to expose that to user space?  It is entirely internal IMO.

Besides, we have that already in ACPI, for example, so that would need to be
taken into account somehow.

But this a PCI driver, right?

Anyway, please post the original series again with a CC to linux-pm.

Thanks,
Rafael

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Rafael J. Wysocki]

On Thursday, December 21, 2017 4:33:59 PM CET Ulf Hansson wrote:
> + linux-pm, Rafael, Geert
> 
> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> > On 21/12/17 16:15, Ulf Hansson wrote:
> >> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
> >>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
> >>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
> >>> after calling sdhci_suspend_host(). So move the calls to
> >>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
> >>> stop calling sdhci_enable_irq_wakeups(). That results in some
> >>> simplification because sdhci_pci_suspend_host() and
> >>> __sdhci_pci_suspend_host() no longer need to be separate functions.
> >>>
> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>> ---
> >>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
> >>>  1 file changed, 21 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >>> index 110c634cfb43..2ffc09f088ee 100644
> >>> --- a/drivers/mmc/host/sdhci-pci-core.c
> >>> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >>> @@ -39,10 +39,29 @@
> >>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
> >>>
> >>>  #ifdef CONFIG_PM_SLEEP
> >>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> >>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> >>> +{
> >>> +       mmc_pm_flag_t pm_flags = 0;
> >>> +       int i;
> >>> +
> >>> +       for (i = 0; i < chip->num_slots; i++) {
> >>> +               struct sdhci_pci_slot *slot = chip->slots[i];
> >>> +
> >>> +               if (slot)
> >>> +                       pm_flags |= slot->host->mmc->pm_flags;
> >>> +       }
> >>> +
> >>> +       return device_init_wakeup(&chip->pdev->dev,
> >>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> >>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> >>
> >> A couple of comments here.
> >>
> >> Wakeup settings shouldn't be changed during system suspend. That means
> >> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
> >> for that matter) here.
> >>
> >> Instead, device_init_wakeup() should be called during ->probe(), while
> >> device_set_wakeup_enable() should normally *not* have to be called at
> >> all by drivers. There are a exceptions for device_set_wakeup_enable(),
> >> however it should not have to be called during system suspend, at
> >> least.
> >>
> >> Of course, I realize that you are not changing the behavior in this
> >> regards in this patch, so I am fine this anyway.
> >>
> >> My point is, that the end result while doing this improvements in this
> >> series, should strive to that device_init_wakeup() and
> >> device_set_wakeup_enable() becomes used correctly. I don't think that
> >> is the case, or is it?
> >
> > Unfortunately we don't find out what the SDIO driver wants to do (i.e
> > pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
> 
> That's true! Of course, we need to be able to act on this, somehow.

That sounds like a design issue, however ...

> >
> > I considered enabling wakeups by default but wasn't sure that would not
> > increase power consumption in devices not expecting it.
> 
> I understand the problem, believe me. However, I would rather like to
> try to find a generic solution to these problems, else we will keep
> abusing existing wakeups APIs.
> 
> For your information, I have been working on several issues on how to
> handle the "wakeup path" correctly, which is linked to this problem.
> See a few quite small series for this below [1], [2].
> 
> I think the general problems can be described liked this:
> 
> 1) The dev->power.wakeup_path flag doesn't get set for the device by
> the PM core, in case device_set_wakeup_enable() is called from a
> ->suspend() callback. That also means, that the parent device don't
> get its >power.wakeup_path flag set in __device_suspend() by the PM
> core, while this may be expected by upper layers (PM domains, middle
> layers).
> 
> 2) device_may_wakeup() doesn't tell us the hole story about the
> wakeup. Only that is *may* be configured. :-)
> So in cases when device_may_wakeup() returns true, we still have these
> three conditions to consider, which we currently can't distinguish
> between:
> 
> *) The wakeup is configured and enabled, so the device should be
> enabled in the wakeup path.

That's when the in-band device interrupt is used for wakeup, right?

> **) The wakeup is configured and enabled, but as an out-band-wakeup
> signal (like a GPIO IRQ). This means the device shouldn't be enabled
> in the wakeup path.

I wouldn't really call all of that out of band, but really there's more to it
in general.

There may be a standard way to configure wakeup signaling which is handled by
a separate code layer, like ACPI or PCI.  In that case the driver basically
doesn't care (but it still may need to configure the device for remote
wakeup internally).  That standard way may do whatever the driver would do
to the wakeup interrupt or it may use an out-of-band mechanism.

If there is no standard way, the driver may need to set up things by itself.
In that case it needs to know what's available and how to use it, essentially
at probe time.  I guess the information then would need to come from a DT or
similar.

If it knows that it will use the sideband thing, it will not do *) obviously.

> ***) The wakeup isn't configured, thus disabled, because there are no
> consumers of the wakeup IRQ registered (as may be the case for SDIO
> IRQ). This also means the device shouldn't be enabled in the wakeup
> path.
> 
> Potentially, one could consider **) and ***) being treated in the same
> way, via using an opt-out method, avoiding the wakeup path to be
> enabled.

Which means exactly what?

> Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
> driver PM flag, to deal with this, however there may be other
> preferred options.

Yeah.  Messy stuff.

Thanks,
Rafael

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Ulf Hansson]

On 9 January 2018 at 01:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, December 21, 2017 4:33:59 PM CET Ulf Hansson wrote:
>> + linux-pm, Rafael, Geert
>>
>> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> > On 21/12/17 16:15, Ulf Hansson wrote:
>> >> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> >>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>> >>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>> >>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>> >>> after calling sdhci_suspend_host(). So move the calls to
>> >>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>> >>> stop calling sdhci_enable_irq_wakeups(). That results in some
>> >>> simplification because sdhci_pci_suspend_host() and
>> >>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>> >>>
>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> >>> ---
>> >>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>> >>>  1 file changed, 21 insertions(+), 37 deletions(-)
>> >>>
>> >>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> >>> index 110c634cfb43..2ffc09f088ee 100644
>> >>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> >>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> >>> @@ -39,10 +39,29 @@
>> >>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>> >>>
>> >>>  #ifdef CONFIG_PM_SLEEP
>> >>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>> >>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>> >>> +{
>> >>> +       mmc_pm_flag_t pm_flags = 0;
>> >>> +       int i;
>> >>> +
>> >>> +       for (i = 0; i < chip->num_slots; i++) {
>> >>> +               struct sdhci_pci_slot *slot = chip->slots[i];
>> >>> +
>> >>> +               if (slot)
>> >>> +                       pm_flags |= slot->host->mmc->pm_flags;
>> >>> +       }
>> >>> +
>> >>> +       return device_init_wakeup(&chip->pdev->dev,
>> >>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
>> >>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
>> >>
>> >> A couple of comments here.
>> >>
>> >> Wakeup settings shouldn't be changed during system suspend. That means
>> >> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
>> >> for that matter) here.
>> >>
>> >> Instead, device_init_wakeup() should be called during ->probe(), while
>> >> device_set_wakeup_enable() should normally *not* have to be called at
>> >> all by drivers. There are a exceptions for device_set_wakeup_enable(),
>> >> however it should not have to be called during system suspend, at
>> >> least.
>> >>
>> >> Of course, I realize that you are not changing the behavior in this
>> >> regards in this patch, so I am fine this anyway.
>> >>
>> >> My point is, that the end result while doing this improvements in this
>> >> series, should strive to that device_init_wakeup() and
>> >> device_set_wakeup_enable() becomes used correctly. I don't think that
>> >> is the case, or is it?
>> >
>> > Unfortunately we don't find out what the SDIO driver wants to do (i.e
>> > pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
>>
>> That's true! Of course, we need to be able to act on this, somehow.
>
> That sounds like a design issue, however ...
>
>> >
>> > I considered enabling wakeups by default but wasn't sure that would not
>> > increase power consumption in devices not expecting it.
>>
>> I understand the problem, believe me. However, I would rather like to
>> try to find a generic solution to these problems, else we will keep
>> abusing existing wakeups APIs.
>>
>> For your information, I have been working on several issues on how to
>> handle the "wakeup path" correctly, which is linked to this problem.
>> See a few quite small series for this below [1], [2].
>>
>> I think the general problems can be described liked this:
>>
>> 1) The dev->power.wakeup_path flag doesn't get set for the device by
>> the PM core, in case device_set_wakeup_enable() is called from a
>> ->suspend() callback. That also means, that the parent device don't
>> get its >power.wakeup_path flag set in __device_suspend() by the PM
>> core, while this may be expected by upper layers (PM domains, middle
>> layers).
>>
>> 2) device_may_wakeup() doesn't tell us the hole story about the
>> wakeup. Only that is *may* be configured. :-)
>> So in cases when device_may_wakeup() returns true, we still have these
>> three conditions to consider, which we currently can't distinguish
>> between:
>>
>> *) The wakeup is configured and enabled, so the device should be
>> enabled in the wakeup path.
>
> That's when the in-band device interrupt is used for wakeup, right?

Yes.

>
>> **) The wakeup is configured and enabled, but as an out-band-wakeup
>> signal (like a GPIO IRQ). This means the device shouldn't be enabled
>> in the wakeup path.
>
> I wouldn't really call all of that out of band, but really there's more to it
> in general.
>
> There may be a standard way to configure wakeup signaling which is handled by
> a separate code layer, like ACPI or PCI.  In that case the driver basically
> doesn't care (but it still may need to configure the device for remote
> wakeup internally).  That standard way may do whatever the driver would do
> to the wakeup interrupt or it may use an out-of-band mechanism.

Even if this is a PCI-MMC/SD driver, we can also have wakeup GPIO
irqs, for example to handle card detect when inserting/removing an SD
card.

The point is, there are cases when the driver has this of information,
but upper layer doesn't.

>
> If there is no standard way, the driver may need to set up things by itself.
> In that case it needs to know what's available and how to use it, essentially
> at probe time.  I guess the information then would need to come from a DT or
> similar.

Right, on those SoC I mostly work on, this data comes from DT.
However, on some SoCs this data come from platform data as well.

>
> If it knows that it will use the sideband thing, it will not do *) obviously.

Right, however device_may_wakeup() may still return true for the device.

>
>> ***) The wakeup isn't configured, thus disabled, because there are no
>> consumers of the wakeup IRQ registered (as may be the case for SDIO
>> IRQ). This also means the device shouldn't be enabled in the wakeup
>> path.
>>
>> Potentially, one could consider **) and ***) being treated in the same
>> way, via using an opt-out method, avoiding the wakeup path to be
>> enabled.
>
> Which means exactly what?

>From the driver perspective, it decides to put the device into low
power state (clocks will be gated, etc), thus upper layers (PM domain)
should be informed that it can do that too.

>
>> Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
>> driver PM flag, to deal with this, however there may be other
>> preferred options.
>
> Yeah.  Messy stuff.

Well, let's try to agree on how to best describe the problem first. It
seems like we are soon getting there. :-)

Kind regards
Uffe

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Adrian Hunter]

On 09/01/18 02:26, Rafael J. Wysocki wrote:
> On Monday, January 8, 2018 3:49:26 PM CET Adrian Hunter wrote:
>> On 21/12/17 17:33, Ulf Hansson wrote:
>>> + linux-pm, Rafael, Geert
>>>
>>> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 21/12/17 16:15, Ulf Hansson wrote:
>>>>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>>>>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>>>>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>>>>>> after calling sdhci_suspend_host(). So move the calls to
>>>>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>>>>>> stop calling sdhci_enable_irq_wakeups(). That results in some
>>>>>> simplification because sdhci_pci_suspend_host() and
>>>>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>>>>>>
>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>>> ---
>>>>>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>>>>>>  1 file changed, 21 insertions(+), 37 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>>>>> index 110c634cfb43..2ffc09f088ee 100644
>>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>>>> @@ -39,10 +39,29 @@
>>>>>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
>>>>>>
>>>>>>  #ifdef CONFIG_PM_SLEEP
>>>>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>>>>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>>>>>> +{
>>>>>> +       mmc_pm_flag_t pm_flags = 0;
>>>>>> +       int i;
>>>>>> +
>>>>>> +       for (i = 0; i < chip->num_slots; i++) {
>>>>>> +               struct sdhci_pci_slot *slot = chip->slots[i];
>>>>>> +
>>>>>> +               if (slot)
>>>>>> +                       pm_flags |= slot->host->mmc->pm_flags;
>>>>>> +       }
>>>>>> +
>>>>>> +       return device_init_wakeup(&chip->pdev->dev,
>>>>>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
>>>>>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
>>>>>
>>>>> A couple of comments here.
>>>>>
>>>>> Wakeup settings shouldn't be changed during system suspend. That means
>>>>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
>>>>> for that matter) here.
>>>>>
>>>>> Instead, device_init_wakeup() should be called during ->probe(), while
>>>>> device_set_wakeup_enable() should normally *not* have to be called at
>>>>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
>>>>> however it should not have to be called during system suspend, at
>>>>> least.
>>>>>
>>>>> Of course, I realize that you are not changing the behavior in this
>>>>> regards in this patch, so I am fine this anyway.
>>>>>
>>>>> My point is, that the end result while doing this improvements in this
>>>>> series, should strive to that device_init_wakeup() and
>>>>> device_set_wakeup_enable() becomes used correctly. I don't think that
>>>>> is the case, or is it?
>>>>
>>>> Unfortunately we don't find out what the SDIO driver wants to do (i.e
>>>> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
>>>
>>> That's true! Of course, we need to be able to act on this, somehow.
>>>
>>>>
>>>> I considered enabling wakeups by default but wasn't sure that would not
>>>> increase power consumption in devices not expecting it.
>>>
>>> I understand the problem, believe me. However, I would rather like to
>>> try to find a generic solution to these problems, else we will keep
>>> abusing existing wakeups APIs.
>>>
>>> For your information, I have been working on several issues on how to
>>> handle the "wakeup path" correctly, which is linked to this problem.
>>> See a few quite small series for this below [1], [2].
>>>
>>> I think the general problems can be described liked this:
>>>
>>> 1) The dev->power.wakeup_path flag doesn't get set for the device by
>>> the PM core, in case device_set_wakeup_enable() is called from a
>>> ->suspend() callback. That also means, that the parent device don't
>>> get its >power.wakeup_path flag set in __device_suspend() by the PM
>>> core, while this may be expected by upper layers (PM domains, middle
>>> layers).
>>>
>>> 2) device_may_wakeup() doesn't tell us the hole story about the
>>> wakeup. Only that is *may* be configured. :-)
>>> So in cases when device_may_wakeup() returns true, we still have these
>>> three conditions to consider, which we currently can't distinguish
>>> between:
>>>
>>> *) The wakeup is configured and enabled, so the device should be
>>> enabled in the wakeup path.
>>>
>>> **) The wakeup is configured and enabled, but as an out-band-wakeup
>>> signal (like a GPIO IRQ). This means the device shouldn't be enabled
>>> in the wakeup path.
>>
>> So what about just adding can_oob_wakeup and oob_wakeup i.e.
> 
> Why do you want to expose that to user space?  It is entirely internal IMO.

It was not exposed to user space.  The idea was to use the same power/wake
attribute for both "normal" and oob wake.

The user doesn't care whether the wakeup is native to the device or
performed by GPIO.  So it seems reasonable to use the power/wake attribute.

> Besides, we have that already in ACPI, for example, so that would need to be
> taken into account somehow.

What in ACPI are you referring to?

> But this a PCI driver, right?

Yes.

> Anyway, please post the original series again with a CC to linux-pm.

Done.

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Rafael J. Wysocki]

On Tuesday, January 9, 2018 8:14:37 AM CET Ulf Hansson wrote:
> On 9 January 2018 at 01:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, December 21, 2017 4:33:59 PM CET Ulf Hansson wrote:
> >> + linux-pm, Rafael, Geert
> >>
> >> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> > On 21/12/17 16:15, Ulf Hansson wrote:
> >> >> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> >>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
> >> >>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
> >> >>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
> >> >>> after calling sdhci_suspend_host(). So move the calls to
> >> >>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
> >> >>> stop calling sdhci_enable_irq_wakeups(). That results in some
> >> >>> simplification because sdhci_pci_suspend_host() and
> >> >>> __sdhci_pci_suspend_host() no longer need to be separate functions.
> >> >>>
> >> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> >>> ---
> >> >>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
> >> >>>  1 file changed, 21 insertions(+), 37 deletions(-)
> >> >>>
> >> >>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >> >>> index 110c634cfb43..2ffc09f088ee 100644
> >> >>> --- a/drivers/mmc/host/sdhci-pci-core.c
> >> >>> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >> >>> @@ -39,10 +39,29 @@
> >> >>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
> >> >>>
> >> >>>  #ifdef CONFIG_PM_SLEEP
> >> >>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> >> >>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> >> >>> +{
> >> >>> +       mmc_pm_flag_t pm_flags = 0;
> >> >>> +       int i;
> >> >>> +
> >> >>> +       for (i = 0; i < chip->num_slots; i++) {
> >> >>> +               struct sdhci_pci_slot *slot = chip->slots[i];
> >> >>> +
> >> >>> +               if (slot)
> >> >>> +                       pm_flags |= slot->host->mmc->pm_flags;
> >> >>> +       }
> >> >>> +
> >> >>> +       return device_init_wakeup(&chip->pdev->dev,
> >> >>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> >> >>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> >> >>
> >> >> A couple of comments here.
> >> >>
> >> >> Wakeup settings shouldn't be changed during system suspend. That means
> >> >> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
> >> >> for that matter) here.
> >> >>
> >> >> Instead, device_init_wakeup() should be called during ->probe(), while
> >> >> device_set_wakeup_enable() should normally *not* have to be called at
> >> >> all by drivers. There are a exceptions for device_set_wakeup_enable(),
> >> >> however it should not have to be called during system suspend, at
> >> >> least.
> >> >>
> >> >> Of course, I realize that you are not changing the behavior in this
> >> >> regards in this patch, so I am fine this anyway.
> >> >>
> >> >> My point is, that the end result while doing this improvements in this
> >> >> series, should strive to that device_init_wakeup() and
> >> >> device_set_wakeup_enable() becomes used correctly. I don't think that
> >> >> is the case, or is it?
> >> >
> >> > Unfortunately we don't find out what the SDIO driver wants to do (i.e
> >> > pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
> >>
> >> That's true! Of course, we need to be able to act on this, somehow.
> >
> > That sounds like a design issue, however ...
> >
> >> >
> >> > I considered enabling wakeups by default but wasn't sure that would not
> >> > increase power consumption in devices not expecting it.
> >>
> >> I understand the problem, believe me. However, I would rather like to
> >> try to find a generic solution to these problems, else we will keep
> >> abusing existing wakeups APIs.
> >>
> >> For your information, I have been working on several issues on how to
> >> handle the "wakeup path" correctly, which is linked to this problem.
> >> See a few quite small series for this below [1], [2].
> >>
> >> I think the general problems can be described liked this:
> >>
> >> 1) The dev->power.wakeup_path flag doesn't get set for the device by
> >> the PM core, in case device_set_wakeup_enable() is called from a
> >> ->suspend() callback. That also means, that the parent device don't
> >> get its >power.wakeup_path flag set in __device_suspend() by the PM
> >> core, while this may be expected by upper layers (PM domains, middle
> >> layers).
> >>
> >> 2) device_may_wakeup() doesn't tell us the hole story about the
> >> wakeup. Only that is *may* be configured. :-)
> >> So in cases when device_may_wakeup() returns true, we still have these
> >> three conditions to consider, which we currently can't distinguish
> >> between:
> >>
> >> *) The wakeup is configured and enabled, so the device should be
> >> enabled in the wakeup path.
> >
> > That's when the in-band device interrupt is used for wakeup, right?
> 
> Yes.
> 
> >
> >> **) The wakeup is configured and enabled, but as an out-band-wakeup
> >> signal (like a GPIO IRQ). This means the device shouldn't be enabled
> >> in the wakeup path.
> >
> > I wouldn't really call all of that out of band, but really there's more to it
> > in general.
> >
> > There may be a standard way to configure wakeup signaling which is handled by
> > a separate code layer, like ACPI or PCI.  In that case the driver basically
> > doesn't care (but it still may need to configure the device for remote
> > wakeup internally).  That standard way may do whatever the driver would do
> > to the wakeup interrupt or it may use an out-of-band mechanism.
> 
> Even if this is a PCI-MMC/SD driver, we can also have wakeup GPIO
> irqs, for example to handle card detect when inserting/removing an SD
> card.
> 
> The point is, there are cases when the driver has this of information,
> but upper layer doesn't.

In the PCI case that is an extra wakeup mechanism that the upper layer doesn't
need to know about.

In other cases it may be the only one in which the upper layer may or may not
be interested.

I guess the point is, rather, that the upper layer may not be able to make
decisions based on the value of device_may_wakeup() alone, which is fair
enough, but then how to provide it with the extra information and what that
information should be still is a matter of discussion IMO.

You seem to be focusing on the wakeup_path approach which hasn't even been
designed for that.  Setting wakeup_path doesn't really tell the upper layers
what wakeup mechanism exactly is going to be used, does it?

So maybe we should try to define the problem more precisely ...

> >
> > If there is no standard way, the driver may need to set up things by itself.
> > In that case it needs to know what's available and how to use it, essentially
> > at probe time.  I guess the information then would need to come from a DT or
> > similar.
> 
> Right, on those SoC I mostly work on, this data comes from DT.
> However, on some SoCs this data come from platform data as well.

That doesn't matter.  In any case that information has to be available to the
driver at the probe time.

> >
> > If it knows that it will use the sideband thing, it will not do *) obviously.
> 
> Right, however device_may_wakeup() may still return true for the device.

Sure, it may.

> >
> >> ***) The wakeup isn't configured, thus disabled, because there are no
> >> consumers of the wakeup IRQ registered (as may be the case for SDIO
> >> IRQ). This also means the device shouldn't be enabled in the wakeup
> >> path.
> >>
> >> Potentially, one could consider **) and ***) being treated in the same
> >> way, via using an opt-out method, avoiding the wakeup path to be
> >> enabled.
> >
> > Which means exactly what?
> 
> From the driver perspective, it decides to put the device into low
> power state (clocks will be gated, etc), thus upper layers (PM domain)
> should be informed that it can do that too.

Right, but that's not the whole story.

The upper layer may or may not recognize the concept of "wakeup".  The PCI
bus type and the ACPI PM domain do recognize it and in their view it is the
standard wakeup mechanisms to be used.  Everything beyond that is not
recognized.  genpd doesn't really understand wakeup.  It only understands
turning off/on domains and stopping/starting devices (and that may or may
not understand wakeup, but the framework is oblivious to that).  So whatever
mechanism you think about should be generic enough to match all of these
different cases.

Moreover, in fact, it should cover runtime PM too, because "wakeup" is not
just limited to system-wide PM.

So maybe let's just take a step back and have a deeper look at things to
see what *really* needs to be addressed.

> >
> >> Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
> >> driver PM flag, to deal with this, however there may be other
> >> preferred options.
> >
> > Yeah.  Messy stuff.
> 
> Well, let's try to agree on how to best describe the problem first.

Exactly.

> It seems like we are soon getting there. :-)

Well, we'll see.

Thanks,
Rafael

Re: [PATCH 1/9] mmc: sdhci-pci: Stop calling sdhci_enable_irq_wakeups()

[Rafael J. Wysocki]

On Tuesday, January 9, 2018 9:08:46 AM CET Adrian Hunter wrote:
> On 09/01/18 02:26, Rafael J. Wysocki wrote:
> > On Monday, January 8, 2018 3:49:26 PM CET Adrian Hunter wrote:
> >> On 21/12/17 17:33, Ulf Hansson wrote:
> >>> + linux-pm, Rafael, Geert
> >>>
> >>> On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>> On 21/12/17 16:15, Ulf Hansson wrote:
> >>>>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
> >>>>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
> >>>>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
> >>>>>> after calling sdhci_suspend_host(). So move the calls to
> >>>>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
> >>>>>> stop calling sdhci_enable_irq_wakeups(). That results in some
> >>>>>> simplification because sdhci_pci_suspend_host() and
> >>>>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
> >>>>>>
> >>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>>>> ---
> >>>>>>  drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
> >>>>>>  1 file changed, 21 insertions(+), 37 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> index 110c634cfb43..2ffc09f088ee 100644
> >>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
> >>>>>> @@ -39,10 +39,29 @@
> >>>>>>  static void sdhci_pci_hw_reset(struct sdhci_host *host);
> >>>>>>
> >>>>>>  #ifdef CONFIG_PM_SLEEP
> >>>>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
> >>>>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
> >>>>>> +{
> >>>>>> +       mmc_pm_flag_t pm_flags = 0;
> >>>>>> +       int i;
> >>>>>> +
> >>>>>> +       for (i = 0; i < chip->num_slots; i++) {
> >>>>>> +               struct sdhci_pci_slot *slot = chip->slots[i];
> >>>>>> +
> >>>>>> +               if (slot)
> >>>>>> +                       pm_flags |= slot->host->mmc->pm_flags;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       return device_init_wakeup(&chip->pdev->dev,
> >>>>>> +                                 (pm_flags & MMC_PM_KEEP_POWER) &&
> >>>>>> +                                 (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
> >>>>>
> >>>>> A couple of comments here.
> >>>>>
> >>>>> Wakeup settings shouldn't be changed during system suspend. That means
> >>>>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
> >>>>> for that matter) here.
> >>>>>
> >>>>> Instead, device_init_wakeup() should be called during ->probe(), while
> >>>>> device_set_wakeup_enable() should normally *not* have to be called at
> >>>>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
> >>>>> however it should not have to be called during system suspend, at
> >>>>> least.
> >>>>>
> >>>>> Of course, I realize that you are not changing the behavior in this
> >>>>> regards in this patch, so I am fine this anyway.
> >>>>>
> >>>>> My point is, that the end result while doing this improvements in this
> >>>>> series, should strive to that device_init_wakeup() and
> >>>>> device_set_wakeup_enable() becomes used correctly. I don't think that
> >>>>> is the case, or is it?
> >>>>
> >>>> Unfortunately we don't find out what the SDIO driver wants to do (i.e
> >>>> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
> >>>
> >>> That's true! Of course, we need to be able to act on this, somehow.
> >>>
> >>>>
> >>>> I considered enabling wakeups by default but wasn't sure that would not
> >>>> increase power consumption in devices not expecting it.
> >>>
> >>> I understand the problem, believe me. However, I would rather like to
> >>> try to find a generic solution to these problems, else we will keep
> >>> abusing existing wakeups APIs.
> >>>
> >>> For your information, I have been working on several issues on how to
> >>> handle the "wakeup path" correctly, which is linked to this problem.
> >>> See a few quite small series for this below [1], [2].
> >>>
> >>> I think the general problems can be described liked this:
> >>>
> >>> 1) The dev->power.wakeup_path flag doesn't get set for the device by
> >>> the PM core, in case device_set_wakeup_enable() is called from a
> >>> ->suspend() callback. That also means, that the parent device don't
> >>> get its >power.wakeup_path flag set in __device_suspend() by the PM
> >>> core, while this may be expected by upper layers (PM domains, middle
> >>> layers).
> >>>
> >>> 2) device_may_wakeup() doesn't tell us the hole story about the
> >>> wakeup. Only that is *may* be configured. :-)
> >>> So in cases when device_may_wakeup() returns true, we still have these
> >>> three conditions to consider, which we currently can't distinguish
> >>> between:
> >>>
> >>> *) The wakeup is configured and enabled, so the device should be
> >>> enabled in the wakeup path.
> >>>
> >>> **) The wakeup is configured and enabled, but as an out-band-wakeup
> >>> signal (like a GPIO IRQ). This means the device shouldn't be enabled
> >>> in the wakeup path.
> >>
> >> So what about just adding can_oob_wakeup and oob_wakeup i.e.
> > 
> > Why do you want to expose that to user space?  It is entirely internal IMO.
> 
> It was not exposed to user space.  The idea was to use the same power/wake
> attribute for both "normal" and oob wake.
> 
> The user doesn't care whether the wakeup is native to the device or
> performed by GPIO.  So it seems reasonable to use the power/wake attribute.

I agree.  The kernel has to figure out which wakeup mechanism(s) to use.

> > Besides, we have that already in ACPI, for example, so that would need to be
> > taken into account somehow.
> 
> What in ACPI are you referring to?

Well, the normal ACPI stuff.

On a full ACPI HW system if the device has an ACPI companion object and there is
a _PRW under it (and it is actually valid), then it points to the GPE that will
be used for signaling wakeup and from the device's perspective that is out of
band.  That is very much like having a sideband wakeup GPIO IRQ (except that
the IRQ is muxed into the SCI in the ACPI GPE case).

> > But this a PCI driver, right?
> 
> Yes.
> 
> > Anyway, please post the original series again with a CC to linux-pm.
> 
> Done.

Cool, thanks!

I'll have a look at that soon (hopefully), but it looks like everybody had
the idea to send me stuff at the same time. :-)

Thanks,
Rafael