linux-kernel - Re: [PATCH RFC] PM: sleep: wakeirq: Fix a serious logical error in dev_pm_disarm_wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZvqigTC7RvngLpme@hovoldconsulting.com>
Date: Mon, 30 Sep 2024 15:07:13 +0200
From: Johan Hovold <johan@...nel.org>
To: Zijun Hu <quic_zijuhu@...cinc.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Johan Hovold <johan+linaro@...nel.org>,
	Tony Lindgren <tony@...mide.com>, Zijun Hu <zijun_hu@...oud.com>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] PM: sleep: wakeirq: Fix a serious logical error in
 dev_pm_disarm_wake_irq()

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@...cinc.com>
> ---
> List relevant commits as following:
> 
> johan+linaro@...nel.org  2023-07-13
> Commit: 8527beb12087 ("PM: sleep: wakeirq: fix wake irq arming")
> 
> tony@...mide.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