Skip to contentSkip to conversation

arm64: kexec: Check if IRQ is already masked before masking

PATCH Superseded Eliav Farber 2 participants 3 messages Nov 26, 2024 – Nov 27, 2024 arm64

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: kexec: Check if IRQ is already masked before masking

During machine kexec, the function machine_kexec_mask_interrupts() is responsible for masking all interrupts. However, the current implementation unconditionally calls the irq_mask() function for each interrupt descriptor, even if the interrupt is already masked.

This commit adds a check to verify if the interrupt is not already masked before calling the irq_mask() function. This change avoids redundant masking operations and potential issues that might arise from attempting to mask an already masked interrupt.

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

This issue began after commit a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy disable"), which replaced IRQ disable/enable hooks with mask/unmask hooks in some cases. The irq_disable hook was protected against disabling an already disabled IRQ, but the irq_mask hook in machine_kexec_mask_interrupts() was not.

When a driver that uses a GPIO-irq is unbound, the corresponding IRQ is released, invoking __irq_disable() and irq_state_set_masked(). Subsequently, machine_kexec_mask_interrupts() attempts to call the chip->irq_mask() function again. This invokes gpiochip_irq_mask() and gpiochip_disable_irq(), and since FLAG_USED_AS_IRQ has already been cleared, this results in a warning being printed.

---
 arch/arm64/kernel/machine_kexec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/machine_kexec.c +1 -1
index 82e2203d86a3..6f56ec676844 100644
@@ -230,7 +230,7 @@ static void machine_kexec_mask_interrupts(void)
 		    chip->irq_eoi)
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
+		if (chip->irq_mask && !irqd_irq_masked(&desc->irq_data))
 			chip->irq_mask(&desc->irq_data);
 
 		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
Signature · 1 line
2.40.1
#1 lore raw

Thanks Catalin for pointing me to this patch.

Quoted 50 lines · Eliav Farber

During machine kexec, the function machine_kexec_mask_interrupts() is responsible for masking all interrupts. However, the current implementation unconditionally calls the irq_mask() function for each interrupt descriptor, even if the interrupt is already masked.

This commit adds a check to verify if the interrupt is not already masked before calling the irq_mask() function. This change avoids redundant masking operations and potential issues that might arise from attempting to mask an already masked interrupt.

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

This issue began after commit a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy disable"), which replaced IRQ disable/enable hooks with mask/unmask hooks in some cases. The irq_disable hook was protected against disabling an already disabled IRQ, but the irq_mask hook in machine_kexec_mask_interrupts() was not.

When a driver that uses a GPIO-irq is unbound, the corresponding IRQ is released, invoking __irq_disable() and irq_state_set_masked(). Subsequently, machine_kexec_mask_interrupts() attempts to call the chip->irq_mask() function again. This invokes gpiochip_irq_mask() and gpiochip_disable_irq(), and since FLAG_USED_AS_IRQ has already been cleared, this results in a warning being printed.

---
 arch/arm64/kernel/machine_kexec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/machine_kexec.c +0 -0
index 82e2203d86a3..6f56ec676844 100644

arch/arm64/kernel/machine_kexec.c static void machine_kexec_mask_interrupts(void) · line 234

Quoted 6 lines
arch/arm64/kernel/machine_kexec.c +1 -1
@@ -230,7 +230,7 @@ static void machine_kexec_mask_interrupts(void)
 		    chip->irq_eoi)
 			chip->irq_eoi(&desc->irq_data);
 
-		if (chip->irq_mask)
+		if (chip->irq_mask && !irqd_irq_masked(&desc->irq_data))
 			chip->irq_mask(&desc->irq_data);

Maybe a slightly better approach would be to simplify this code for something that actually uses the kernel infrastructure:

arch/arm64/kernel/machine_kexec.c +2 -5
index 82e2203d86a31..9b48d952df3ec 100644
@@ -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);
 	}
 }
 

This is of course untested.

But a *much* better approach would be to have a way to turn the irqchip off altogether and stop this silly "walk 1000s of interrupts for no purpose". Unfortunately, we don't have a good way to do this today.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
#2 lore raw
Quoted 21 lines

Maybe a slightly better approach would be to simplify this code for something that actually uses the kernel infrastructure:

arch/arm64/kernel/machine_kexec.c +2 -5
index 82e2203d86a31..9b48d952df3ec 100644
@@ -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);
         }
  }
 
Quoted 1 line

This is of course untested.

I tested your suggested approach and it works.
I will upload V2 for this change.

Thanks, Eliav