| Message ID | 20240928-fix_wakeirq-v1-1-25d13a7e13ba@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC] PM: sleep: wakeirq: Fix a serious logical error in dev_pm_disarm_wake_irq() | expand |
On Sat, Sep 28, 2024 at 02:26:27AM -0700, Zijun Hu wrote: > IT is a serious logical error for dev_pm_disarm_wake_irq() not to disable > the wake irq enabled by dev_pm_arm_wake_irq(), fixed by simply correcting > the wrong if condition. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> What commit id does this fix? > --- > List relevant commits as following: > > johan+linaro@kernel.org 2023-07-13 > Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") > > tony@atomide.com 2018-02-09 > Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") > > The former commit fixes the later. I do not understand this series of commits, what exactly are you trying to show here? confused, greg k-h
On 9/28/2024 6:50 PM, Greg Kroah-Hartman wrote: > On Sat, Sep 28, 2024 at 02:26:27AM -0700, Zijun Hu wrote: >> IT is a serious logical error for dev_pm_disarm_wake_irq() not to disable >> the wake irq enabled by dev_pm_arm_wake_irq(), fixed by simply correcting >> the wrong if condition. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > > What commit id does this fix? > it is below commit ever mentioned. tony@atomide.com 2018-02-09 Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") >> --- >> List relevant commits as following: >> there are one more related commit shown below: qingliang.li@mediatek.com 2024-03-01 Fixes: e7a7681c8596 ("PM: sleep: wakeirq: fix wake irq warning in system suspend") >> johan+linaro@kernel.org 2023-07-13 >> Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") >> >> tony@atomide.com 2018-02-09 >> Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") >> >> The former commit fixes the later. > > I do not understand this series of commits, what exactly are you trying > to show here? > there are total 3 history commits mentioned, and the 1st fixes the 2nd which in turn fixes the 3rd, and all these commits involves "unbalanced wake IRQ enablement" is the issue "unbalanced wake IRQ enablement" relevant to this weird logic which looks wrong ? > confused, > > greg k-h
On Sat, Sep 28, 2024 at 02:26:27AM -0700, Zijun Hu wrote: > IT is a serious logical error for dev_pm_disarm_wake_irq() not to disable > the wake irq enabled by dev_pm_arm_wake_irq() You need to explain *why* you believe this is an error. > fixed by simply correcting > the wrong if condition. Your commit message is basically just claims "P is wrong, fix P", which doesn't really explain anything. Writing good commit messages explaining what the problem is is not just required because this is a collaborative project where others need to understand your reasoning, but it also forces you as the author to think through your changes, which can often prevent broken patches from being submitted in the first place. > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > List relevant commits as following: > > johan+linaro@kernel.org 2023-07-13 > Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") > > tony@atomide.com 2018-02-09 > Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") > > The former commit fixes the later. These references are relevant, but you need to include them in your commit messages (above ---) and explain why. > --- > drivers/base/power/wakeirq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c > index 5a5a9e978e85..8b15f9a0e8f9 100644 > --- a/drivers/base/power/wakeirq.c > +++ b/drivers/base/power/wakeirq.c > @@ -356,7 +356,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq) > disable_irq_wake(wirq->irq); > > if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED && > - !(wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) > + (wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) > disable_irq_nosync(wirq->irq); I think the current code works as intended. > } > } Johan
On 2024/9/30 21:07, Johan Hovold wrote: > On Sat, Sep 28, 2024 at 02:26:27AM -0700, Zijun Hu wrote: >> IT is a serious logical error for dev_pm_disarm_wake_irq() not to disable >> the wake irq enabled by dev_pm_arm_wake_irq() > > You need to explain *why* you believe this is an error. > thank you for code review. sorry to give reply late due to travel. by convention, dev_pm_disarm_wake_irq() needs to undo the jobs done by dev_pm_arm_wake_irq(). but actually it does not do that. >> fixed by simply correcting >> the wrong if condition. > > Your commit message is basically just claims "P is wrong, fix P", which > doesn't really explain anything. > > Writing good commit messages explaining what the problem is is not just > required because this is a collaborative project where others need to > understand your reasoning, but it also forces you as the author to think > through your changes, which can often prevent broken patches from being > submitted in the first place. > thanks for these good suggestions and will follow it for further patches (^^)(^^) >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> List relevant commits as following: >> >> johan+linaro@kernel.org 2023-07-13 >> Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") >> >> tony@atomide.com 2018-02-09 >> Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") >> >> The former commit fixes the later. > > These references are relevant, but you need to include them in your > commit messages (above ---) and explain why. > got it, thank you >> --- >> drivers/base/power/wakeirq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c >> index 5a5a9e978e85..8b15f9a0e8f9 100644 >> --- a/drivers/base/power/wakeirq.c >> +++ b/drivers/base/power/wakeirq.c >> @@ -356,7 +356,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq) >> disable_irq_wake(wirq->irq); >> >> if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED && >> - !(wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) >> + (wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) >> disable_irq_nosync(wirq->irq); > > I think the current code works as intended. > thank you for this point and let me do more investigation. (^^) >> } >> } > > Johan
diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c index 5a5a9e978e85..8b15f9a0e8f9 100644 --- a/drivers/base/power/wakeirq.c +++ b/drivers/base/power/wakeirq.c @@ -356,7 +356,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq) disable_irq_wake(wirq->irq); if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED && - !(wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) + (wirq->status & WAKE_IRQ_DEDICATED_ENABLED)) disable_irq_nosync(wirq->irq); } }
IT is a serious logical error for dev_pm_disarm_wake_irq() not to disable the wake irq enabled by dev_pm_arm_wake_irq(), fixed by simply correcting the wrong if condition. Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> --- List relevant commits as following: johan+linaro@kernel.org 2023-07-13 Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming") tony@atomide.com 2018-02-09 Commit: 69728051f5bf ("PM / wakeirq: Fix unbalanced IRQ enable for wakeirq") The former commit fixes the later. --- drivers/base/power/wakeirq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: eb46cb321f1f3f3102f4ad3d61dd5c8c06cdbf17 change-id: 20240928-fix_wakeirq-276ff826970e Best regards,