Message-ID: <df145215a21b67b440405736d9628199ccce2a1b.camel@mediatek.com>
Date:   Sat, 23 Oct 2021 14:35:37 +0800
From:   Chunfeng Yun <chunfeng.yun@...iatek.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
CC:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Mathias Nyman <mathias.nyman@...el.com>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" 
        <linux-usb@...r.kernel.org>,
        "Linux ARM" <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@...ts.infradead.org>,
        Eddie Hung <eddie.hung@...iatek.com>,
        Ikjoon Jang <ikjn@...omium.org>
Subject: Re: [PATCH v3 1/2] PM / wakeirq: support enabling wake-up irq after
 runtime_suspend called

On Tue, 2021-10-19 at 17:28 +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 11, 2021 at 5:05 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, 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.
> 
> The terminology above is confusing.
> 
> As a rule, the term "sleep state" applies to the whole system, not to
> an individual component.  It is better to use the term "low-power
> state" instead.  Also, there may be multiple low-power states per
> device and it is not clear which of them is relevant here. 
For USB3 controller, here means enter U3 state (or PCI pm state,
D3hot/D3cold)

>  My guess
> is that the IRQ will trigger unless power is removed from the device
> and you want to remove power from the device in ->runtime_suspend().
The power is off or on dependents on this devices has independent
mtcmos, no matter which case, wake IRQ need be enabled after
->runtime_suspend().
> 
> Moreover, I'm assuming that "the consumer" means "the device using
> the
> wake IRQ", 
Yes, it's.
> but this is not particularly clear either.
> 
> Now, the problem is that you need the device using the wake IRQ to be
> in a low-power state in which the IRQ doesn't trigger automatically
> before enabling the IRQ, and so you need to enable the IRQ after
> running ->runtime_suspend() for that device
Yes
>  and IMO this is how it
> needs to be described.
Ok, I'll modify commit message.

> 
> > 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.
> > 
> > This patch introduces a new status WAKE_IRQ_DEDICATED_LATE_ENABLED
> > to optionally support enabling wake-irq after calling
> > runtime_suspend().
> > 
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@...iatek.com>
> > ---
> > v3: add new status suggested by Rafael
> > 
> > v2: add more commit message
> > 
> >   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
> > 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.
> > 
> >   Send out the patch again for further discussion.
> > 
> > [1]: https://patchwork.kernel.org/patch/12190407
> > 
> > ---
> >  drivers/base/power/power.h   |  7 ++++--
> >  drivers/base/power/runtime.c |  6 +++--
> >  drivers/base/power/wakeirq.c | 49
> > +++++++++++++++++++++++++++++++++---
> >  include/linux/pm_wakeirq.h   |  5 ++++
> >  4 files changed, 60 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/base/power/power.h
> > b/drivers/base/power/power.h
> > index 54292cdd7808..2d5dfc886f0b 100644
> > --- a/drivers/base/power/power.h
> > +++ b/drivers/base/power/power.h
> > @@ -25,8 +25,10 @@ extern u64 pm_runtime_active_time(struct device
> > *dev);
> > 
> >  #define WAKE_IRQ_DEDICATED_ALLOCATED   BIT(0)
> >  #define WAKE_IRQ_DEDICATED_MANAGED     BIT(1)
> > +#define WAKE_IRQ_DEDICATED_LATE_ENABLED        BIT(2)
> 
> This name is a bit long and it doesn't reflect the nautre of the
> problem, which is that you need code ordering that is a reverse of
> the
> usual flow.
> 
> WAKE_IRQ_DEDICATED_REVERSE may be a better name.
OK

