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 Tony Lindgren <tony@atomide.com> - 2016-11-11 21:20 +0100
                Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs Alan Stern <stern@rowland.harvard.edu> - 2016-11-11 22:10 +0100
                Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-11 22: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 "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
    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

Page 1 of 2  [1] 2  Next page →

#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]

#1520006

* 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);
+	}
 }

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

#1520024

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

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

#1520045

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

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

#1520043

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] | [prev] | [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]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web