* [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