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

Groups > linux.kernel > #1519257 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Brian Norris <briannorris@chromium.org> - 2016-11-10 19:10 +0100
    Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-11-10 19:20 +0100
      Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Brian Norris <briannorris@chromium.org> - 2016-11-10 19:50 +0100
        Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-10 21:50 +0100
          Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Brian Norris <briannorris@chromium.org> - 2016-11-10 22:40 +0100
            Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-11 17:50 +0100
              Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Brian Norris <briannorris@chromium.org> - 2016-11-11 20:50 +0100
        Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-11 01:10 +0100
          Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Tony Lindgren <tony@atomide.com> - 2016-11-11 17:40 +0100
    Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Pavel Machek <pavel@ucw.cz> - 2016-11-10 22:00 +0100
      Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Brian Norris <briannorris@chromium.org> - 2016-11-10 22:50 +0100

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

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()).

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/base/power/wakeirq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
index 0d77cd6fd8d1..c35b2db1194c 100644
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -139,6 +139,8 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
 	struct wake_irq *wirq = _wirq;
 	int res;
 
+	pm_wakeup_event(wirq->dev, 0);
+
 	/* We don't want RPM_ASYNC or RPM_NOWAIT here */
 	res = pm_runtime_resume(wirq->dev);
 	if (res < 0)
@@ -240,7 +242,7 @@ void dev_pm_disable_wake_irq(struct device *dev)
 	struct wake_irq *wirq = dev->power.wakeirq;
 
 	if (wirq && wirq->dedicated_irq)
-		disable_irq_nosync(wirq->irq);
+		disable_irq(wirq->irq);
 }
 EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
 
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1519270

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.

>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/base/power/wakeirq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> index 0d77cd6fd8d1..c35b2db1194c 100644
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -139,6 +139,8 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
>         struct wake_irq *wirq = _wirq;
>         int res;
>
> +       pm_wakeup_event(wirq->dev, 0);
> +
>         /* We don't want RPM_ASYNC or RPM_NOWAIT here */
>         res = pm_runtime_resume(wirq->dev);
>         if (res < 0)
> @@ -240,7 +242,7 @@ void dev_pm_disable_wake_irq(struct device *dev)
>         struct wake_irq *wirq = dev->power.wakeirq;
>
>         if (wirq && wirq->dedicated_irq)
> -               disable_irq_nosync(wirq->irq);
> +               disable_irq(wirq->irq);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
>
> --
> 2.8.0.rc3.226.g39d4020
>



-- 
Dmitry

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

#1519296

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.

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

#1519353

* Brian Norris <briannorris@chromium.org> [161110 11:49]:
> 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?

__pm_wakeup_event() ?

Tony

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

#1519380

On Thu, Nov 10, 2016 at 01:49:11PM -0700, Tony Lindgren wrote:
> * Brian Norris <briannorris@chromium.org> [161110 11:49]:
> > 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?
> 
> __pm_wakeup_event() ?

That's not the difficult part. (This patch already uses
pm_wakeup_event() correctly. It's in the ISR, and it doesn't get nested
within any other lock-holding code, so it should use the non-underscore
version, which grabs the lock.)

The difficult part is guaranteeing that the wake IRQ gets reported at
the appropriate time. It seems highly unlikely that a threaded IRQ like
this would take longer than the time for devices to resume, but it's not
guaranteed. So the question is where/when/how we call synchronize_irq().

Brian

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

#1519905

* Brian Norris <briannorris@chromium.org> [161110 13:30]:
> On Thu, Nov 10, 2016 at 01:49:11PM -0700, Tony Lindgren wrote:
> > * Brian Norris <briannorris@chromium.org> [161110 11:49]:
> > > 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?
> > 
> > __pm_wakeup_event() ?
> 
> That's not the difficult part. (This patch already uses
> pm_wakeup_event() correctly. It's in the ISR, and it doesn't get nested
> within any other lock-holding code, so it should use the non-underscore
> version, which grabs the lock.)

