Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520043 > unrolled thread

Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-11 22:40 +0100
    Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-11 23:30 +0100
      Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-11 23:40 +0100
        Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-12 00:40 +0100
          Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-12 01:20 +0100
            Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-12 01:40 +0100
              Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-18 21:20 +0100
                Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-23 23:40 +0100
                  Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-24 15:30 +0100

#1520043 — Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs

On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
>> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
>> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
>> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
>> >> > It's important that user space can figure out what device woke the
>> >> > system from suspend -- e.g., for debugging, or for implementing
>> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
>> >> > that.
>> >> >
>> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
>> >> > synchronize with these events in their resume path (hence, disable_irq()
>> >> > instead of disable_irq_nosync()).
>> >>
>> >> Hmm, dev_pm_disable_wake_irq() is called from
>> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
>> >> disable interrupts. Dropping _nosync() feels dangerous.
>> >
>> > Indeed. So how do you suggest we get sane wakeup reports? Every device
>> > or bus that's going to use the dedicated wake APIs has to
>> > synchronize_irq() [1] in their resume() routine? Seems like an odd
>> > implementation detail to have to remember (and therefore most drivers
>> > will get it wrong).
>> >
>> > Brian
>> >
>> > [1] Or maybe at least create a helper API that will extract the
>> > dedicated wake IRQ number and do the synchronize_irq() for us, so
>> > drivers don't have to stash this separately (or poke at
>> > dev->power.wakeirq->irq) for no good reason.
>>
>> Well, in the first place, can anyone please refresh my memory on why
>> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
>
> I guess no other reason except we need to manage the wakeirq
> for rpm_callback(). So we dev_pm_enable_wake_irq() before
> rpm_callback() in rpm_suspend(), then disable on resume.

But we drop the lock in rpm_callback(), so can't it be moved to where
the callback is invoked?

Thanks,
Rafael

[toc] | [next] | [standalone]

#1520072

* Rafael J. Wysocki <rafael@kernel.org> [161111 13:33]:
> On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
> >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
> >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
> >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
> >> >> > It's important that user space can figure out what device woke the
> >> >> > system from suspend -- e.g., for debugging, or for implementing
> >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
> >> >> > that.
> >> >> >
> >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
> >> >> > synchronize with these events in their resume path (hence, disable_irq()
> >> >> > instead of disable_irq_nosync()).
> >> >>
> >> >> Hmm, dev_pm_disable_wake_irq() is called from
> >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
> >> >> disable interrupts. Dropping _nosync() feels dangerous.
> >> >
> >> > Indeed. So how do you suggest we get sane wakeup reports? Every device
> >> > or bus that's going to use the dedicated wake APIs has to
> >> > synchronize_irq() [1] in their resume() routine? Seems like an odd
> >> > implementation detail to have to remember (and therefore most drivers
> >> > will get it wrong).
> >> >
> >> > Brian
> >> >
> >> > [1] Or maybe at least create a helper API that will extract the
> >> > dedicated wake IRQ number and do the synchronize_irq() for us, so
> >> > drivers don't have to stash this separately (or poke at
> >> > dev->power.wakeirq->irq) for no good reason.
> >>
> >> Well, in the first place, can anyone please refresh my memory on why
> >> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
> >
> > I guess no other reason except we need to manage the wakeirq
> > for rpm_callback(). So we dev_pm_enable_wake_irq() before
> > rpm_callback() in rpm_suspend(), then disable on resume.
> 
> But we drop the lock in rpm_callback(), so can't it be moved to where
> the callback is invoked?

Then we're back to patching all the drivers again, no?

Tony

[toc] | [prev] | [next] | [standalone]

#1520082

