Re: [PATCH RFC] PM: sleep: wakeirq: Fix a serious logical error in dev_pm_disarm_wake_irq()

[Zijun Hu]

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