All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eliav Farber <farbere@amazon.com>
To: <linux@armlinux.org.uk>, <catalin.marinas@arm.com>,
	<will@kernel.org>, <mpe@ellerman.id.au>, <npiggin@gmail.com>,
	<christophe.leroy@csgroup.eu>, <naveen@kernel.org>,
	<maddy@linux.ibm.com>, <paul.walmsley@sifive.com>,
	<palmer@dabbelt.com>, <aou@eecs.berkeley.edu>,
	<tglx@linutronix.de>, <akpm@linux-foundation.org>,
	<bhe@redhat.com>, <farbere@amazon.com>, <hbathini@linux.ibm.com>,
	<adityag@linux.ibm.com>, <songshuaishuai@tinylab.org>,
	<takakura@valinux.co.jp>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
	<linux-riscv@lists.infradead.org>
Cc: <jonnyc@amazon.com>
Subject: [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking
Date: Wed, 27 Nov 2024 15:22:36 +0000	[thread overview]
Message-ID: <20241127152236.26122-1-farbere@amazon.com> (raw)

During machine kexec, the function machine_kexec_mask_interrupts() is
responsible for disabling or masking all interrupts. While the
irq_disable hook ensures that an already-disabled IRQ is not disabled
again, the current implementation unconditionally invokes the irq_mask()
function for every interrupt descriptor, even when the interrupt is
already masked.

A specific issue was observed in the crash kernel flow after unbinding a
device (prior to kexec) that used a GPIO as an IRQ source. The warning
was triggered by the gpiochip_disable_irq() function, which attempted to
clear the FLAG_IRQ_IS_ENABLED flag when FLAG_USED_AS_IRQ was not set:

```
void gpiochip_disable_irq(struct gpio_chip *gc, unsigned int offset)
{
	struct gpio_desc *desc = gpiochip_get_desc(gc, offset);

	if (!IS_ERR(desc) &&
	    !WARN_ON(!test_bit(FLAG_USED_AS_IRQ, &desc->flags)))
		clear_bit(FLAG_IRQ_IS_ENABLED, &desc->flags);
}
```

The issue emerged after commit a8173820f441 ("gpio: gpiolib: Allow GPIO
IRQs to lazy disable"), which introduced lazy disablement for GPIO IRQs
by replacing disable/enable hooks with mask/unmask hooks in some cases.
While irq_disable guarded against redundant operations, the unguarded
irq_mask in machine_kexec_mask_interrupts() led to warnings when invoked
on already-masked IRQs.

When a GPIO-IRQ-using driver is unbound, the IRQ is released, invoking
__irq_disable() and irq_state_set_masked(). A subsequent call to
machine_kexec_mask_interrupts() reinvoked chip->irq_mask(), leading to a
call chain that included gpiochip_irq_mask() and gpiochip_disable_irq().
Because FLAG_USED_AS_IRQ was cleared earlier, a warning was printed.

This patch replaces the direct invocation of the irq_mask() and
irq_disable() hooks with simplified code that leverages the
irq_disable() kernel infrastructure. This higher-level function checks
the interrupt's state to prevent redundant operations. Additionally, the
IRQ_DISABLE_UNLAZY status flag is set to ensure that, for interrupt
chips lacking an irq_disable callback, the disable operation is handled
using the lazy approach.

As part of this change, the irq_disable() declaration was moved from
kernel/irq/internals.h to include/linux/irq.h to make it accessible
outside the kernel/irq/ directory, as the former can only be included
within that directory.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V1 -> V2:
- Implement an alternative solution by utilizing the kernel's
  irq_disable() infrastructure.
- Apply the fix to additional architectures, including ARM, PowerPC,
  and RISC-V.
---
 arch/arm/kernel/machine_kexec.c   | 7 ++-----
 arch/arm64/kernel/machine_kexec.c | 7 ++-----
 arch/powerpc/kexec/core.c         | 7 ++-----
 arch/riscv/kernel/machine_kexec.c | 7 ++-----
 include/linux/irq.h               | 2 ++
 kernel/irq/internals.h            | 1 -
 6 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 80ceb5bd2680..54d0bd1bd449 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -142,11 +142,8 @@ static void machine_kexec_mask_interrupts(void)
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 82e2203d86a3..9b48d952df3e 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -230,11 +230,8 @@ static void machine_kexec_mask_interrupts(void)
 		    chip->irq_eoi)
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/powerpc/kexec/core.c b/arch/powerpc/kexec/core.c
index b8333a49ea5d..3489e50f5017 100644
--- a/arch/powerpc/kexec/core.c
+++ b/arch/powerpc/kexec/core.c
@@ -36,11 +36,8 @@ void machine_kexec_mask_interrupts(void) {
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c
index 3c830a6f7ef4..a9df80e0602c 100644
--- a/arch/riscv/kernel/machine_kexec.c
+++ b/arch/riscv/kernel/machine_kexec.c
@@ -129,11 +129,8 @@ static void machine_kexec_mask_interrupts(void)
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fa711f80957b..176a7f671409 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -600,6 +600,8 @@ enum {
 
 #define IRQ_DEFAULT_INIT_FLAGS	ARCH_IRQ_INIT_FLAGS
 
+extern void irq_disable(struct irq_desc *desc);
+
 struct irqaction;
 extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
 extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index fe0272cd84a5..d9104d2b26b4 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -91,7 +91,6 @@ extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
 extern void irq_shutdown(struct irq_desc *desc);
 extern void irq_shutdown_and_deactivate(struct irq_desc *desc);
 extern void irq_enable(struct irq_desc *desc);
-extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
 extern void mask_irq(struct irq_desc *desc);
-- 
2.40.1



WARNING: multiple messages have this Message-ID (diff)
From: Eliav Farber <farbere@amazon.com>
To: <linux@armlinux.org.uk>, <catalin.marinas@arm.com>,
	<will@kernel.org>, <mpe@ellerman.id.au>, <npiggin@gmail.com>,
	<christophe.leroy@csgroup.eu>, <naveen@kernel.org>,
	<maddy@linux.ibm.com>, <paul.walmsley@sifive.com>,
	<palmer@dabbelt.com>, <aou@eecs.berkeley.edu>,
	<tglx@linutronix.de>, <akpm@linux-foundation.org>,
	<bhe@redhat.com>, <farbere@amazon.com>, <hbathini@linux.ibm.com>,
	<adityag@linux.ibm.com>, <songshuaishuai@tinylab.org>,
	<takakura@valinux.co.jp>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
	<linux-riscv@lists.infradead.org>
Cc: <jonnyc@amazon.com>
Subject: [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking
Date: Wed, 27 Nov 2024 15:22:36 +0000	[thread overview]
Message-ID: <20241127152236.26122-1-farbere@amazon.com> (raw)

During machine kexec, the function machine_kexec_mask_interrupts() is
responsible for disabling or masking all interrupts. While the
irq_disable hook ensures that an already-disabled IRQ is not disabled
again, the current implementation unconditionally invokes the irq_mask()
function for every interrupt descriptor, even when the interrupt is
already masked.

A specific issue was observed in the crash kernel flow after unbinding a
device (prior to kexec) that used a GPIO as an IRQ source. The warning
was triggered by the gpiochip_disable_irq() function, which attempted to
clear the FLAG_IRQ_IS_ENABLED flag when FLAG_USED_AS_IRQ was not set:

```
void gpiochip_disable_irq(struct gpio_chip *gc, unsigned int offset)
{
	struct gpio_desc *desc = gpiochip_get_desc(gc, offset);

	if (!IS_ERR(desc) &&
	    !WARN_ON(!test_bit(FLAG_USED_AS_IRQ, &desc->flags)))
		clear_bit(FLAG_IRQ_IS_ENABLED, &desc->flags);
}
```

The issue emerged after commit a8173820f441 ("gpio: gpiolib: Allow GPIO
IRQs to lazy disable"), which introduced lazy disablement for GPIO IRQs
by replacing disable/enable hooks with mask/unmask hooks in some cases.
While irq_disable guarded against redundant operations, the unguarded
irq_mask in machine_kexec_mask_interrupts() led to warnings when invoked
on already-masked IRQs.

When a GPIO-IRQ-using driver is unbound, the IRQ is released, invoking
__irq_disable() and irq_state_set_masked(). A subsequent call to
machine_kexec_mask_interrupts() reinvoked chip->irq_mask(), leading to a
call chain that included gpiochip_irq_mask() and gpiochip_disable_irq().
Because FLAG_USED_AS_IRQ was cleared earlier, a warning was printed.

This patch replaces the direct invocation of the irq_mask() and
irq_disable() hooks with simplified code that leverages the
irq_disable() kernel infrastructure. This higher-level function checks
the interrupt's state to prevent redundant operations. Additionally, the
IRQ_DISABLE_UNLAZY status flag is set to ensure that, for interrupt
chips lacking an irq_disable callback, the disable operation is handled
using the lazy approach.

As part of this change, the irq_disable() declaration was moved from
kernel/irq/internals.h to include/linux/irq.h to make it accessible
outside the kernel/irq/ directory, as the former can only be included
within that directory.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V1 -> V2:
- Implement an alternative solution by utilizing the kernel's
  irq_disable() infrastructure.
- Apply the fix to additional architectures, including ARM, PowerPC,
  and RISC-V.
---
 arch/arm/kernel/machine_kexec.c   | 7 ++-----
 arch/arm64/kernel/machine_kexec.c | 7 ++-----
 arch/powerpc/kexec/core.c         | 7 ++-----
 arch/riscv/kernel/machine_kexec.c | 7 ++-----
 include/linux/irq.h               | 2 ++
 kernel/irq/internals.h            | 1 -
 6 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 80ceb5bd2680..54d0bd1bd449 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -142,11 +142,8 @@ static void machine_kexec_mask_interrupts(void)
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 82e2203d86a3..9b48d952df3e 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -230,11 +230,8 @@ static void machine_kexec_mask_interrupts(void)
 		    chip->irq_eoi)
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/powerpc/kexec/core.c b/arch/powerpc/kexec/core.c
index b8333a49ea5d..3489e50f5017 100644
--- a/arch/powerpc/kexec/core.c
+++ b/arch/powerpc/kexec/core.c
@@ -36,11 +36,8 @@ void machine_kexec_mask_interrupts(void) {
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c
index 3c830a6f7ef4..a9df80e0602c 100644
--- a/arch/riscv/kernel/machine_kexec.c
+++ b/arch/riscv/kernel/machine_kexec.c
@@ -129,11 +129,8 @@ static void machine_kexec_mask_interrupts(void)
 		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
-			chip->irq_mask(&desc->irq_data);
-
-		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
-			chip->irq_disable(&desc->irq_data);
+		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
+		irq_disable(desc);
 	}
 }
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fa711f80957b..176a7f671409 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -600,6 +600,8 @@ enum {
 
 #define IRQ_DEFAULT_INIT_FLAGS	ARCH_IRQ_INIT_FLAGS
 
+extern void irq_disable(struct irq_desc *desc);
+
 struct irqaction;
 extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
 extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index fe0272cd84a5..d9104d2b26b4 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -91,7 +91,6 @@ extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
 extern void irq_shutdown(struct irq_desc *desc);
 extern void irq_shutdown_and_deactivate(struct irq_desc *desc);
 extern void irq_enable(struct irq_desc *desc);
-extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
 extern void mask_irq(struct irq_desc *desc);
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

             reply	other threads:[~2024-11-27 15:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-27 15:22 Eliav Farber [this message]
2024-11-27 15:22 ` [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking Eliav Farber
2024-11-28 10:39 ` Thomas Gleixner
2024-11-28 10:39   ` Thomas Gleixner
2024-11-28 10:43 ` Thomas Gleixner
2024-11-28 10:43   ` Thomas Gleixner
  -- strict thread matches above, loose matches on Subject: below --
2024-11-28 20:07 Farber, Eliav
2024-11-28 20:07 ` Farber, Eliav

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241127152236.26122-1-farbere@amazon.com \
    --to=farbere@amazon.com \
    --cc=adityag@linux.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=aou@eecs.berkeley.edu \
    --cc=bhe@redhat.com \
    --cc=catalin.marinas@arm.com \
    --cc=christophe.leroy@csgroup.eu \
    --cc=hbathini@linux.ibm.com \
    --cc=jonnyc@amazon.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=linux@armlinux.org.uk \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=maddy@linux.ibm.com \
    --cc=mpe@ellerman.id.au \
    --cc=naveen@kernel.org \
    --cc=npiggin@gmail.com \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=songshuaishuai@tinylab.org \
    --cc=takakura@valinux.co.jp \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.