| Message ID | 20240228020040.25815-1-qingliang.li@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM: wakeirq: fix wake irq warning in system suspend stage | expand |
Hi, On 28/02/24 07:30, Qingliang Li wrote: > When driver registers the wake irq with reverse enable ordering, > the wake irq will be re-enabled when entering system suspend, triggering > an 'Unbalanced enable for IRQ xxx' warning. The wake irq will be > enabled in both dev_pm_enable_wake_irq_complete() and dev_pm_arm_wake_irq() > > To fix this issue, complete the setting of WAKE_IRQ_DEDICATED_ENABLED flag > in dev_pm_enable_wake_irq_complete() to avoid redundant irq enablement. Just trying to understand, why not in dev_pm_arm_wake_irq ? Is it cuz it's called much after dev_pm_enable_wake_irq_complete ? Not sure what's the exact call order, but I am assuming dev_pm_enable_wake_irq_complete is more of a runtime thing and dev_pm_arm_wake_irq happens finally at system suspend? > > Fixes: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") > Signed-off-by: Qingliang Li <qingliang.li@mediatek.com> > --- $subject: Most recent convention used for this file is: "PM: sleep: wakeirq: ..." > drivers/base/power/wakeirq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c > index 42171f766dcb..5a5a9e978e85 100644 > --- a/drivers/base/power/wakeirq.c > +++ b/drivers/base/power/wakeirq.c > @@ -313,8 +313,10 @@ void dev_pm_enable_wake_irq_complete(struct device *dev) > return; > > if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED && > - wirq->status & WAKE_IRQ_DEDICATED_REVERSE) > + wirq->status & WAKE_IRQ_DEDICATED_REVERSE) { > enable_irq(wirq->irq); > + wirq->status |= WAKE_IRQ_DEDICATED_ENABLED; > + } But this does make sense to make sure status is updated, You can pick my R-by. Reviewed-by: Dhruva Gole <d-gole@ti.com>
Hello, On 28/02/24 14:03, Qingliang Li (黎晴亮) wrote: > On Wed, 2024-02-28 at 11:34 +0530, Dhruva Gole wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> Hi, >> >> On 28/02/24 07:30, Qingliang Li wrote: >>> When driver registers the wake irq with reverse enable ordering, >>> the wake irq will be re-enabled when entering system suspend, >> triggering >>> an 'Unbalanced enable for IRQ xxx' warning. The wake irq will be >>> enabled in both dev_pm_enable_wake_irq_complete() and >> dev_pm_arm_wake_irq() >>> >>> To fix this issue, complete the setting of >> WAKE_IRQ_DEDICATED_ENABLED flag >>> in dev_pm_enable_wake_irq_complete() to avoid redundant irq >> enablement. >> >> >> Just trying to understand, why not in dev_pm_arm_wake_irq ? >> Is it cuz it's called much after dev_pm_enable_wake_irq_complete ? >> Not sure what's the exact call order, but I am assuming >> dev_pm_enable_wake_irq_complete is more of a runtime thing and >> dev_pm_arm_wake_irq happens finally at system suspend? > > You are right, the involvement of 'dev_pm_enable_wake_irq_complete' is > due to the driver selecting 'pm_runtime_force_suspend' as the callback > function for system suspend. In this scenario, the call sequence during > system suspend is as follows: > dpm_suspend_start -> dpm_run_callback -> pm_runtime_force_suspend -> > dev_pm_enable_wake_irq_check/complete > suspend_enter -> dpm_suspend_noirq -> dev_pm_arm_wake_irq OK this is what I expected, thanks for clarifying! > > Based on the above, if the driver (i) chooses pm_runtime_force_suspend > as the system suspend callback function and (ii) registers wake irq > with reverse enable ordering, the wake irq will be enabled twice during > system suspend. Yep, makes sense > >> >>> >>> Fixes: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") >>> Signed-off-by: Qingliang Li <qingliang.li@mediatek.com> >>> --- >> >> $subject: Most recent convention used for this file is: >> "PM: sleep: wakeirq: ..." > > I'm sorry, but what is the problem with the description of the "Fixed" > field? I didn't get your point and I wrote it according to the previous > patches. I am not talking about your "Fixed", I am taking about the subject line of the patch. You've used "PM: wakeirq: fix wake ..." Instead use "PM: sleep: wakeirq: fix wake ..." No strong objections here, it's just a nit. [..snip..]
On Wed, Feb 28, 2024 at 9:33 AM Qingliang Li (黎晴亮) <Qingliang.Li@mediatek.com> wrote: > > On Wed, 2024-02-28 at 11:34 +0530, Dhruva Gole wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Hi, > > > > On 28/02/24 07:30, Qingliang Li wrote: > > > When driver registers the wake irq with reverse enable ordering, > > > the wake irq will be re-enabled when entering system suspend, > > triggering > > > an 'Unbalanced enable for IRQ xxx' warning. The wake irq will be > > > enabled in both dev_pm_enable_wake_irq_complete() and > > dev_pm_arm_wake_irq() > > > > > > To fix this issue, complete the setting of > > WAKE_IRQ_DEDICATED_ENABLED flag > > > in dev_pm_enable_wake_irq_complete() to avoid redundant irq > > enablement. > > > > > > Just trying to understand, why not in dev_pm_arm_wake_irq ? > > Is it cuz it's called much after dev_pm_enable_wake_irq_complete ? > > Not sure what's the exact call order, but I am assuming > > dev_pm_enable_wake_irq_complete is more of a runtime thing and > > dev_pm_arm_wake_irq happens finally at system suspend? > > You are right, the involvement of 'dev_pm_enable_wake_irq_complete' is > due to the driver selecting 'pm_runtime_force_suspend' as the callback > function for system suspend. In this scenario, the call sequence during > system suspend is as follows: > dpm_suspend_start -> dpm_run_callback -> pm_runtime_force_suspend -> > dev_pm_enable_wake_irq_check/complete > suspend_enter -> dpm_suspend_noirq -> dev_pm_arm_wake_irq > > Based on the above, if the driver (i) chooses pm_runtime_force_suspend > as the system suspend callback function and (ii) registers wake irq > with reverse enable ordering, the wake irq will be enabled twice during > system suspend. It would be good to put the above information into the patch changelog, as it actually explains the problem. Thanks!
diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c index 42171f766dcb..5a5a9e978e85 100644 --- a/drivers/base/power/wakeirq.c +++ b/drivers/base/power/wakeirq.c @@ -313,8 +313,10 @@ void dev_pm_enable_wake_irq_complete(struct device *dev) return; if (wirq->status & WAKE_IRQ_DEDICATED_MANAGED && - wirq->status & WAKE_IRQ_DEDICATED_REVERSE) + wirq->status & WAKE_IRQ_DEDICATED_REVERSE) { enable_irq(wirq->irq); + wirq->status |= WAKE_IRQ_DEDICATED_ENABLED; + } } /**
When driver registers the wake irq with reverse enable ordering, the wake irq will be re-enabled when entering system suspend, triggering an 'Unbalanced enable for IRQ xxx' warning. The wake irq will be enabled in both dev_pm_enable_wake_irq_complete() and dev_pm_arm_wake_irq() To fix this issue, complete the setting of WAKE_IRQ_DEDICATED_ENABLED flag in dev_pm_enable_wake_irq_complete() to avoid redundant irq enablement. Fixes: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") Signed-off-by: Qingliang Li <qingliang.li@mediatek.com> --- drivers/base/power/wakeirq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)