[PATCH 0/3] PM / wakeirq: fix wake irq arming

[PATCH 0/3] PM / wakeirq: fix wake irq arming

[Johan Hovold]

When reviewing the Qualcomm serial-driver suspend implementation I
noticed the odd runtime PM state update which had snuck in. Turns out it
was added to work around a bug in PM core which prevented drivers not
implementing runtime PM from using dedicated wake irqs.

This series fixes the wake irq arming and drops the unused wake irq
enable helpers before dropping the bogus runtime PM state update in the
Qualcomm driver.

I suggest that Rafael takes all of these through his tree.

Johan


Johan Hovold (3):
  PM / wakeirq: fix wake irq arming
  PM / wakeirq: drop unused enable helpers
  serial: qcom-geni: drop bogus runtime pm state update

 drivers/base/power/power.h            |  1 +
 drivers/base/power/wakeirq.c          | 61 ++++-----------------------
 drivers/tty/serial/qcom_geni_serial.c |  7 ---
 include/linux/pm_wakeirq.h            | 10 -----
 4 files changed, 9 insertions(+), 70 deletions(-)

-- 
2.41.0

[PATCH 1/3] PM / wakeirq: fix wake irq arming

[Johan Hovold]

The decision whether to enable a wake irq during suspend can not be done
based on the runtime PM state directly as a driver may use wake irqs
without implementing runtime PM. Such drivers specifically leave the
state set to the default 'suspended' and the wake irq is thus never
enabled at suspend.

Add a new wake irq flag to track whether a dedicated wake irq has been
enabled at runtime suspend and therefore must not be enabled at system
suspend.

Note that pm_runtime_enabled() can not be used as runtime PM is always
disabled during late suspend.

Fixes: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq")
Cc: stable@vger.kernel.org      # 4.16
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/base/power/power.h   |  1 +
 drivers/base/power/wakeirq.c | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 0eb7f02b3ad5..922ed457db19 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -29,6 +29,7 @@ extern u64 pm_runtime_active_time(struct device *dev);
 #define WAKE_IRQ_DEDICATED_MASK		(WAKE_IRQ_DEDICATED_ALLOCATED | \
 					 WAKE_IRQ_DEDICATED_MANAGED | \
 					 WAKE_IRQ_DEDICATED_REVERSE)