> 
> >  #define
> > WAKE_IRQ_DEDICATED_MASK                (WAKE_IRQ_DEDICATED_ALLOCATE
> > D | \
> > -                                        WAKE_IRQ_DEDICATED_MANAGED
> > )
> > +                                        WAKE_IRQ_DEDICATED_MANAGED
> > | \
> > +                                        WAKE_IRQ_DEDICATED_LATE_EN
> > ABLED)
> > 
> >  struct wake_irq {
> >         struct device *dev;
> > @@ -39,7 +41,8 @@ extern void dev_pm_arm_wake_irq(struct wake_irq
> > *wirq);
> >  extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
> >  extern void dev_pm_enable_wake_irq_check(struct device *dev,
> >                                          bool can_change_status);
> > -extern void dev_pm_disable_wake_irq_check(struct device *dev);
> > +extern void dev_pm_disable_wake_irq_check(struct device *dev, bool
> > skip_enable_late);
> > +extern void dev_pm_enable_wake_irq_complete(struct device *dev);
> > 
> >  #ifdef CONFIG_PM_SLEEP
> > 
> > diff --git a/drivers/base/power/runtime.c
> > b/drivers/base/power/runtime.c
> > index 8a66eaf731e4..97646aa11376 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -645,6 +645,8 @@ static int rpm_suspend(struct device *dev, int
> > rpmflags)
> >         if (retval)
> >                 goto fail;
> > 
> > +       dev_pm_enable_wake_irq_complete(dev);
> > +
> >   no_callback:
> >         __update_runtime_status(dev, RPM_SUSPENDED);
> >         pm_runtime_deactivate_timer(dev);
> > @@ -690,7 +692,7 @@ static int rpm_suspend(struct device *dev, int
> > rpmflags)
> >         return retval;
> > 
> >   fail:
> > -       dev_pm_disable_wake_irq_check(dev);
> > +       dev_pm_disable_wake_irq_check(dev, false);
> >         __update_runtime_status(dev, RPM_ACTIVE);
> >         dev->power.deferred_resume = false;
> >         wake_up_all(&dev->power.wait_queue);
> > @@ -873,7 +875,7 @@ static int rpm_resume(struct device *dev, int
> > rpmflags)
> > 
> >         callback = RPM_GET_CALLBACK(dev, runtime_resume);
> > 
> > -       dev_pm_disable_wake_irq_check(dev);
> > +       dev_pm_disable_wake_irq_check(dev, true);
> >         retval = rpm_callback(callback, dev);
> >         if (retval) {
> >                 __update_runtime_status(dev, RPM_SUSPENDED);
> > diff --git a/drivers/base/power/wakeirq.c
> > b/drivers/base/power/wakeirq.c
> > index 3bad3266a2ad..a612f5c26c6c 100644
> > --- a/drivers/base/power/wakeirq.c
> > +++ b/drivers/base/power/wakeirq.c
> > @@ -215,6 +215,24 @@ int dev_pm_set_dedicated_wake_irq(struct
> > device *dev, int irq)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_set_dedicated_wake_irq);
> > 
> > +/**
> > + * dev_pm_wake_irq_set_late_enabled_status - set status
> > WAKE_IRQ_DEDICATED_LATE_ENABLED
> > + * @dev: Device
> > + *
> > + * Set the status of WAKE_IRQ_DEDICATED_LATE_ENABLED to tell
> > rpm_suspend()
> > + * to enable dedicated wake-up interrupt after invoking the
> > runtime_suspend(),
> > + *
> > + * Should be called after setting dedicated wake-up interrupt.
> > + */
> > +void dev_pm_wake_irq_set_late_enabled_status(struct device *dev)
> > +{
> > +       struct wake_irq *wirq = dev->power.wakeirq;
> > +
> > +       if (wirq && (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED))
> > +               wirq->status |= WAKE_IRQ_DEDICATED_LATE_ENABLED;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_wake_irq_set_late_enabled_status);
> 
> Instead of doing this, I would provide a special version of
> dev_pm_set_dedicated_wake_irq() for this special use case such that
> it
> will set WAKE_IRQ_DEDICATED_LATE_ENABLED (or whatever you call it) at
> the allocation time (because this is a property of the IRQ and not
> something that can change).
> 
> Maybe call it dev_pm_set_dedicated_wake_irq_reverse() and implemet
> both it and dev_pm_set_dedicated_wake_irq() as wrappers around
> something like __dev_pm_set_dedicated_wake_irq() taking an extra
> argument that will indicate whether or not to set the new flag for
> this IRQ.
OK, it's a better way.

> 
> > +
> >  /**
> >   * dev_pm_enable_wake_irq - Enable device wake-up interrupt
> >   * @dev: Device
> > @@ -285,27 +303,52 @@ void dev_pm_enable_wake_irq_check(struct
> > device *dev,
> >         return;
> > 
> >  enable:
> > -       enable_irq(wirq->irq);
> > +       if (!can_change_status || !(wirq->status &
> > WAKE_IRQ_DEDICATED_LATE_ENABLED))
> > +               enable_irq(wirq->irq);
> >  }
> > 
> >  /**
> >   * dev_pm_disable_wake_irq_check - Checks and disables wake-up
> > interrupt
> >   * @dev: Device
> > + * @skip_late_enabled_status: skip checking
> > WAKE_IRQ_DEDICATED_LATE_ENABLED
> 
> I would call this argument "cond_disable" or similarly to mean that
> the IRQ should be disabled conditionally depending on the new flag.
> 
> And the description of it would be "If set, also check
> WAKE_IRQ_DEDICATED_LATE_ENABLED".
OK
> 
> >   *
> >   * Disables wake-up interrupt conditionally based on status.
> >   * Should be only called from rpm_suspend() and rpm_resume() path.
> >   */
> > -void dev_pm_disable_wake_irq_check(struct device *dev)
> > +void dev_pm_disable_wake_irq_check(struct device *dev, bool
> > skip_late_enabled_status)
> 
> Can't this function be static?
If want to make it static, should move it from wakeirq.c into runtime.c

> 
> >  {
> >         struct wake_irq *wirq = dev->power.wakeirq;
> > 
> >         if (!wirq || !(wirq->status & WAKE_IRQ_DEDICATED_MASK))
> >                 return;
> 
> And I would just add the following line here:
> 
> if (cond_disable && (wirq->status & WAKE_IRQ_DEDICATED_LATE_ENABLED))
>         return;
This change doesn't cover the case (WAKE_IRQ_DEDICATED_LATE_ENABLED and
 WAKE_IRQ_DEDICATED_MANAGED are both set 1):

-->rpm_suspend(): wirq->irq is enabled
-->rpm_resume(): disable wirq->irq; (if change it, doesn't disable
wirq->irq)

> 
> > 
> > -       if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED)
> > +       if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED &&
> > +           (skip_late_enabled_status ||
> > +            !(wirq->status & WAKE_IRQ_DEDICATED_LATE_ENABLED)))
> >                 disable_irq_nosync(wirq->irq);
> >  }
> > 
> > +/**
> > + * dev_pm_enable_wake_irq_complete - enable wake irq based on
> > status
> 
> "Enable wake IRQ not enabled before"
> 
> > + * @dev: Device
> 
> "Device using the wake IRQ"
> 
> > + *
> > + * Enable wake-up interrupt conditionally based on status, mainly
> > for
> > + * enabling wake-up interrupt after runtime_suspend() is called.
> > + *
> > + * Should be only called from rpm_suspend() path.
> 
> This part of the kerneldoc comment needs to be rewritten too,
OK
>  but it
> looks like the function can be static, in which case it won't need
> the
> kerneldoc comment at all.
Will also need to move it into runtime.c if make it static

Thanks a lot
> 
> > + */
> > +void dev_pm_enable_wake_irq_complete(struct device *dev)
> > +{
> > +       struct wake_irq *wirq = dev->power.wakeirq;
> > +
> > +       if (!wirq || !(wirq->status & WAKE_IRQ_DEDICATED_MASK))
> > +               return;
> > +
> > +       if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED &&
> > +           wirq->status & WAKE_IRQ_DEDICATED_LATE_ENABLED)
> > +               enable_irq(wirq->irq);
> > +}
> > +
> >  /**
> >   * dev_pm_arm_wake_irq - Arm device wake-up
> >   * @wirq: Device wake-up interrupt
> > diff --git a/include/linux/pm_wakeirq.h
> > b/include/linux/pm_wakeirq.h
> > index cd5b62db9084..92f814d583f8 100644
> > --- a/include/linux/pm_wakeirq.h
> > +++ b/include/linux/pm_wakeirq.h
> > @@ -22,6 +22,7 @@ extern int dev_pm_set_dedicated_wake_irq(struct
> > device *dev,
> >  extern void dev_pm_clear_wake_irq(struct device *dev);
> >  extern void dev_pm_enable_wake_irq(struct device *dev);
> >  extern void dev_pm_disable_wake_irq(struct device *dev);
> > +extern void dev_pm_wake_irq_set_late_enabled_status(struct device
> > *dev);
> > 
> >  #else  /* !CONFIG_PM */
> > 
> > @@ -47,5 +48,9 @@ static inline void dev_pm_disable_wake_irq(struct
> > device *dev)
> >  {
> >  }
> > 
> > +static inline void dev_pm_wake_irq_set_late_enabled_status(struct
> > device *dev)
> > +{
> > +}
> > +
> >  #endif /* CONFIG_PM */
> >  #endif /* _LINUX_PM_WAKEIRQ_H */
> > --

Powered by blists - more mailing lists