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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Nov 2016 01:35:01 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Brian Norris <briannorris@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Brian Norris <computersforpeace@...il.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs

On Sat, Nov 12, 2016 at 1:19 AM, Tony Lindgren <tony@...mide.com> wrote:
> * Rafael J. Wysocki <rafael@...nel.org> [161111 15:35]:
>> On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren <tony@...mide.com> wrote:
>> > * Tony Lindgren <tony@...mide.com> [161111 14:29]:
>> >> * Rafael J. Wysocki <rafael@...nel.org> [161111 13:33]:
>> >> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren <tony@...mide.com> wrote:
>> >> > > * Rafael J. Wysocki <rafael@...nel.org> [161110 16:06]:
>> >> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris <briannorris@...omium.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@...omium.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