[PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq

[PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq

[Muchun Song]

Currently, the PM core disables runtime PM for all devices right after
executing subsystem/driver .suspend_late() callbacks for them and
re-enables it right before executing subsystem/driver .resume_early()
callbacks for them. This may lead to problems when there are two devices
such that the irq handler thread function executed for one of them
depends on runtime PM working for the other. E.g. There are two devices,
one is i2c slave device depends on another device which can be the i2c
adapter device. The slave device can generate system wakeup signals and
is enabled to wake up the system(via call enable_irq_wake()). So, the irq
of slave device is enabled. If a wakeup signal generate after executing
subsystem/driver .suspend_late() callbacks. Then, the irq handler thread
function will be called(The irq is requested via request_threaded_irq())
and the slave device reads data via i2c adapter device(via i2c_transfer()).
In that case, it may be failed to read data because of the runtime PM
disabled.

It is also analogously for resume. If a wakeup signal generate when the
system is in the sleep state. The irq handler thread function may be
called before executing subsystem/driver .resume_early(). In that case,
it also may be failed to read data because of the runtime PM disabled.

To make those issues go away, make the PM core disable runtime PM for
devices right before executing subsystem/driver .suspend_noirq() callbacks
for them and enable runtime PM for them right after executing subsystem/
driver .resume_noirq() callbacks for them.

Signed-off-by: Muchun Song <smuchun@gmail.com>
---

Change in v2:
       Update subject from:
           "PM: Move disabling/enabling runtime PM to noirq suspend/early resume"
       to:
           "PM: Move disabling/enabling runtime PM to suspend/resume noirq"

 Documentation/power/runtime_pm.txt | 4 ++--
 drivers/base/power/main.c          | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index 937e33c46211..8cca4df3adc4 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -691,11 +691,11 @@ out the following operations:
     pm_runtime_barrier() is called for every device right before executing the
     subsystem-level .suspend() callback for it.  In addition to that the PM core
     calls  __pm_runtime_disable() with 'false' as the second argument for every
-    device right before executing the subsystem-level .suspend_late() callback
+    device right before executing the subsystem-level .suspend_noirq() callback
     for it.
 
   * During system resume pm_runtime_enable() and pm_runtime_put() are called for
-    every device right after executing the subsystem-level .resume_early()
+    every device right after executing the subsystem-level .resume_noirq()
     callback and right after executing the subsystem-level .complete() callback
     for it, respectively.
 
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index dcfc0a36c8f7..ad0282d637ae 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -693,6 +693,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 	}
 
 Out:
+	pm_runtime_enable(dev);
 	complete_all(&dev->power.completion);
 	TRACE_RESUME(error);
 	return error;
@@ -860,7 +861,6 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
  Out:
 	TRACE_RESUME(error);
 
-	pm_runtime_enable(dev);
 	complete_all(&dev->power.completion);
 	return error;
 }
@@ -1299,6 +1299,8 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
+	__pm_runtime_disable(dev, false);
+
 	dpm_wait_for_subordinate(dev, async);
 
 	if (async_error)
@@ -1508,8 +1510,6 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 	TRACE_DEVICE(dev);
 	TRACE_SUSPEND(0);
 
-	__pm_runtime_disable(dev, false);
-
 	dpm_wait_for_subordinate(dev, async);
 
 	if (async_error)
-- 
2.17.1

