[PATCH] genirq: Disable suspended irqs when restoring affinity

[PATCH] genirq: Disable suspended irqs when restoring affinity

[David Stevens]

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);
 		return;
 	}
 

base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH] genirq: Disable suspended irqs when restoring affinity

[Thomas Gleixner]

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;

Re: [PATCH] genirq: Disable suspended irqs when restoring affinity

[David Stevens]

On Tue, Apr 23, 2024 at 5:36 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> 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 for taking a look at my patch. Should I send a v2 with your
suggestions, or will you create a patch for your tree yourself?

-David

Re: [PATCH] genirq: Disable suspended irqs when restoring affinity

[Thomas Gleixner]

On Tue, Apr 23 2024 at 11:06, David Stevens wrote:
> On Tue, Apr 23, 2024 at 5:36 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Makes sense. But I rather avoid the whole startup/disable dance in this
>> case and let resume_device_irqs() deal with it.
>
> Thanks for taking a look at my patch. Should I send a v2 with your
> suggestions, or will you create a patch for your tree yourself?

Please send a V2.