Re: [PATCH] genirq: Disable suspended irqs when restoring affinity
From: Thomas Gleixner
Date: Mon Apr 22 2024 - 16:37:04 EST
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@xxxxxxxxxxxx>
> ---
> 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;