Received-SPF: pass (google.com: domain of linux-kernel+bounces-153999-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Thomas Gleixner <tglx@linutronix.de>
To: David Stevens <stevensd@chromium.org>
Cc: linux-kernel@vger.kernel.org, David Stevens <stevensd@chromium.org>
Subject: Re: [PATCH] genirq: Disable suspended irqs when restoring affinity
In-Reply-To: <20240418074652.126355-1-stevensd@chromium.org>
References: <20240418074652.126355-1-stevensd@chromium.org>
Date: Mon, 22 Apr 2024 22:36:49 +0200
Message-ID: <87wmopuobi.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Thu, Apr 18 2024 at 16:46, David Stevens wrote:
> In irq_restore_affinity_of_irq(), after initializing an affinity
> managed irq, disable the irq if it should be suspended. This ensures
> that irqs are not delivered to drivers during the noirq phase of
> resuming from S3, after non-boot CPUs are brought back online.
>
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  kernel/irq/cpuhotplug.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
> index 1ed2b1739363..c00132013708 100644
> --- a/kernel/irq/cpuhotplug.c
> +++ b/kernel/irq/cpuhotplug.c
> @@ -197,6 +197,8 @@ static void irq_restore_affinity_of_irq(struct irq_desc *desc, unsigned int cpu)
>  
>  	if (irqd_is_managed_and_shutdown(data)) {
>  		irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
> +		if (desc->istate & IRQS_SUSPENDED)
> +			__disable_irq(desc);

Makes sense. But I rather avoid the whole startup/disable dance in this
case and let resume_device_irqs() deal with it.

Thanks,

        tglx
---
--- a/kernel/irq/cpuhotplug.c
+++ b/kernel/irq/cpuhotplug.c
@@ -195,10 +195,15 @@ static void irq_restore_affinity_of_irq(
 	    !irq_data_get_irq_chip(data) || !cpumask_test_cpu(cpu, affinity))
 		return;
 
-	if (irqd_is_managed_and_shutdown(data)) {
-		irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
+	/*
+	 * Don't restore suspended interrupts here when a system comes back
+	 * from S3. They are reenabled via resume_device_irqs().
+	 */
+	if (desc->istat & IRQS_SUSPENDED)
 		return;
-	}
+
+	if (irqd_is_managed_and_shutdown(data))
+		irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
 
 	/*
 	 * If the interrupt can only be directed to a single target
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -800,10 +800,14 @@ void __enable_irq(struct irq_desc *desc)
 		irq_settings_set_noprobe(desc);
 		/*
 		 * Call irq_startup() not irq_enable() here because the
-		 * interrupt might be marked NOAUTOEN. So irq_startup()
-		 * needs to be invoked when it gets enabled the first
-		 * time. If it was already started up, then irq_startup()
-		 * will invoke irq_enable() under the hood.
+		 * interrupt might be marked NOAUTOEN so irq_startup()
+		 * needs to be invoked when it gets enabled the first time.
+		 * This is also required when __enable_irq() is invoked for
+		 * a managed and shutdown interrupt from the S3 resume
+		 * path.
+		 *
+		 * If it was already started up, then irq_startup() will
+		 * invoke irq_enable() under the hood.
 		 */
 		irq_startup(desc, IRQ_RESEND, IRQ_START_FORCE);
 		break;