On Sat, Nov 12, 2016 at 1:19 AM, Tony Lindgren [off-list ref] wrote:

* Rafael J. Wysocki [off-list ref] [161111 15:35]:

quoted

On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren [off-list ref] wrote:

quoted

* Tony Lindgren [off-list ref] [161111 14:29]:

quoted

* Rafael J. Wysocki [off-list ref] [161111 13:33]:

quoted

On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren [off-list ref] wrote:

quoted

* Rafael J. Wysocki [off-list ref] [161110 16:06]:

quoted

On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris [off-list ref] wrote:

quoted

On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote:

quoted

On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris [off-list ref] wrote:

quoted

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