Re: [PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq

[Rafael J. Wysocki]

On Tue, Jul 2, 2019 at 6:37 PM Muchun Song <smuchun@gmail.com> wrote:
>
> Currently, the PM core disables runtime PM for all devices right after
> executing subsystem/driver .suspend_late() callbacks for them and
> re-enables it right before executing subsystem/driver .resume_early()
> callbacks for them. This may lead to problems when there are two devices
> such that the irq handler thread function executed for one of them
> depends on runtime PM working for the other. E.g. There are two devices,
> one is i2c slave device depends on another device which can be the i2c
> adapter device. The slave device can generate system wakeup signals and
> is enabled to wake up the system(via call enable_irq_wake()). So, the irq
> of slave device is enabled. If a wakeup signal generate after executing
> subsystem/driver .suspend_late() callbacks. Then, the irq handler thread
> function will be called(The irq is requested via request_threaded_irq())
> and the slave device reads data via i2c adapter device(via i2c_transfer()).
> In that case, it may be failed to read data because of the runtime PM
> disabled.
>
> It is also analogously for resume. If a wakeup signal generate when the
> system is in the sleep state. The irq handler thread function may be
> called before executing subsystem/driver .resume_early(). In that case,
> it also may be failed to read data because of the runtime PM disabled.
>
> To make those issues go away, make the PM core disable runtime PM for
> devices right before executing subsystem/driver .suspend_noirq() callbacks
> for them and enable runtime PM for them right after executing subsystem/
> driver .resume_noirq() callbacks for them.
>
> Signed-off-by: Muchun Song <smuchun@gmail.com>

This has been discussed for a number of times, documented and no, I'm
not going to apply this patch.

PM-runtime cannot be relied on during the "noirq" stages of suspend
and resume, which is why it is disabled by the core in the "late" and
"early" stages, respectively.

> ---
>
> Change in v2:
>        Update subject from:
>            "PM: Move disabling/enabling runtime PM to noirq suspend/early resume"
>        to:
>            "PM: Move disabling/enabling runtime PM to suspend/resume noirq"
>
>  Documentation/power/runtime_pm.txt | 4 ++--
>  drivers/base/power/main.c          | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
> index 937e33c46211..8cca4df3adc4 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -691,11 +691,11 @@ out the following operations:
>      pm_runtime_barrier() is called for every device right before executing the
>      subsystem-level .suspend() callback for it.  In addition to that the PM core
>      calls  __pm_runtime_disable() with 'false' as the second argument for every
> -    device right before executing the subsystem-level .suspend_late() callback
> +    device right before executing the subsystem-level .suspend_noirq() callback
>      for it.
>
>    * During system resume pm_runtime_enable() and pm_runtime_put() are called for
> -    every device right after executing the subsystem-level .resume_early()
> +    every device right after executing the subsystem-level .resume_noirq()
>      callback and right after executing the subsystem-level .complete() callback
>      for it, respectively.
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index dcfc0a36c8f7..ad0282d637ae 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -693,6 +693,7 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
>         }
>
>  Out:
> +       pm_runtime_enable(dev);
>         complete_all(&dev->power.completion);
>         TRACE_RESUME(error);
>         return error;
> @@ -860,7 +861,6 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
>   Out:
>         TRACE_RESUME(error);
>
> -       pm_runtime_enable(dev);
>         complete_all(&dev->power.completion);
>         return error;
>  }
> @@ -1299,6 +1299,8 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
>         TRACE_DEVICE(dev);
>         TRACE_SUSPEND(0);
>
> +       __pm_runtime_disable(dev, false);
> +
>         dpm_wait_for_subordinate(dev, async);
>
>         if (async_error)
> @@ -1508,8 +1510,6 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
>         TRACE_DEVICE(dev);
>         TRACE_SUSPEND(0);
>
> -       __pm_runtime_disable(dev, false);
> -
>         dpm_wait_for_subordinate(dev, async);
>
>         if (async_error)
> --
> 2.17.1
>

