2024-04-18 07:47:29

by David Stevens

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

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 <[email protected]>
---
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

2024-04-22 20:37:04

by Thomas Gleixner

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

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 <[email protected]>
> ---
> 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;

2024-04-23 02:06:42

by David Stevens

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

On Tue, Apr 23, 2024 at 5:36 AM Thomas Gleixner <[email protected]> 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 <[email protected]>
> > ---
> > 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

2024-04-23 13:23:03

by Thomas Gleixner

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

On Tue, Apr 23 2024 at 11:06, David Stevens wrote:
> On Tue, Apr 23, 2024 at 5:36 AM Thomas Gleixner <[email protected]> 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.