linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH RT] PM: runtime: avoid retry loops on RT
@ 2021-10-05 15:54 John Keeping
  2021-10-05 16:38 ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: John Keeping @ 2021-10-05 15:54 UTC (permalink / raw)
  To: linux-rt-users
  Cc: John Keeping, Rafael J. Wysocki, Pavel Machek, Len Brown,
	Greg Kroah-Hartman, linux-pm, linux-kernel

With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
is no reason to have a special case using the former.  Furthermore,
spin_unlock() enables preemption meaning that a task in RESUMING or
SUSPENDING state may be preempted by a higher priority task running
pm_runtime_get_sync() leading to a livelock.

Use the non-irq_safe path for all waiting so that the waiting task will
block.

Note that this changes only the waiting behaviour of irq_safe, other
uses are left unchanged so that the parent device always remains active
in the same way as !RT.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/base/power/runtime.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 96972d5f6ef3..5e0d349fab4e 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -347,8 +347,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 {
 	int retval = 0, idx;
 	bool use_links = dev->power.links_count > 0;
+	bool irq_safe = dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT);
 
-	if (dev->power.irq_safe) {
+	if (irq_safe) {
 		spin_unlock(&dev->power.lock);
 	} else {
 		spin_unlock_irq(&dev->power.lock);
@@ -376,7 +377,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 	if (cb)
 		retval = cb(dev);
 
-	if (dev->power.irq_safe) {
+	if (irq_safe) {
 		spin_lock(&dev->power.lock);
 	} else {
 		/*
@@ -596,7 +597,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 			goto out;
 		}
 
-		if (dev->power.irq_safe) {
+		if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
 			spin_unlock(&dev->power.lock);
 
 			cpu_relax();
@@ -777,7 +778,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 			goto out;
 		}
 
-		if (dev->power.irq_safe) {
+		if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
 			spin_unlock(&dev->power.lock);
 
 			cpu_relax();
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-05 15:54 [RFC PATCH RT] PM: runtime: avoid retry loops on RT John Keeping
@ 2021-10-05 16:38 ` Rafael J. Wysocki
  2021-10-05 17:17   ` John Keeping
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-10-05 16:38 UTC (permalink / raw)
  To: John Keeping
  Cc: linux-rt-users, Rafael J. Wysocki, Pavel Machek, Len Brown,
	Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List

On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@metanate.com> wrote:
>
> With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> is no reason to have a special case using the former.  Furthermore,
> spin_unlock() enables preemption meaning that a task in RESUMING or
> SUSPENDING state may be preempted by a higher priority task running
> pm_runtime_get_sync() leading to a livelock.
>
> Use the non-irq_safe path for all waiting so that the waiting task will
> block.
>
> Note that this changes only the waiting behaviour of irq_safe, other
> uses are left unchanged so that the parent device always remains active
> in the same way as !RT.
>
> Signed-off-by: John Keeping <john@metanate.com>

So basically, the idea is that the irq_safe flag should have no effect
when CONFIG_PREEMPT_RT is set, right?

Wouldn't it be cleaner to make it not present at all in that case?

> ---
>  drivers/base/power/runtime.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 96972d5f6ef3..5e0d349fab4e 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -347,8 +347,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>  {
>         int retval = 0, idx;
>         bool use_links = dev->power.links_count > 0;
> +       bool irq_safe = dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT);
>
> -       if (dev->power.irq_safe) {
> +       if (irq_safe) {
>                 spin_unlock(&dev->power.lock);
>         } else {
>                 spin_unlock_irq(&dev->power.lock);
> @@ -376,7 +377,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>         if (cb)
>                 retval = cb(dev);
>
> -       if (dev->power.irq_safe) {
> +       if (irq_safe) {
>                 spin_lock(&dev->power.lock);
>         } else {
>                 /*
> @@ -596,7 +597,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>                         goto out;
>                 }
>
> -               if (dev->power.irq_safe) {
> +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
>                         spin_unlock(&dev->power.lock);
>
>                         cpu_relax();
> @@ -777,7 +778,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
>                         goto out;
>                 }
>
> -               if (dev->power.irq_safe) {
> +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
>                         spin_unlock(&dev->power.lock);
>
>                         cpu_relax();
> --
> 2.33.0
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-05 16:38 ` Rafael J. Wysocki
@ 2021-10-05 17:17   ` John Keeping
  2021-10-06 17:05     ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: John Keeping @ 2021-10-05 17:17 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-rt-users, Pavel Machek, Len Brown, Greg Kroah-Hartman,
	Linux PM, Linux Kernel Mailing List

On Tue, 5 Oct 2021 18:38:27 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@metanate.com> wrote:
> >
> > With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> > is no reason to have a special case using the former.  Furthermore,
> > spin_unlock() enables preemption meaning that a task in RESUMING or
> > SUSPENDING state may be preempted by a higher priority task running
> > pm_runtime_get_sync() leading to a livelock.
> >
> > Use the non-irq_safe path for all waiting so that the waiting task will
> > block.
> >
> > Note that this changes only the waiting behaviour of irq_safe, other
> > uses are left unchanged so that the parent device always remains active
> > in the same way as !RT.
> >
> > Signed-off-by: John Keeping <john@metanate.com>  
> 
> So basically, the idea is that the irq_safe flag should have no effect
> when CONFIG_PREEMPT_RT is set, right?
> 
> Wouldn't it be cleaner to make it not present at all in that case?

Yes, just replacing pm_runtime_irq_safe() with an empty function would
also fix it, but I'm not sure if that will have unexpected effects from
the parent device suspending/resuming, especially in terms of latency
for handling interrupts.

> > ---
> >  drivers/base/power/runtime.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 96972d5f6ef3..5e0d349fab4e 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -347,8 +347,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> >  {
> >         int retval = 0, idx;
> >         bool use_links = dev->power.links_count > 0;
> > +       bool irq_safe = dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT);
> >
> > -       if (dev->power.irq_safe) {
> > +       if (irq_safe) {
> >                 spin_unlock(&dev->power.lock);
> >         } else {
> >                 spin_unlock_irq(&dev->power.lock);
> > @@ -376,7 +377,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> >         if (cb)
> >                 retval = cb(dev);
> >
> > -       if (dev->power.irq_safe) {
> > +       if (irq_safe) {
> >                 spin_lock(&dev->power.lock);
> >         } else {
> >                 /*
> > @@ -596,7 +597,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >                         goto out;
> >                 }
> >
> > -               if (dev->power.irq_safe) {
> > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >                         spin_unlock(&dev->power.lock);
> >
> >                         cpu_relax();
> > @@ -777,7 +778,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
> >                         goto out;
> >                 }
> >
> > -               if (dev->power.irq_safe) {
> > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >                         spin_unlock(&dev->power.lock);
> >
> >                         cpu_relax();
> > --
> > 2.33.0
> >  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-05 17:17   ` John Keeping
@ 2021-10-06 17:05     ` Rafael J. Wysocki
  2021-10-06 18:18       ` John Keeping
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-10-06 17:05 UTC (permalink / raw)
  To: John Keeping
  Cc: Rafael J. Wysocki, linux-rt-users, Pavel Machek, Len Brown,
	Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List

On Tue, Oct 5, 2021 at 7:17 PM John Keeping <john@metanate.com> wrote:
>
> On Tue, 5 Oct 2021 18:38:27 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@metanate.com> wrote:
> > >
> > > With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> > > is no reason to have a special case using the former.  Furthermore,
> > > spin_unlock() enables preemption meaning that a task in RESUMING or
> > > SUSPENDING state may be preempted by a higher priority task running
> > > pm_runtime_get_sync() leading to a livelock.
> > >
> > > Use the non-irq_safe path for all waiting so that the waiting task will
> > > block.
> > >
> > > Note that this changes only the waiting behaviour of irq_safe, other
> > > uses are left unchanged so that the parent device always remains active
> > > in the same way as !RT.
> > >
> > > Signed-off-by: John Keeping <john@metanate.com>
> >
> > So basically, the idea is that the irq_safe flag should have no effect
> > when CONFIG_PREEMPT_RT is set, right?
> >
> > Wouldn't it be cleaner to make it not present at all in that case?
>
> Yes, just replacing pm_runtime_irq_safe() with an empty function would
> also fix it, but I'm not sure if that will have unexpected effects from
> the parent device suspending/resuming, especially in terms of latency
> for handling interrupts.

Well, the code as is doesn't work with CONFIG_PREEMPT_RT set anyway in general.

Also this is not just pm_runtime_irq_safe(), but every access to this
flag (and there's more  of them than just the ones changed below).

What about putting the flag under #ifdef CONFIG_PREEMPT_RT and
providing read/write accessor helpers for it that will be empty in
RT-enabled kernels?

> > > ---
> > >  drivers/base/power/runtime.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index 96972d5f6ef3..5e0d349fab4e 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -347,8 +347,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> > >  {
> > >         int retval = 0, idx;
> > >         bool use_links = dev->power.links_count > 0;
> > > +       bool irq_safe = dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT);
> > >
> > > -       if (dev->power.irq_safe) {
> > > +       if (irq_safe) {
> > >                 spin_unlock(&dev->power.lock);
> > >         } else {
> > >                 spin_unlock_irq(&dev->power.lock);
> > > @@ -376,7 +377,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> > >         if (cb)
> > >                 retval = cb(dev);
> > >
> > > -       if (dev->power.irq_safe) {
> > > +       if (irq_safe) {
> > >                 spin_lock(&dev->power.lock);
> > >         } else {
> > >                 /*
> > > @@ -596,7 +597,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> > >                         goto out;
> > >                 }
> > >
> > > -               if (dev->power.irq_safe) {
> > > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > >                         spin_unlock(&dev->power.lock);
> > >
> > >                         cpu_relax();
> > > @@ -777,7 +778,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > >                         goto out;
> > >                 }
> > >
> > > -               if (dev->power.irq_safe) {
> > > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > >                         spin_unlock(&dev->power.lock);
> > >
> > >                         cpu_relax();
> > > --
> > > 2.33.0
> > >
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-06 17:05     ` Rafael J. Wysocki
@ 2021-10-06 18:18       ` John Keeping
  2021-10-21 10:37         ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: John Keeping @ 2021-10-06 18:18 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-rt-users, Pavel Machek, Len Brown, Greg Kroah-Hartman,
	Linux PM, Linux Kernel Mailing List

On Wed, 6 Oct 2021 19:05:50 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Tue, Oct 5, 2021 at 7:17 PM John Keeping <john@metanate.com> wrote:
> >
> > On Tue, 5 Oct 2021 18:38:27 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >  
> > > On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@metanate.com> wrote:  
> > > >
> > > > With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> > > > is no reason to have a special case using the former.  Furthermore,
> > > > spin_unlock() enables preemption meaning that a task in RESUMING or
> > > > SUSPENDING state may be preempted by a higher priority task running
> > > > pm_runtime_get_sync() leading to a livelock.
> > > >
> > > > Use the non-irq_safe path for all waiting so that the waiting task will
> > > > block.
> > > >
> > > > Note that this changes only the waiting behaviour of irq_safe, other
> > > > uses are left unchanged so that the parent device always remains active
> > > > in the same way as !RT.
> > > >
> > > > Signed-off-by: John Keeping <john@metanate.com>  
> > >
> > > So basically, the idea is that the irq_safe flag should have no effect
> > > when CONFIG_PREEMPT_RT is set, right?
> > >
> > > Wouldn't it be cleaner to make it not present at all in that case?  
> >
> > Yes, just replacing pm_runtime_irq_safe() with an empty function would
> > also fix it, but I'm not sure if that will have unexpected effects from
> > the parent device suspending/resuming, especially in terms of latency
> > for handling interrupts.  
> 
> Well, the code as is doesn't work with CONFIG_PREEMPT_RT set anyway in general.
> 
> Also this is not just pm_runtime_irq_safe(), but every access to this
> flag (and there's more  of them than just the ones changed below).
> 
> What about putting the flag under #ifdef CONFIG_PREEMPT_RT and
> providing read/write accessor helpers for it that will be empty in
> RT-enabled kernels?

That's the other approach I considered, but there are really two things
that irq_safe means here:

1) Call the suspend/resume hooks with interrupts disabled

2) Keep the parent device running and make other changes that allow (1)
   on non-RT systems (for example in amba_pm_runtime_suspend() leave the
   clock prepared when irq_safe is set, but unprepare it otherwise)

In the approach of leaving the flag unset on PREEMPT_RT we solve the
primary problem which is that (1) is irrelevant on RT, but that would
also affect (2) and I'm not sure whether that's desirable or not.

It's quite possible the answer is that the other changes don't matter
and we should take the simpler approach of just removing irq_safe under
CONFIG_PREEMPT_RT.  I'm becoming convinced that this is the right
answer, but I'm not confident that I fully understand the wider
ramifications.

> > > > ---
> > > >  drivers/base/power/runtime.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > index 96972d5f6ef3..5e0d349fab4e 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -347,8 +347,9 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> > > >  {
> > > >         int retval = 0, idx;
> > > >         bool use_links = dev->power.links_count > 0;
> > > > +       bool irq_safe = dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT);
> > > >
> > > > -       if (dev->power.irq_safe) {
> > > > +       if (irq_safe) {
> > > >                 spin_unlock(&dev->power.lock);
> > > >         } else {
> > > >                 spin_unlock_irq(&dev->power.lock);
> > > > @@ -376,7 +377,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> > > >         if (cb)
> > > >                 retval = cb(dev);
> > > >
> > > > -       if (dev->power.irq_safe) {
> > > > +       if (irq_safe) {
> > > >                 spin_lock(&dev->power.lock);
> > > >         } else {
> > > >                 /*
> > > > @@ -596,7 +597,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> > > >                         goto out;
> > > >                 }
> > > >
> > > > -               if (dev->power.irq_safe) {
> > > > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > > >                         spin_unlock(&dev->power.lock);
> > > >
> > > >                         cpu_relax();
> > > > @@ -777,7 +778,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > >                         goto out;
> > > >                 }
> > > >
> > > > -               if (dev->power.irq_safe) {
> > > > +               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > > >                         spin_unlock(&dev->power.lock);
> > > >
> > > >                         cpu_relax();
> > > > --
> > > > 2.33.0
> > > >  
> >  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-06 18:18       ` John Keeping
@ 2021-10-21 10:37         ` Rafael J. Wysocki
  2021-10-25 10:30           ` John Keeping
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-10-21 10:37 UTC (permalink / raw)
  To: John Keeping
  Cc: Rafael J. Wysocki, linux-rt-users, Pavel Machek, Len Brown,
	Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List

On Wed, Oct 6, 2021 at 8:18 PM John Keeping <john@metanate.com> wrote:
>
> On Wed, 6 Oct 2021 19:05:50 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Tue, Oct 5, 2021 at 7:17 PM John Keeping <john@metanate.com> wrote:
> > >
> > > On Tue, 5 Oct 2021 18:38:27 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > > On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@metanate.com> wrote:
> > > > >
> > > > > With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> > > > > is no reason to have a special case using the former.  Furthermore,
> > > > > spin_unlock() enables preemption meaning that a task in RESUMING or
> > > > > SUSPENDING state may be preempted by a higher priority task running
> > > > > pm_runtime_get_sync() leading to a livelock.
> > > > >
> > > > > Use the non-irq_safe path for all waiting so that the waiting task will
> > > > > block.
> > > > >
> > > > > Note that this changes only the waiting behaviour of irq_safe, other
> > > > > uses are left unchanged so that the parent device always remains active
> > > > > in the same way as !RT.
> > > > >
> > > > > Signed-off-by: John Keeping <john@metanate.com>
> > > >
> > > > So basically, the idea is that the irq_safe flag should have no effect
> > > > when CONFIG_PREEMPT_RT is set, right?
> > > >
> > > > Wouldn't it be cleaner to make it not present at all in that case?
> > >
> > > Yes, just replacing pm_runtime_irq_safe() with an empty function would
> > > also fix it, but I'm not sure if that will have unexpected effects from
> > > the parent device suspending/resuming, especially in terms of latency
> > > for handling interrupts.
> >
> > Well, the code as is doesn't work with CONFIG_PREEMPT_RT set anyway in general.
> >
> > Also this is not just pm_runtime_irq_safe(), but every access to this
> > flag (and there's more  of them than just the ones changed below).
> >
> > What about putting the flag under #ifdef CONFIG_PREEMPT_RT and
> > providing read/write accessor helpers for it that will be empty in
> > RT-enabled kernels?
>
> That's the other approach I considered, but there are really two things
> that irq_safe means here:
>
> 1) Call the suspend/resume hooks with interrupts disabled
>
> 2) Keep the parent device running and make other changes that allow (1)
>    on non-RT systems (for example in amba_pm_runtime_suspend() leave the
>    clock prepared when irq_safe is set, but unprepare it otherwise)
>
> In the approach of leaving the flag unset on PREEMPT_RT we solve the
> primary problem which is that (1) is irrelevant on RT, but that would
> also affect (2) and I'm not sure whether that's desirable or not.
>
> It's quite possible the answer is that the other changes don't matter
> and we should take the simpler approach of just removing irq_safe under
> CONFIG_PREEMPT_RT.  I'm becoming convinced that this is the right
> answer, but I'm not confident that I fully understand the wider
> ramifications.

The initial motivation for adding irq_safe was to allow interrupt
handlers of some devices to use PM-runtime, but in RT kernels that's
possible regardless IIUC, so I don't see a reason for having irq_safe
at all in that case.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT
  2021-10-21 10:37         ` Rafael J. Wysocki
@ 2021-10-25 10:30           ` John Keeping
  0 siblings, 0 replies; 7+ messages in thread
From: John Keeping @ 2021-10-25 10:30 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-rt-users, Pavel Machek, Len Brown, Greg Kroah-Hartman,
	Linux PM, Linux Kernel Mailing List, Thomas Gleixner,
	Ingo Molnar, Peter Zijlstra

On Thu, 21 Oct 2021 12:37:05 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> The initial motivation for adding irq_safe was to allow interrupt
> handlers of some devices to use PM-runtime, but in RT kernels that's
> possible regardless IIUC, so I don't see a reason for having irq_safe
> at all in that case.

I coded up the "no irq_safe" version but lockdep complains loudly about
it:

	BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1111
	in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 237, name: pm-runtime-prio
	preempt_count: 0, expected: 0
	RCU nest depth: 2, expected: 0
	INFO: lockdep is turned off.
	CPU: 3 PID: 237 Comm: pm-runtime-prio Tainted: G        W         5.15.0-rc6-rt13 #1
	Hardware name: Rockchip (Device Tree)
	[<c010f9d0>] (unwind_backtrace) from [<c010afc8>] (show_stack+0x10/0x14)
	[<c010afc8>] (show_stack) from [<c090ec30>] (dump_stack_lvl+0x58/0x70)
	[<c090ec30>] (dump_stack_lvl) from [<c014bee0>] (__might_resched+0x1dc/0x270)
	[<c014bee0>] (__might_resched) from [<c059a1a0>] (__pm_runtime_resume+0x2c/0x6c)
	[<c059a1a0>] (__pm_runtime_resume) from [<c04b8a44>] (pl330_issue_pending+0x60/0x84)
	[<c04b8a44>] (pl330_issue_pending) from [<c07306b8>] (snd_dmaengine_pcm_trigger+0xec/0x14c)
	[<c07306b8>] (snd_dmaengine_pcm_trigger) from [<c0767528>] (soc_component_trigger+0x20/0x38)
	[<c0767528>] (soc_component_trigger) from [<c0768440>] (snd_soc_pcm_component_trigger+0xd8/0xf4)
	[<c0768440>] (snd_soc_pcm_component_trigger) from [<c0768e34>] (soc_pcm_trigger+0x48/0x154)
	[<c0768e34>] (soc_pcm_trigger) from [<c0725f74>] (snd_pcm_action_single+0x38/0x64)
	[<c0725f74>] (snd_pcm_action_single) from [<c0727f28>] (snd_pcm_action+0x5c/0x60)
	[<c0727f28>] (snd_pcm_action) from [<c0727f68>] (snd_pcm_action_lock_irq+0x28/0x3c)
	[<c0727f68>] (snd_pcm_action_lock_irq) from [<c027f474>] (vfs_ioctl+0x20/0x38)
	[<c027f474>] (vfs_ioctl) from [<c027fe54>] (sys_ioctl+0xc0/0x96c)
	[<c027fe54>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c)

Now that I have a reliable reproducer, it turns out the original patch
in this thread also has problems and causes a WARN from RCU.  The
version I have now that seems to work and doesn't cause any dmesg
complaints is below, but I'm really not sure if this is considered an
acceptable use of schedule_rtlock() (I suspect this also fails to
compile without CONFIG_PREEMPT_RT since schedule_rtlock() isn't declared
in that case).


-- >8 --
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ec94049442b9..79cf9997f71b 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
                        goto out;
                }
 
-               if (dev->power.irq_safe) {
+               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
                        spin_unlock(&dev->power.lock);
 
                        cpu_relax();
@@ -614,7 +614,10 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 
                        spin_unlock_irq(&dev->power.lock);
 
-                       schedule();
+                       if (dev->power.irq_safe)
+                               schedule_rtlock();
+                       else
+                               schedule();
 
                        spin_lock_irq(&dev->power.lock);
                }
@@ -777,7 +780,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
                        goto out;
                }
 
-               if (dev->power.irq_safe) {
+               if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
                        spin_unlock(&dev->power.lock);
 
                        cpu_relax();
@@ -796,7 +799,10 @@ static int rpm_resume(struct device *dev, int rpmflags)
 
                        spin_unlock_irq(&dev->power.lock);
 
-                       schedule();
+                       if (dev->power.irq_safe)
+                               schedule_rtlock();
+                       else
+                               schedule();
 
                        spin_lock_irq(&dev->power.lock);
                }

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-25 10:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 15:54 [RFC PATCH RT] PM: runtime: avoid retry loops on RT John Keeping
2021-10-05 16:38 ` Rafael J. Wysocki
2021-10-05 17:17   ` John Keeping
2021-10-06 17:05     ` Rafael J. Wysocki
2021-10-06 18:18       ` John Keeping
2021-10-21 10:37         ` Rafael J. Wysocki
2021-10-25 10:30           ` John Keeping

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).