Received-SPF: pass (google.com: domain of linux-kernel+bounces-84551-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <1390e743-2216-4435-b2ef-7d92a55605b1@ti.com>
Date: Wed, 28 Feb 2024 11:34:23 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] PM: wakeirq: fix wake irq warning in system suspend stage
To: Qingliang Li <qingliang.li@mediatek.com>,
        "Rafael J . Wysocki"
	<rafael@kernel.org>, Pavel Machek <pavel@ucw.cz>,
        Len Brown
	<len.brown@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
        Johan Hovold
	<johan+linaro@kernel.org>,
        Tony Lindgren <tony@atomide.com>, Dhruva Gole
	<d-gole@ti.com>
CC: <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>
References: <20240228020040.25815-1-qingliang.li@mediatek.com>
Content-Language: en-US
From: Dhruva Gole <d-gole@ti.com>
In-Reply-To: <20240228020040.25815-1-qingliang.li@mediatek.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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>

-- 
Thanks and Regards,
Dhruva Gole