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

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

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.


>
> > > > +               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
> > > >