| Message ID | 1643016384-11161-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RESEND] PM: wakeup: Wakeup accounting for interrupts | expand |
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 >
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 > >
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 > > >
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 > > > >
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 > > > > >
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 > > > > > >
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); } }
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(+)