Re: [RESEND PATCH] PM: wakeup: Wakeup accounting for interrupts

[Loic Poulain <loic.poulain@linaro.org>]

On Wed, 16 Feb 2022 at 17:59, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Sorry for the delay.
>
> On Fri, Jan 28, 2022 at 10:55 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> >
> > On Thu, 27 Jan 2022 at 20:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Jan 27, 2022 at 9:51 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On Wed, 26 Jan 2022 at 19:23, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Mon, Jan 24, 2022 at 10:14 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > > > > >
> > > > > > Most of the time, system wakeup is caused by a wakeup-enabled
> > > > > > interrupt, but the accounting is not done for the related wakeup
> > > > > > source, causing 'wrong' values reported by device's wakeup attributes
> > > > > > and debugfs stats (debug/wakeup_sources).
> > > > > >
> > > > > > This change reports a wakeup event for any wakeup-sources the irq is
> > > > > > attached with.
> > > > > >
> > > > > > Note: This only works for drivers explicitly attaching the irq to
> > > > > > a given device (e.g. with dev_pm_set_wake_irq).
> > > > > >
> > > > > > Note2: Some drivers call pm_wakeup_event() in their irq handler, but
> > > > > > not all, moreover, an interrupt can be disabled while being flagged
> > > > > > as wakeup source, and so accounting must be performed. This solution
> > > > > > ensures that accounting will always be done for the interrupt waking
> > > > > > up the host.
> > > > > >
> > > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > > > > ---
> > > > > >  drivers/base/power/wakeup.c | 11 +++++++++++
> > > > > >  1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > > > > index 99bda0da..2d75e057 100644
> > > > > > --- a/drivers/base/power/wakeup.c
> > > > > > +++ b/drivers/base/power/wakeup.c
> > > > > > @@ -952,8 +952,19 @@ void pm_wakeup_clear(bool reset)
> > > > > >  void pm_system_irq_wakeup(unsigned int irq_number)
> > > > > >  {
> > > > > >         if (pm_wakeup_irq == 0) {
> > > > > > +               struct wakeup_source *ws;
> > > > > > +               int srcuidx;
> > > > > > +
> > > > > >                 pm_wakeup_irq = irq_number;
> > > > > >                 pm_system_wakeup();
> > > > > > +
> > > > > > +               /* wakeup accounting */
> > > > > > +               srcuidx = srcu_read_lock(&wakeup_srcu);
> > > > >
> > > > > This is called from interrupt context, so srcu_read_lock() cannot be used here.
> > > >
> > > >
> > > > AFAIU from srcu.h, it is not prohibited, right? and we are not
> > > > blocking while locked here.
> > >
> > > OK, you can do that if you release it from the same context.
> > >
> > > There are other concerns regarding this change, though.
> > >
> > > First off, it adds overhead for all systems, but the benefit is there
> > > only for the systems where wake IRQs are used.
> >
> > It adds very little overhead for interrupts with armed wakeup, during
> > system suspend(ing/ed) only, other IRQs are not affected.
> >
> > >
> > > Second, it may lead to reporting the same wakeup event twice in a row
> > > if the driver decides to do that too.  Is this not a problem?
> >
> > That's a legitimate point, but I don't see it as a big concern
> > compared to not recording the event at all. If a driver deliberately
> > attaches an interrupt to a wake source, it seems sane to expect that
> > this interrupt will cause a corresponding wakeup event (should
> > probably be documented), whatever the driver's irq handler is doing.
>
> This needs to be rebased on top of 5.17-rc4 after a somewhat related
> bug fix that went into it.

Ok

>
>
> Moreover, I think that you'd want to do the below for all of the
> wakeup interrupts that trigger, not just for the first one.

Yes you're right, it makes sense.

>
>
> Also, the problem mentioned in the changelog is not limited to devices
> having dedicated wakeup IRQs, but the proposed approach to address it
> is.  Why are the other cases not relevant?

Not sure to understand what other cases you are referring to? here we
ensure that interrupts bound to a wakeup-source trigger the
corresponding wakeup event. So the ones configured via one of the
dev_pm_set_(dedicated_)wake_irq functions. But many drivers do not
explicitly attach IRQ as device's wake-irq, so there is no attached
wakeup-source to manage in that case.


>
>
> > >
> > > > > > +               list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> > > > > > +                       if (ws->wakeirq && ws->wakeirq->irq == irq_number)
> > > > > > +                               pm_wakeup_ws_event(ws, 0, false);
> > > > > > +               }
> > > > > > +               srcu_read_unlock(&wakeup_srcu, srcuidx);
> > > > > >         }
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > > >