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

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

[Loic Poulain]

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

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

[Rafael J. Wysocki]

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.

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

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

[Loic Poulain]

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.

Regards,
Loic

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

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

[Rafael J. Wysocki]

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.

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?

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

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

[Loic Poulain]

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

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

[Rafael J. Wysocki]

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.

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.

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?

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

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

[Loic Poulain]

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