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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0iH7D=D70ea7Q0LqP0iUdRYPf4GH2g7dC7J2_54WW1C8A@mail.gmail.com>
Date:   Fri, 16 Jul 2021 14:52:20 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Chunfeng Yun <chunfeng.yun@...iatek.com>
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, 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.

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