Re: [PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq

[Muchun Song]

Rafael J. Wysocki <rafael@kernel.org> 于2019年7月3日周三 上午1:54写道：
>
> On Tue, Jul 2, 2019 at 6:37 PM Muchun Song <smuchun@gmail.com> wrote:
> >
> > Currently, the PM core disables runtime PM for all devices right after
> > executing subsystem/driver .suspend_late() callbacks for them and
> > re-enables it right before executing subsystem/driver .resume_early()
> > callbacks for them. This may lead to problems when there are two devices
> > such that the irq handler thread function executed for one of them
> > depends on runtime PM working for the other. E.g. There are two devices,
> > one is i2c slave device depends on another device which can be the i2c
> > adapter device. The slave device can generate system wakeup signals and
> > is enabled to wake up the system(via call enable_irq_wake()). So, the irq
> > of slave device is enabled. If a wakeup signal generate after executing
> > subsystem/driver .suspend_late() callbacks. Then, the irq handler thread
> > function will be called(The irq is requested via request_threaded_irq())
> > and the slave device reads data via i2c adapter device(via i2c_transfer()).
> > In that case, it may be failed to read data because of the runtime PM
> > disabled.
> >
> > It is also analogously for resume. If a wakeup signal generate when the
> > system is in the sleep state. The irq handler thread function may be
> > called before executing subsystem/driver .resume_early(). In that case,
> > it also may be failed to read data because of the runtime PM disabled.
> >
>
> This has been discussed for a number of times, documented and no, I'm
> not going to apply this patch.

Thanks for your reply. I want to know why we can't do that, so where
can I find the discussion?

> PM-runtime cannot be relied on during the "noirq" stages of suspend
> and resume, which is why it is disabled by the core in the "late" and
> "early" stages, respectively.
>

What better solution do we have for the example I am talking about
which is described in the commit message? Thanks.

Yours,
Muchun

Re: [PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq

[Rafael J. Wysocki]

On Wed, Jul 3, 2019 at 2:56 AM Muchun Song <smuchun@gmail.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 于2019年7月3日周三 上午1:54写道：
> >
> > On Tue, Jul 2, 2019 at 6:37 PM Muchun Song <smuchun@gmail.com> wrote:
> > >
> > > Currently, the PM core disables runtime PM for all devices right after
> > > executing subsystem/driver .suspend_late() callbacks for them and
> > > re-enables it right before executing subsystem/driver .resume_early()
> > > callbacks for them. This may lead to problems when there are two devices
> > > such that the irq handler thread function executed for one of them
> > > depends on runtime PM working for the other. E.g. There are two devices,
> > > one is i2c slave device depends on another device which can be the i2c
> > > adapter device. The slave device can generate system wakeup signals and
> > > is enabled to wake up the system(via call enable_irq_wake()). So, the irq
> > > of slave device is enabled. If a wakeup signal generate after executing
> > > subsystem/driver .suspend_late() callbacks. Then, the irq handler thread
> > > function will be called(The irq is requested via request_threaded_irq())
> > > and the slave device reads data via i2c adapter device(via i2c_transfer()).
> > > In that case, it may be failed to read data because of the runtime PM
> > > disabled.
> > >
> > > It is also analogously for resume. If a wakeup signal generate when the
> > > system is in the sleep state. The irq handler thread function may be
> > > called before executing subsystem/driver .resume_early(). In that case,
> > > it also may be failed to read data because of the runtime PM disabled.
> > >
> >
> > This has been discussed for a number of times, documented and no, I'm
> > not going to apply this patch.
>
> Thanks for your reply. I want to know why we can't do that, so where
> can I find the discussion?

The discussions rather.

You need to search email archives, especially for the reason why the
"late" and "early" callbacks have been added.  There was a conference
presentation about that even.

The bottom line is that the change you are proposing cannot be made.

> > PM-runtime cannot be relied on during the "noirq" stages of suspend
> > and resume, which is why it is disabled by the core in the "late" and
> > "early" stages, respectively.
> >
>
> What better solution do we have for the example I am talking about
> which is described in the commit message?

It's not "what better solution is there", because what you are
proposing would simply not work.

Basically, if there are devices that may need the given one in the
"noirq" resume phase, this device needs to be resumed upfront.  It may
be runtime-suspended again in the "early" resume phase.