* Tony Lindgren <tony@atomide.com> [161111 14:29]:
> * Rafael J. Wysocki <rafael@kernel.org> [161111 13:33]:
> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
> > >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
> > >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
> > >> >> > It's important that user space can figure out what device woke the
> > >> >> > system from suspend -- e.g., for debugging, or for implementing
> > >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
> > >> >> > that.
> > >> >> >
> > >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
> > >> >> > synchronize with these events in their resume path (hence, disable_irq()
> > >> >> > instead of disable_irq_nosync()).
> > >> >>
> > >> >> Hmm, dev_pm_disable_wake_irq() is called from
> > >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
> > >> >> disable interrupts. Dropping _nosync() feels dangerous.
> > >> >
> > >> > Indeed. So how do you suggest we get sane wakeup reports? Every device
> > >> > or bus that's going to use the dedicated wake APIs has to
> > >> > synchronize_irq() [1] in their resume() routine? Seems like an odd
> > >> > implementation detail to have to remember (and therefore most drivers
> > >> > will get it wrong).
> > >> >
> > >> > Brian
> > >> >
> > >> > [1] Or maybe at least create a helper API that will extract the
> > >> > dedicated wake IRQ number and do the synchronize_irq() for us, so
> > >> > drivers don't have to stash this separately (or poke at
> > >> > dev->power.wakeirq->irq) for no good reason.
> > >>
> > >> Well, in the first place, can anyone please refresh my memory on why
> > >> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
> > >
> > > I guess no other reason except we need to manage the wakeirq
> > > for rpm_callback(). So we dev_pm_enable_wake_irq() before
> > > rpm_callback() in rpm_suspend(), then disable on resume.
> > 
> > But we drop the lock in rpm_callback(), so can't it be moved to where
> > the callback is invoked?
> 
> Then we're back to patching all the drivers again, no?

Sorry I misunderstood, yeah that should work if rpm_callback() drops
the lock.

Somehow I remembered we're calling the consumer callback function
directly :)

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1520101

On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [161111 14:29]:
>> * Rafael J. Wysocki <rafael@kernel.org> [161111 13:33]:
>> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > > * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
>> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
>> > >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
>> > >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
>> > >> >> > It's important that user space can figure out what device woke the
>> > >> >> > system from suspend -- e.g., for debugging, or for implementing
>> > >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
>> > >> >> > that.
>> > >> >> >
>> > >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
>> > >> >> > synchronize with these events in their resume path (hence, disable_irq()
>> > >> >> > instead of disable_irq_nosync()).
>> > >> >>
>> > >> >> Hmm, dev_pm_disable_wake_irq() is called from
>> > >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
>> > >> >> disable interrupts. Dropping _nosync() feels dangerous.
>> > >> >
>> > >> > Indeed. So how do you suggest we get sane wakeup reports? Every device
>> > >> > or bus that's going to use the dedicated wake APIs has to
>> > >> > synchronize_irq() [1] in their resume() routine? Seems like an odd
>> > >> > implementation detail to have to remember (and therefore most drivers
>> > >> > will get it wrong).
>> > >> >
>> > >> > Brian
>> > >> >
>> > >> > [1] Or maybe at least create a helper API that will extract the
>> > >> > dedicated wake IRQ number and do the synchronize_irq() for us, so
>> > >> > drivers don't have to stash this separately (or poke at
>> > >> > dev->power.wakeirq->irq) for no good reason.
>> > >>
>> > >> Well, in the first place, can anyone please refresh my memory on why
>> > >> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
>> > >
>> > > I guess no other reason except we need to manage the wakeirq
>> > > for rpm_callback(). So we dev_pm_enable_wake_irq() before
>> > > rpm_callback() in rpm_suspend(), then disable on resume.
>> >
>> > But we drop the lock in rpm_callback(), so can't it be moved to where
>> > the callback is invoked?
>>
>> Then we're back to patching all the drivers again, no?
>
> Sorry I misunderstood, yeah that should work if rpm_callback() drops
> the lock.

It still will not re-enable interrupts if the irq_safe flag is set.  I
wonder if we really care about this case, though.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1520110