OK, right it's the disable_wake_irq() that takes the lock.

> The difficult part is guaranteeing that the wake IRQ gets reported at
> the appropriate time. It seems highly unlikely that a threaded IRQ like
> this would take longer than the time for devices to resume, but it's not
> guaranteed. So the question is where/when/how we call synchronize_irq().

Yeah OK.

FYI, the wake up time can be really long as in tens of milliseconds
in some cases when enabling regulator(s) and before the state is
restored. Then not using threaded IRQ for the wakeirq will lead
into extra troubles as that assumes that the consumer devices has
pm_runtime_irq_safe() set. And having pm_runtime_irq_safe() will
lead into extra issues in the driver as it permanently blocks the
consume parent device PM.

But sounds like the threaded IRQ is not your concern and you mostly
care about getting the right time for the wake up interrupt.
The wakeup interrupt controller knows something happened earlier,
so maybe it could report that time if queried somehow?

Regards,

Tony

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

#1519995

On Fri, Nov 11, 2016 at 08:47:54AM -0800, Tony Lindgren wrote:
> But sounds like the threaded IRQ is not your concern and you mostly

Right, threaded is OK for this; it's not performance critical. It just
highlighted the fact that its completion is not synchronized with
anything.

> care about getting the right time for the wake up interrupt.

Not "time", per se, but blame. But that blame is timing related: if it
comes after the system finished resuming, then it's useless, since
user-space won't know to come back and check later.

> The wakeup interrupt controller knows something happened earlier,
> so maybe it could report that time if queried somehow?

Sort of. We have /sys/power/pm_wakeup_irq already. But it's really less
useful to get IRQ-level stats for this, than to get device info. AFAICT,
there's no machine-readable association between IRQs and devices; the
best you can get is by parsing the names in /proc/interrupts.

Or, if we really want to say that's sufficient, then maybe we should
kill all the device-level wakeup stats in sysfs... (Is that what the
flamewar was all about? I hope I'm not poking the hornet's nest.)

BTW, for context, I'm working on using dev_pm_set_dedicated_wake_irq()
for a Wifi driver which supports out-of-band (e.g., GPIO-based) wakeup.
I see it's used in the I2C core, but the I2C code never actually calls
dev_pm_enable_wake_irq(). So while I think I can use this API OK for
my Wifi driver (calling dev_pm_{en,dis}able_wake_irq() at system
suspend/resume), I'm not sure this will help the I2C case.

The more I look at this API, the more I'm confused, especially about its
seeming dependence on runtime PM.

Brian

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

#1519465

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?

Thanks,
Rafael

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

#1519902

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

Regards,

Tony

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

#1519359

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-11-10 10:07:07, Brian Norris 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()).
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

How is this supposed to be presented to userspace?

There was big flamewar about that some time ago, and "what woke up the
system" is pretty much meaningless, and certainly unavailable on most
PC class hardware. Good question to ask is "what wakeup events
happened while the userspace was not available"....

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1519387

On Thu, Nov 10, 2016 at 09:57:20PM +0100, Pavel Machek wrote:
> On Thu 2016-11-10 10:07:07, Brian Norris 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()).
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> 
> How is this supposed to be presented to userspace?

/sys/kernel/debug/wakeup_sources or /sys/devices/.../power/wakeup*, for
some examples.

> There was big flamewar about that some time ago, and "what woke up the
> system" is pretty much meaningless, and certainly unavailable on most
> PC class hardware.

OK... I'm not privy to the flamewar, but I'm also not sure how it's
relevant here. These APIs specifically handle an IRQ that is solely used
for wakeup purposes, and so it should clearly be able to tell us
something about who/what woke the system.

> Good question to ask is "what wakeup events
> happened while the userspace was not available"....

That's what I'm patching here. handle_threaded_wake_irq() makes no note
of the wake event at the moment.

Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web