From: Chunfeng Yun <chunfeng.yun@mediatek.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Linux PM <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC..."
<linux-mediatek@lists.infradead.org>,
Alan Stern <stern@rowland.harvard.edu>,
Ikjoon Jang <ikjn@chromium.org>,
"Tony Lindgren" <tony@atomide.com>
Subject: Re: [PATCH v2] PM: runtime: enable wake irq after runtime_suspend hook called
Date: Tue, 20 Jul 2021 13:26:58 +0800 [thread overview]
Message-ID: <1626758818.4247.0.camel@mhfsdcap03> (raw)
In-Reply-To: <CAJZ5v0iH7D=D70ea7Q0LqP0iUdRYPf4GH2g7dC7J2_54WW1C8A@mail.gmail.com>
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@mediatek.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@mediatek.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
> >
prev parent reply other threads:[~2021-07-20 5:27 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-16 9:23 [PATCH v2] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
2021-07-16 12:52 ` Rafael J. Wysocki
2021-07-20 5:26 ` Chunfeng Yun [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1626758818.4247.0.camel@mhfsdcap03 \
--to=chunfeng.yun@mediatek.com \
--cc=gregkh@linuxfoundation.org \
--cc=ikjn@chromium.org \
--cc=len.brown@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=pavel@ucw.cz \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=stern@rowland.harvard.edu \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).