linux-kernel - Re: [PATCH v2] PM: runtime: enable wake irq after runtime

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1626758818.4247.0.camel@mhfsdcap03>
Date:   Tue, 20 Jul 2021 13:26:58 +0800
From:   Chunfeng Yun <chunfeng.yun@...iatek.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
CC:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@...ts.infradead.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Ikjoon Jang <ikjn@...omium.org>,
        "Tony Lindgren" <tony@...mide.com>
Subject: Re: [PATCH v2] PM: runtime: enable wake irq after runtime_suspend
 hook called

On Fri, 2021-07-16 at 14:52 +0200, Rafael J. Wysocki wrote:
> On Fri, Jul 16, 2021 at 11:23 AM Chunfeng Yun <chunfeng.yun@...iatek.com> wrote:
> >
> > When the dedicated wake-irq is level trigger, and it uses the
> > consumer's sleep status as the wakeup source,
> 
> This is not a typical case, though, AFAICS.
> 
> So you seem to have a special requirement and you want to change a
> generic framework in an incompatible way to accommodate it.
> 
> > that means if the
> > consumer is not in sleep state, the wake-irq will be triggered
> > when enable it; For this case, need enable the wake-irq after
> > invoking the consumer's runtime_suspend() which make the consumer
> > enter sleep state.
> >
> > e.g.
> > Assume the wake-irq is a low level trigger type, and the wakeup
> > signal comes from the sleep status of consumer.
> > The wakeup signal is low level at running time (0), and becomes
> > high level when the consumer enters sleep state (runtime_suspend
> > (1) is called), a wakeup event at (2) make the consumer exit sleep
> > state, then the wakeup signal also becomes low level.
> >
> >                 ------------------
> >                |           ^     ^|
> > ----------------           |     | --------------
> >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> >
> > if enable the wake-irq before calling runtime_suspend during (0),
> > an interrupt will arise, it causes resume immediately;
> > it works if enable wake-irq ( e.g. at (3) or (4)) after calling
> > runtime_suspend.
> >
> > In this example, can't fix it by using falling edge trigger without
> > this patch, the issue will happen as below steps:
> > 1. use another wakeup source to wake up the suspended system;
> > 2. the consumer's resume() will be called, and exits sleep state;
> > 3. the consumer's wakeup signal will fall into low level, due to
> >    currently the wakeup irq is disabled, the wake-irq is pending;
> > 4. the consumer tries to enter runtime suspend, but there is a
> >    pending wakeup irq, so will resume again, this will repeat
> >    endlessly.
> >
> > This patch seems no side effect on edge trigger wake-irq that works
> > before.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@...iatek.com>
> > ---
> > v2: add more commit message
> >
> >   I use the falling edge trigger interrupt suggested by Ikjoon [1], it
> > works well at firstly when only use this related wakeup source, but
> > encounter issues if use other wakeup sources to wakeup platform as
> > described in commit message.
> >   Send out the patch again for further discussion.
> >
> > [1]: https://patchwork.kernel.org/patch/12190407
> >
> > ---
> >  drivers/base/power/runtime.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 8a66eaf731e4..90a91b2b1364 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -639,12 +639,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         __update_runtime_status(dev, RPM_SUSPENDING);
> >
> >         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> > -
> > -       dev_pm_enable_wake_irq_check(dev, true);
> 
> I would suggest adding a wake IRQ flag to indicate that it needs
> special handling and modifying the above to take that new flag into
> account.
> 
> >         retval = rpm_callback(callback, dev);
> >         if (retval)
> >                 goto fail;
> >
> > +       dev_pm_enable_wake_irq_check(dev, true);
> 
> Then, you can add an entirely new call here, say
> dev_pm_enable_wake_irq_complete(dev), that will do the actual enable
> for the wake IRQs with the new flag set and nothing for the others.
> 
> > +
> >   no_callback:
> >         __update_runtime_status(dev, RPM_SUSPENDED);
> >         pm_runtime_deactivate_timer(dev);
> > @@ -690,7 +690,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         return retval;
> >
> >   fail:
> > -       dev_pm_disable_wake_irq_check(dev);
> 
> And obviously this will really disable wake IRQs with the new flag unset only.

Ok, I'll try it, thanks a lot

> 
> >         __update_runtime_status(dev, RPM_ACTIVE);
> >         dev->power.deferred_resume = false;
> >         wake_up_all(&dev->power.wait_queue);
> > --
> > 2.18.0
> >