PM: runtime: Extend support for wakeirq for force_suspend|resume

[PATCH] PM: runtime: Extend support for wakeirq for force_suspend|resume

Posted by Ulf Hansson 3 years, 10 months ago

A driver that makes use of pm_runtime_force_suspend|resume() to support
system suspend/resume, currently needs to manage the wakeirq support
itself. To avoid the boilerplate code in the driver's system suspend/resume
callbacks in particular, let's extend pm_runtime_force_suspend|resume() to
deal with the wakeirq.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Note that, the reason behind this patch came up while discussing an MMC patch
for a Mediatek MMC host driver [1].

Kind regards
Ulf Hansson

[1]
https://lkml.org/lkml/2022/6/8/813

---
 drivers/base/power/runtime.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 676dc72d912d..445a724cbded 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1876,10 +1876,13 @@ int pm_runtime_force_suspend(struct device *dev)
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
 
+	dev_pm_enable_wake_irq_check(dev, true);
 	ret = callback ? callback(dev) : 0;
 	if (ret)
 		goto err;
 
+	dev_pm_enable_wake_irq_complete(dev);
+
 	/*
 	 * If the device can stay in suspend after the system-wide transition
 	 * to the working state that will follow, drop the children counter of
@@ -1896,6 +1899,7 @@ int pm_runtime_force_suspend(struct device *dev)
 	return 0;
 
 err:
+	dev_pm_disable_wake_irq_check(dev, true);
 	pm_runtime_enable(dev);
 	return ret;
 }
@@ -1929,9 +1933,11 @@ int pm_runtime_force_resume(struct device *dev)
 
 	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 
+	dev_pm_disable_wake_irq_check(dev, false);
 	ret = callback ? callback(dev) : 0;
 	if (ret) {
 		pm_runtime_set_suspended(dev);
+		dev_pm_enable_wake_irq_check(dev, false);
 		goto out;
 	}
 
-- 
2.25.1

Re: [PATCH] PM: runtime: Extend support for wakeirq for force_suspend|resume

Posted by Tony Lindgren 3 years, 10 months ago

* Ulf Hansson <ulf.hansson@linaro.org> [220608 15:26]:
> A driver that makes use of pm_runtime_force_suspend|resume() to support
> system suspend/resume, currently needs to manage the wakeirq support
> itself. To avoid the boilerplate code in the driver's system suspend/resume
> callbacks in particular, let's extend pm_runtime_force_suspend|resume() to
> deal with the wakeirq.

Looks good to me:

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

Re: [PATCH] PM: runtime: Extend support for wakeirq for force_suspend|resume

Posted by Rafael J. Wysocki 3 years, 9 months ago

On Tue, Jun 14, 2022 at 1:16 PM Tony Lindgren <tony@atomide.com> wrote:
>
> * Ulf Hansson <ulf.hansson@linaro.org> [220608 15:26]:
> > A driver that makes use of pm_runtime_force_suspend|resume() to support
> > system suspend/resume, currently needs to manage the wakeirq support
> > itself. To avoid the boilerplate code in the driver's system suspend/resume
> > callbacks in particular, let's extend pm_runtime_force_suspend|resume() to
> > deal with the wakeirq.
>
> Looks good to me:
>
> Reviewed-by: Tony Lindgren <tony@atomide.com>

Applied (as 5.20 material), but there is still a quite fundamental
problem with pm_runtime_force_suspend|resume() which is using
RPM_GET_CALLBACK() in these functions, because that macro returns a
middle-layer PM-runtime callback, so any bus type or PM domain
implementing meaningful generic power management ends up calling its
own PM-runtime callback from its system-wide PM callback which is
super-confusing at best.

Another problem with them is that they are generally not suitable for
devices that can wake up the system from sleep.

Re: [PATCH] PM: runtime: Extend support for wakeirq for force_suspend|resume

Posted by Chunfeng Yun 3 years, 10 months ago

On Wed, 2022-06-08 at 17:31 +0200, Ulf Hansson wrote:
> A driver that makes use of pm_runtime_force_suspend|resume() to
> support
> system suspend/resume, currently needs to manage the wakeirq support
> itself. To avoid the boilerplate code in the driver's system
> suspend/resume
> callbacks in particular, let's extend
> pm_runtime_force_suspend|resume() to
> deal with the wakeirq.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Note that, the reason behind this patch came up while discussing an
> MMC patch
> for a Mediatek MMC host driver [1].
> 
> Kind regards
> Ulf Hansson
> 
> [1]
> https://lkml.org/lkml/2022/6/8/813
> 
> ---
>  drivers/base/power/runtime.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/base/power/runtime.c
> b/drivers/base/power/runtime.c
> index 676dc72d912d..445a724cbded 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1876,10 +1876,13 @@ int pm_runtime_force_suspend(struct device
> *dev)
>  
>  	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>  
> +	dev_pm_enable_wake_irq_check(dev, true);
>  	ret = callback ? callback(dev) : 0;
>  	if (ret)
>  		goto err;
>  
> +	dev_pm_enable_wake_irq_complete(dev);
> +
>  	/*
>  	 * If the device can stay in suspend after the system-wide
> transition
>  	 * to the working state that will follow, drop the children
> counter of
> @@ -1896,6 +1899,7 @@ int pm_runtime_force_suspend(struct device
> *dev)
>  	return 0;
>  
>  err:
> +	dev_pm_disable_wake_irq_check(dev, true);
>  	pm_runtime_enable(dev);
>  	return ret;
>  }
> @@ -1929,9 +1933,11 @@ int pm_runtime_force_resume(struct device
> *dev)
>  
>  	callback = RPM_GET_CALLBACK(dev, runtime_resume);
>  
> +	dev_pm_disable_wake_irq_check(dev, false);
>  	ret = callback ? callback(dev) : 0;
>  	if (ret) {
>  		pm_runtime_set_suspended(dev);
> +		dev_pm_enable_wake_irq_check(dev, false);
>  		goto out;
>  	}
>  
Reviewed-by Chunfeng Yun <chunfeng.yun@mediatek.com>