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

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

[Brian Norris]

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

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

[Dmitry Torokhov]

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

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

[Brian Norris]

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.

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

[Tony Lindgren]

* 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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Brian Norris]

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

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

[Brian Norris]

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

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

[Rafael J. Wysocki]

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

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* 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

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

[Brian Norris]

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

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

[Tony Lindgren]

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

OK it's used for that purpose with the SDIO dat1 interrupt for
omaps. This allows the WLAN to stay on and connected while the
SoC can hit deeper idle states.

The calling of dev_pm_enable_wake_irq() happens automagically from
rpm_suspend() and then it's disabled after rpm_resume().

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

Are you talking about suspend/resume only? If so, see if the
following snippet from Grygorii helps. Grygorii, care to send it with
proper description and Signed-off-by if you did not yet do that?

Regards,

Tony

8< ----------------------
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -256,8 +256,12 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
 	if (!wirq)
 		return;
 
-	if (device_may_wakeup(wirq->dev))
+	if (device_may_wakeup(wirq->dev)) {
+		if (wirq->dedicated_irq)
+			enable_irq(wirq->irq);
+
 		enable_irq_wake(wirq->irq);
+	}
 }
 
 /**
@@ -272,6 +276,10 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
 	if (!wirq)
 		return;
 
-	if (device_may_wakeup(wirq->dev))
+	if (device_may_wakeup(wirq->dev)) {
 		disable_irq_wake(wirq->irq);
+
+		if (wirq->dedicated_irq)
+			disable_irq_nosync(wirq->irq);
+	}
 }

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

[Alan Stern]

On Fri, 11 Nov 2016, Brian Norris wrote:

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

If I recall correctly, that flamewar was about the whole idea of what
caused the system to wake up.  In general, the system does not know
what caused it to wake up.  All it knows, once it is awake again, is
what IRQs (or other similar events, such as ACPI GPEs) are pending.  It
doesn't know which of those events caused it to wake up.  And if 
multiple devices share the same IRQ line, the PM core won't know which 
of them raised the IRQ.

Of course, for some purposes this distinction doesn't matter.

Alan Stern

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

[Rafael J. Wysocki]

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

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

[Rafael J. Wysocki]

On Fri, Nov 11, 2016 at 8:40 PM, Brian Norris <briannorris@chromium.org> wrote:
> 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.)

Do you mean the wakeup_* attributes in <device>/power/ ?

If so, then they are in there, because they were asked for by people
at the time they were introduced (I can't recall exactly who wanted
them, though), but if they are not useful to anyone after all (and I
guess that this is the case), they can just go away as far as I'm
concerned.

Thanks,
Rafael

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* 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

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

[Rafael J. Wysocki]

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

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

[Tony Lindgren]

* 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

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

[Rafael J. Wysocki]

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

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

[Tony Lindgren]

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

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

[Rafael J. Wysocki]

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

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

[Tony Lindgren]

* 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