+#define WAKE_IRQ_DEDICATED_ENABLED	BIT(3)
 
 struct wake_irq {
 	struct device *dev;
diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
index d487a6bac630..afd094dec5ca 100644
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -314,8 +314,10 @@ void dev_pm_enable_wake_irq_check(struct device *dev,
 	return;
 
 enable:
-	if (!can_change_status || !(wirq->status & WAKE_IRQ_DEDICATED_REVERSE))
+	if (!can_change_status || !(wirq->status & WAKE_IRQ_DEDICATED_REVERSE)) {
 		enable_irq(wirq->irq);
+		wirq->status |= WAKE_IRQ_DEDICATED_ENABLED;
+	}
 }
 
 /**
@@ -336,8 +338,10 @@ void dev_pm_disable_wake_irq_check(struct device *dev, bool cond_disable)
 	if (cond_disable && (wirq->status & WAKE_IRQ_DEDICATED_REVERSE))
 		return;
 
-	if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED)
+	if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED) {
+		wirq->status &= ~WAKE_IRQ_DEDICATED_ENABLED;
 		disable_irq_nosync(wirq->irq);
+	}
 }
 
 /**
@@ -376,7 +380,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
 
 	if (device_may_wakeup(wirq->dev)) {
 		if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED &&
-		    !pm_runtime_status_suspended(wirq->dev))
+		    !(wirq->status & WAKE_IRQ_DEDICATED_ENABLED))
 			enable_irq(wirq->irq);
 
 		enable_irq_wake(wirq->irq);
@@ -399,7 +403,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
 		disable_irq_wake(wirq->irq);
 
 		if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED &&
-		    !pm_runtime_status_suspended(wirq->dev))
+		    !(wirq->status & WAKE_IRQ_DEDICATED_ENABLED))
 			disable_irq_nosync(wirq->irq);
 	}
 }
-- 
2.41.0

Re: [PATCH 1/3] PM / wakeirq: fix wake irq arming

[Tony Lindgren]

* Johan Hovold <johan+linaro@kernel.org> [230713 15:01]:
> The decision whether to enable a wake irq during suspend can not be done
> based on the runtime PM state directly as a driver may use wake irqs
> without implementing runtime PM. Such drivers specifically leave the
> state set to the default 'suspended' and the wake irq is thus never
> enabled at suspend.
> 
> Add a new wake irq flag to track whether a dedicated wake irq has been
> enabled at runtime suspend and therefore must not be enabled at system
> suspend.
> 
> Note that pm_runtime_enabled() can not be used as runtime PM is always
> disabled during late suspend.

Works for me:

Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>

[PATCH 2/3] PM / wakeirq: drop unused enable helpers

[Johan Hovold]

Drop the wake-irq enable and disable helpers which have not been used
since commit bed570307ed7 ("PM / wakeirq: Fix dedicated wakeirq for
drivers not using autosuspend").

Note that these functions are essentially just leftovers from the first
iteration of the wake-irq implementation where device drivers were
supposed to call these functions themselves instead of PM core (as
is also indicated by the bogus kernel doc comments).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/base/power/wakeirq.c | 49 ------------------------------------
 include/linux/pm_wakeirq.h   | 10 --------
 2 files changed, 59 deletions(-)

diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
index afd094dec5ca..42171f766dcb 100644
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -194,7 +194,6 @@ static int __dev_pm_set_dedicated_wake_irq(struct device *dev, int irq, unsigned
 	return err;
 }
 
-
 /**
  * dev_pm_set_dedicated_wake_irq - Request a dedicated wake-up interrupt
  * @dev: Device entry
@@ -206,11 +205,6 @@ static int __dev_pm_set_dedicated_wake_irq(struct device *dev, int irq, unsigned
  * Sets up a threaded interrupt handler for a device that has
  * a dedicated wake-up interrupt in addition to the device IO
  * interrupt.
- *
- * The interrupt starts disabled, and needs to be managed for
- * the device by the bus code or the device driver using
- * dev_pm_enable_wake_irq*() and dev_pm_disable_wake_irq*()
- * functions.
  */
 int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
 {
@@ -232,11 +226,6 @@ EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq);
  * the status of WAKE_IRQ_DEDICATED_REVERSE to tell rpm_suspend()
  * to enable dedicated wake-up interrupt after running the runtime suspend
  * callback for @dev.
- *
- * The interrupt starts disabled, and needs to be managed for
- * the device by the bus code or the device driver using
- * dev_pm_enable_wake_irq*() and dev_pm_disable_wake_irq*()
- * functions.
  */
 int dev_pm_set_dedicated_wake_irq_reverse(struct device *dev, int irq)
 {
@@ -244,44 +233,6 @@ int dev_pm_set_dedicated_wake_irq_reverse(struct device *dev, int irq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq_reverse);
 
-/**
- * dev_pm_enable_wake_irq - Enable device wake-up interrupt
- * @dev: Device
- *
- * Optionally called from the bus code or the device driver for
- * runtime_resume() to override the PM runtime core managed wake-up
- * interrupt handling to enable the wake-up interrupt.
- *
- * Note that for runtime_suspend()) the wake-up interrupts
- * should be unconditionally enabled unlike for suspend()
- * that is conditional.
- */
-void dev_pm_enable_wake_irq(struct device *dev)
-{
-	struct wake_irq *wirq = dev->power.wakeirq;
-
-	if (wirq && (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED))
-		enable_irq(wirq->irq);
-}
-EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
-
-/**
- * dev_pm_disable_wake_irq - Disable device wake-up interrupt
- * @dev: Device
- *
- * Optionally called from the bus code or the device driver for
- * runtime_suspend() to override the PM runtime core managed wake-up
- * interrupt handling to disable the wake-up interrupt.
- */
-void dev_pm_disable_wake_irq(struct device *dev)
-{
-	struct wake_irq *wirq = dev->power.wakeirq;
-
-	if (wirq && (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED))
-		disable_irq_nosync(wirq->irq);
-}
-EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
-
 /**
  * dev_pm_enable_wake_irq_check - Checks and enables wake-up interrupt
  * @dev: Device
diff --git a/include/linux/pm_wakeirq.h b/include/linux/pm_wakeirq.h
index dd42d16945d0..d9642c6cf852 100644
--- a/include/linux/pm_wakeirq.h
+++ b/include/linux/pm_wakeirq.h
@@ -10,8 +10,6 @@ extern int dev_pm_set_wake_irq(struct device *dev, int irq);
 extern int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq);
 extern int dev_pm_set_dedicated_wake_irq_reverse(struct device *dev, int irq);
 extern void dev_pm_clear_wake_irq(struct device *dev);
-extern void dev_pm_enable_wake_irq(struct device *dev);
-extern void dev_pm_disable_wake_irq(struct device *dev);
 
 #else	/* !CONFIG_PM */
 
@@ -34,13 +32,5 @@ static inline void dev_pm_clear_wake_irq(struct device *dev)
 {
 }
 
-static inline void dev_pm_enable_wake_irq(struct device *dev)
-{
-}
-
-static inline void dev_pm_disable_wake_irq(struct device *dev)
-{
-}
-
 #endif	/* CONFIG_PM */
 #endif	/* _LINUX_PM_WAKEIRQ_H */
-- 
2.41.0

Re: [PATCH 2/3] PM / wakeirq: drop unused enable helpers

[Tony Lindgren]

* Johan Hovold <johan+linaro@kernel.org> [230713 15:01]:
> Drop the wake-irq enable and disable helpers which have not been used
> since commit bed570307ed7 ("PM / wakeirq: Fix dedicated wakeirq for
> drivers not using autosuspend").
> 
> Note that these functions are essentially just leftovers from the first
> iteration of the wake-irq implementation where device drivers were
> supposed to call these functions themselves instead of PM core (as
> is also indicated by the bogus kernel doc comments).

Agreed no need for these any longer:

Reviewed-by: Tony Lindgren <tony@atomide.com>

[PATCH 3/3] serial: qcom-geni: drop bogus runtime pm state update

[Johan Hovold]

The runtime PM state should not be changed by drivers that do not
implement runtime PM even if it happens to work around a bug in PM core.

With the wake irq arming now fixed, drop the bogus runtime PM state
update which left the device in active state (and could potentially
prevent a parent device from suspending).

Fixes: f3974413cf02 ("tty: serial: qcom_geni_serial: Wakeup IRQ cleanup")
Cc: stable@vger.kernel.org      # 5.6
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 88ed5bbe25a8..b825b05e6137 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1681,13 +1681,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/*
-	 * Set pm_runtime status as ACTIVE so that wakeup_irq gets
-	 * enabled/disabled from dev_pm_arm_wake_irq during system
-	 * suspend/resume respectively.
-	 */
-	pm_runtime_set_active(&pdev->dev);
-
 	if (port->wakeup_irq > 0) {
 		device_init_wakeup(&pdev->dev, true);
 		ret = dev_pm_set_dedicated_wake_irq(&pdev->dev,
-- 
2.41.0

Re: [PATCH 3/3] serial: qcom-geni: drop bogus runtime pm state update

[Tony Lindgren]

* Johan Hovold <johan+linaro@kernel.org> [230713 15:01]:
> The runtime PM state should not be changed by drivers that do not
> implement runtime PM even if it happens to work around a bug in PM core.
> 
> With the wake irq arming now fixed, drop the bogus runtime PM state
> update which left the device in active state (and could potentially
> prevent a parent device from suspending).

Reviewed-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 0/3] PM / wakeirq: fix wake irq arming

[Rafael J. Wysocki]

On Thu, Jul 13, 2023 at 5:01 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> When reviewing the Qualcomm serial-driver suspend implementation I
> noticed the odd runtime PM state update which had snuck in. Turns out it
> was added to work around a bug in PM core which prevented drivers not
> implementing runtime PM from using dedicated wake irqs.
>
> This series fixes the wake irq arming and drops the unused wake irq
> enable helpers before dropping the bogus runtime PM state update in the
> Qualcomm driver.
>
> I suggest that Rafael takes all of these through his tree.
>
> Johan
>
>
> Johan Hovold (3):
>   PM / wakeirq: fix wake irq arming
>   PM / wakeirq: drop unused enable helpers
>   serial: qcom-geni: drop bogus runtime pm state update

All applied and I'm inclined to push them as fixed for 6.5-rc, thanks!

Re: [PATCH 0/3] PM / wakeirq: fix wake irq arming

[Greg Kroah-Hartman]

On Thu, Jul 13, 2023 at 04:57:38PM +0200, Johan Hovold wrote:
> When reviewing the Qualcomm serial-driver suspend implementation I
> noticed the odd runtime PM state update which had snuck in. Turns out it
> was added to work around a bug in PM core which prevented drivers not
> implementing runtime PM from using dedicated wake irqs.
> 
> This series fixes the wake irq arming and drops the unused wake irq
> enable helpers before dropping the bogus runtime PM state update in the
> Qualcomm driver.
> 
> I suggest that Rafael takes all of these through his tree.

I agree:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>