* Rafael J. Wysocki <rafael@kernel.org> [161111 15:35]:
> On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Tony Lindgren <tony@atomide.com> [161111 14:29]:
> >> * Rafael J. Wysocki <rafael@kernel.org> [161111 13:33]:
> >> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> > > * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
> >> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
> >> > >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
> >> > >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
> >> > >> >> > It's important that user space can figure out what device woke the
> >> > >> >> > system from suspend -- e.g., for debugging, or for implementing
> >> > >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
> >> > >> >> > that.
> >> > >> >> >
> >> > >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
> >> > >> >> > synchronize with these events in their resume path (hence, disable_irq()
> >> > >> >> > instead of disable_irq_nosync()).
> >> > >> >>
> >> > >> >> Hmm, dev_pm_disable_wake_irq() is called from
> >> > >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
> >> > >> >> disable interrupts. Dropping _nosync() feels dangerous.
> >> > >> >
> >> > >> > Indeed. So how do you suggest we get sane wakeup reports? Every device
> >> > >> > or bus that's going to use the dedicated wake APIs has to
> >> > >> > synchronize_irq() [1] in their resume() routine? Seems like an odd
> >> > >> > implementation detail to have to remember (and therefore most drivers
> >> > >> > will get it wrong).
> >> > >> >
> >> > >> > Brian
> >> > >> >
> >> > >> > [1] Or maybe at least create a helper API that will extract the
> >> > >> > dedicated wake IRQ number and do the synchronize_irq() for us, so
> >> > >> > drivers don't have to stash this separately (or poke at
> >> > >> > dev->power.wakeirq->irq) for no good reason.
> >> > >>
> >> > >> Well, in the first place, can anyone please refresh my memory on why
> >> > >> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
> >> > >
> >> > > I guess no other reason except we need to manage the wakeirq
> >> > > for rpm_callback(). So we dev_pm_enable_wake_irq() before
> >> > > rpm_callback() in rpm_suspend(), then disable on resume.
> >> >
> >> > But we drop the lock in rpm_callback(), so can't it be moved to where
> >> > the callback is invoked?
> >>
> >> Then we're back to patching all the drivers again, no?
> >
> > Sorry I misunderstood, yeah that should work if rpm_callback() drops
> > the lock.
> 
> It still will not re-enable interrupts if the irq_safe flag is set.  I
> wonder if we really care about this case, though.

We have at least 8250-omap and serial-omap using wakeirqs with
irq_safe flag set.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1520116

On Sat, Nov 12, 2016 at 1:19 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Rafael J. Wysocki <rafael@kernel.org> [161111 15:35]:
>> On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Tony Lindgren <tony@atomide.com> [161111 14:29]:
>> >> * Rafael J. Wysocki <rafael@kernel.org> [161111 13:33]:
>> >> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@atomide.com> wrote:
>> >> > > * Rafael J. Wysocki <rafael@kernel.org> [161110 16:06]:
>> >> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@chromium.org> wrote:
>> >> > >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:
>> >> > >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris <briannorris@chromium.org> wrote:
>> >> > >> >> > It's important that user space can figure out what device woke the
>> >> > >> >> > system from suspend -- e.g., for debugging, or for implementing
>> >> > >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do
>> >> > >> >> > that.
>> >> > >> >> >
>> >> > >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to
>> >> > >> >> > synchronize with these events in their resume path (hence, disable_irq()
>> >> > >> >> > instead of disable_irq_nosync()).
>> >> > >> >>
>> >> > >> >> Hmm, dev_pm_disable_wake_irq() is called from
>> >> > >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and
>> >> > >> >> disable interrupts. Dropping _nosync() feels dangerous.
>> >> > >> >
>> >> > >> > Indeed. So how do you suggest we get sane wakeup reports? Every device
>> >> > >> > or bus that's going to use the dedicated wake APIs has to
>> >> > >> > synchronize_irq() [1] in their resume() routine? Seems like an odd
>> >> > >> > implementation detail to have to remember (and therefore most drivers
>> >> > >> > will get it wrong).
>> >> > >> >
>> >> > >> > Brian
>> >> > >> >
>> >> > >> > [1] Or maybe at least create a helper API that will extract the
>> >> > >> > dedicated wake IRQ number and do the synchronize_irq() for us, so
>> >> > >> > drivers don't have to stash this separately (or poke at
>> >> > >> > dev->power.wakeirq->irq) for no good reason.
>> >> > >>
>> >> > >> Well, in the first place, can anyone please refresh my memory on why
>> >> > >> it is necessary to call dev_pm_disable_wake_irq() under power.lock?
>> >> > >
>> >> > > I guess no other reason except we need to manage the wakeirq
>> >> > > for rpm_callback(). So we dev_pm_enable_wake_irq() before
>> >> > > rpm_callback() in rpm_suspend(), then disable on resume.
>> >> >
>> >> > But we drop the lock in rpm_callback(), so can't it be moved to where
>> >> > the callback is invoked?
>> >>
>> >> Then we're back to patching all the drivers again, no?
>> >
>> > Sorry I misunderstood, yeah that should work if rpm_callback() drops
>> > the lock.
>>
>> It still will not re-enable interrupts if the irq_safe flag is set.  I
>> wonder if we really care about this case, though.
>
> We have at least 8250-omap and serial-omap using wakeirqs with
> irq_safe flag set.

