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@xxxxxxxxxxx> wrote:
> * Rafael J. Wysocki <rafael@xxxxxxxxxx> [161111 15:35]:
>> On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>> > * Tony Lindgren <tony@xxxxxxxxxxx> [161111 14:29]:
>> >> * Rafael J. Wysocki <rafael@xxxxxxxxxx> [161111 13:33]:
>> >> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>> >> > > * Rafael J. Wysocki <rafael@xxxxxxxxxx> [161110 16:06]:
>> >> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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