| Message ID | 1478801227-65527-1-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)