OK, that's a deal killer for this approach.

However, my understanding is that the current code actually works for
runtime PM just fine.

What Brian seems to be wanting is to make system resume synchronize
the wakeup interrupt at one point, so maybe there could be a "sync"
version of dev_pm_disable_wake_irq() to be invoked then?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1525701

Hi,

* Rafael J. Wysocki <rafael@kernel.org> [161111 16:35]:
> However, my understanding is that the current code actually works for
> runtime PM just fine.

Hmm well I just noticed that for drivers not using autosuspend it can be
flakey, see the patch below. That probably explains some mysteries people
are seeing with wakeirqs.

Do you have any better ideas for setting wirq->active on the first
rpm_suspend()?

> What Brian seems to be wanting is to make system resume synchronize
> the wakeup interrupt at one point, so maybe there could be a "sync"
> version of dev_pm_disable_wake_irq() to be invoked then?

We call rpm_resume() from handle_threaded_wake_irq(), that's no better :)

Regards,

Tony

8< -------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 18 Nov 2016 10:15:34 -0800
Subject: [PATCH] PM / wakeirq: Fix wakeirq init

I noticed some wakeirq flakeyness with consumer drivers not using
autosuspend. For drivers not using autosuspend, the wakeirq may never
get unmasked in rpm_suspend() because of irq desc->depth.

We are configuring dedicated wakeirqs to start with IRQ_NOAUTOEN as we
naturally don't want them running until rpm_suspend() is called.

However, when a consumer driver calls pm_runtime_get functions, we now
wrongly start with disable_irq_nosync() call on the dedicated wakeirq
that is disabled to start with.

This causes desc->depth to toggle between 1 and 2 instead of the usual
0 and 1. This can prevent enable_irq() from unmasking the wakeirq as
that only happens at desc->depth 1.

This does not necessarily show up with drivers using autosuspend as
there is time for disable_irq_nosync() before rpm_suspend() gets called
after the autosuspend timeout.

Fix the issue by adding wirq->active flag that lazily gets set on
the first rpm_suspend().

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/base/power/power.h   | 19 +++++++++++++++++++
 drivers/base/power/runtime.c |  1 +
 drivers/base/power/wakeirq.c | 10 ++++------
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -24,9 +24,24 @@ extern void pm_runtime_remove(struct device *dev);
 struct wake_irq {
 	struct device *dev;
 	int irq;
+	bool active:1;
 	bool dedicated_irq:1;
 };
 
+/* Caller must hold &dev->power.lock to change wirq->active */
+static inline void dev_pm_check_wake_irq(struct device *dev)
+{
+	struct wake_irq *wirq = dev->power.wakeirq;
+
+	if (!wirq)
+		return;
+
+	if (unlikely(!wirq->active)) {
+		wirq->active = true;
+		wmb();	/* ensure dev_pm_enable_wake_irq() sees active */
+	}
+}
+
 extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
 extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
 
@@ -96,6 +111,10 @@ static inline void wakeup_sysfs_remove(struct device *dev) {}
 static inline int pm_qos_sysfs_add(struct device *dev) { return 0; }
 static inline void pm_qos_sysfs_remove(struct device *dev) {}
 
+static inline void dev_pm_check_wake_irq(struct device *dev)
+{
+}
+
 static inline void dev_pm_arm_wake_irq(struct wake_irq *wirq)
 {
 }
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -592,6 +592,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
 
+	dev_pm_check_wake_irq(dev);
 	dev_pm_enable_wake_irq(dev);
 	retval = rpm_callback(callback, dev);
 	if (retval)
diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -212,8 +212,7 @@ EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq);
  * dev_pm_enable_wake_irq - Enable device wake-up interrupt
  * @dev: Device
  *
- * Called from the bus code or the device driver for
- * runtime_suspend() to enable the wake-up interrupt while
+ * Called from rpm_suspend() to enable the wake-up interrupt while
  * the device is running.
  *
  * Note that for runtime_suspend()) the wake-up interrupts
@@ -224,7 +223,7 @@ void dev_pm_enable_wake_irq(struct device *dev)
 {
 	struct wake_irq *wirq = dev->power.wakeirq;
 
-	if (wirq && wirq->dedicated_irq)
+	if (wirq && wirq->dedicated_irq && wirq->active)
 		enable_irq(wirq->irq);
 }
 EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
@@ -233,15 +232,14 @@ EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
  * dev_pm_disable_wake_irq - Disable device wake-up interrupt
  * @dev: Device
  *
- * Called from the bus code or the device driver for
- * runtime_resume() to disable the wake-up interrupt while
+ * Called from rpm_resume() to disable the wake-up interrupt while
  * the device is running.
  */
 void dev_pm_disable_wake_irq(struct device *dev)
 {
 	struct wake_irq *wirq = dev->power.wakeirq;
 
-	if (wirq && wirq->dedicated_irq)
+	if (wirq && wirq->dedicated_irq && wirq->active)
 		disable_irq_nosync(wirq->irq);
 }
 EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1528807

On Fri, Nov 18, 2016 at 9:18 PM, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Rafael J. Wysocki <rafael@kernel.org> [161111 16:35]:
>> However, my understanding is that the current code actually works for
>> runtime PM just fine.
>
> Hmm well I just noticed that for drivers not using autosuspend it can be
> flakey, see the patch below. That probably explains some mysteries people
> are seeing with wakeirqs.
>
> Do you have any better ideas for setting wirq->active on the first
> rpm_suspend()?

You could change dedicated_irq into status and have three values for
it: INVALID, ALLOCATED and IN_USE.

dev_pm_set_dedicated_wake_irq() would make it ALLOCATED and
dev_pm_check_wake_irq() would change it into IN_USE.  In turn,
dev_pm_enable/disable_wake_irq() would only touch it if it is IN_USE
and dev_pm_clear_wake_irq() would free it if it is not INVALID.

>> What Brian seems to be wanting is to make system resume synchronize
>> the wakeup interrupt at one point, so maybe there could be a "sync"
>> version of dev_pm_disable_wake_irq() to be invoked then?
>
> We call rpm_resume() from handle_threaded_wake_irq(), that's no better :)
>
> Regards,
>
> Tony
>
> 8< -------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 18 Nov 2016 10:15:34 -0800
> Subject: [PATCH] PM / wakeirq: Fix wakeirq init
>
> I noticed some wakeirq flakeyness with consumer drivers not using
> autosuspend. For drivers not using autosuspend, the wakeirq may never
> get unmasked in rpm_suspend() because of irq desc->depth.
>
> We are configuring dedicated wakeirqs to start with IRQ_NOAUTOEN as we
> naturally don't want them running until rpm_suspend() is called.
>
> However, when a consumer driver calls pm_runtime_get functions, we now
> wrongly start with disable_irq_nosync() call on the dedicated wakeirq
> that is disabled to start with.
>
> This causes desc->depth to toggle between 1 and 2 instead of the usual
> 0 and 1. This can prevent enable_irq() from unmasking the wakeirq as
> that only happens at desc->depth 1.
>
> This does not necessarily show up with drivers using autosuspend as
> there is time for disable_irq_nosync() before rpm_suspend() gets called
> after the autosuspend timeout.
>
> Fix the issue by adding wirq->active flag that lazily gets set on
> the first rpm_suspend().
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/base/power/power.h   | 19 +++++++++++++++++++
>  drivers/base/power/runtime.c |  1 +
>  drivers/base/power/wakeirq.c | 10 ++++------
>  3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -24,9 +24,24 @@ extern void pm_runtime_remove(struct device *dev);
>  struct wake_irq {
>         struct device *dev;
>         int irq;
> +       bool active:1;
>         bool dedicated_irq:1;
>  };
>
> +/* Caller must hold &dev->power.lock to change wirq->active */
> +static inline void dev_pm_check_wake_irq(struct device *dev)
> +{
> +       struct wake_irq *wirq = dev->power.wakeirq;
> +
> +       if (!wirq)
> +               return;
> +
> +       if (unlikely(!wirq->active)) {
> +               wirq->active = true;
> +               wmb();  /* ensure dev_pm_enable_wake_irq() sees active */

smp_wmb()?

Also, do we have a corresponding barrier on the reader side?

> +       }
> +}
> +
>  extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
>  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
>
> @@ -96,6 +111,10 @@ static inline void wakeup_sysfs_remove(struct device *dev) {}
>  static inline int pm_qos_sysfs_add(struct device *dev) { return 0; }
>  static inline void pm_qos_sysfs_remove(struct device *dev) {}
>
> +static inline void dev_pm_check_wake_irq(struct device *dev)
> +{
> +}
> +
>  static inline void dev_pm_arm_wake_irq(struct wake_irq *wirq)
>  {
>  }
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -592,6 +592,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>
> +       dev_pm_check_wake_irq(dev);

I wonder if it would make sense to fold dev_pm_check_wake_irq() into
dev_pm_enable_wake_irq()?

>         dev_pm_enable_wake_irq(dev);
>         retval = rpm_callback(callback, dev);
>         if (retval)
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -212,8 +212,7 @@ EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq);
>   * dev_pm_enable_wake_irq - Enable device wake-up interrupt
>   * @dev: Device
>   *
> - * Called from the bus code or the device driver for
> - * runtime_suspend() to enable the wake-up interrupt while
> + * Called from rpm_suspend() to enable the wake-up interrupt while
>   * the device is running.
>   *
>   * Note that for runtime_suspend()) the wake-up interrupts
> @@ -224,7 +223,7 @@ void dev_pm_enable_wake_irq(struct device *dev)
>  {
>         struct wake_irq *wirq = dev->power.wakeirq;
>
> -       if (wirq && wirq->dedicated_irq)
> +       if (wirq && wirq->dedicated_irq && wirq->active)
>                 enable_irq(wirq->irq);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
> @@ -233,15 +232,14 @@ EXPORT_SYMBOL_GPL(dev_pm_enable_wake_irq);
>   * dev_pm_disable_wake_irq - Disable device wake-up interrupt
>   * @dev: Device
>   *
> - * Called from the bus code or the device driver for
> - * runtime_resume() to disable the wake-up interrupt while
> + * Called from rpm_resume() to disable the wake-up interrupt while
>   * the device is running.
>   */
>  void dev_pm_disable_wake_irq(struct device *dev)
>  {
>         struct wake_irq *wirq = dev->power.wakeirq;
>
> -       if (wirq && wirq->dedicated_irq)
> +       if (wirq && wirq->dedicated_irq && wirq->active)
>                 disable_irq_nosync(wirq->irq);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
> --

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1529331

* Rafael J. Wysocki <rafael@kernel.org> [161123 14:37]:
> On Fri, Nov 18, 2016 at 9:18 PM, Tony Lindgren <tony@atomide.com> wrote:
> > Hi,
> >
> > * Rafael J. Wysocki <rafael@kernel.org> [161111 16:35]:
> >> However, my understanding is that the current code actually works for
> >> runtime PM just fine.
> >
> > Hmm well I just noticed that for drivers not using autosuspend it can be
> > flakey, see the patch below. That probably explains some mysteries people
> > are seeing with wakeirqs.
> >
> > Do you have any better ideas for setting wirq->active on the first
> > rpm_suspend()?
> 
> You could change dedicated_irq into status and have three values for
> it: INVALID, ALLOCATED and IN_USE.
> 
> dev_pm_set_dedicated_wake_irq() would make it ALLOCATED and
> dev_pm_check_wake_irq() would change it into IN_USE.  In turn,
> dev_pm_enable/disable_wake_irq() would only touch it if it is IN_USE
> and dev_pm_clear_wake_irq() would free it if it is not INVALID.

OK sounds good to me.

> > +static inline void dev_pm_check_wake_irq(struct device *dev)
> > +{
> > +       struct wake_irq *wirq = dev->power.wakeirq;
> > +
> > +       if (!wirq)
> > +               return;
> > +
> > +       if (unlikely(!wirq->active)) {
> > +               wirq->active = true;
> > +               wmb();  /* ensure dev_pm_enable_wake_irq() sees active */
> 
> smp_wmb()?
> 
> Also, do we have a corresponding barrier on the reader side?

Will check thanks.

> I wonder if it would make sense to fold dev_pm_check_wake_irq() into
> dev_pm_enable_wake_irq()?

I was thinking that too but then bus or device itself won't be
able to manage the wakeirq if needed.

Let me check if we could easily add something to initialize things
like dev_pm_manage_wake_irq() and dev_pm_dont_manage_wake_irq().
That means we need to update the drivers using it, but then we don't
need to add extra checks to the idle path and can let bus or
drivers mange the wakeirq if necessary.

Regards,

Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web