[PATCH] pinctrl: amd: Clear interrupt enable on spurious IRQ and skip unhandled EOI

[PATCH] pinctrl: amd: Clear interrupt enable on spurious IRQ and skip unhandled EOI

[Munique Feitoza]

When do_amd_gpio_irq_handler() detects a spurious interrupt on a pin
that is not configured as an IRQ line (!gpiochip_line_is_irq()), it
currently only clears BIT(INTERRUPT_MASK_OFF) while leaving
BIT(INTERRUPT_ENABLE_OFF) set. Unlike amd_gpio_irq_disable(), leaving
INTERRUPT_ENABLE_OFF enabled allows the pin status bit to keep
re-asserting in WAKE_INT_STATUS_REG0/1. Clear both INTERRUPT_MASK_OFF
and INTERRUPT_ENABLE_OFF when disabling a spurious GPIO IRQ on both
main and RGPIO banks.

Additionally, if no GPIO interrupt was handled (ret == false), return
early before acquiring gpio_dev->lock and writing EOI_MASK to
WAKE_INT_MASTER_REG. This avoids unnecessary MMIO accesses and spinlock
contention when the IRQ line is shared or asserted by an unhandled
spurious interrupt storm.

Signed-off-by: Munique Feitoza <muniquefeitoz4@gmail.com>
---
 drivers/pinctrl/pinctrl-amd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9fa752f55..428440949 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -671,7 +671,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 			regval = readl(amd_gpio_pin_reg(gpio_dev, irqnr + i));
 			if (!gpiochip_line_is_irq(gc, irqnr + i)) {
-				regval &= ~BIT(INTERRUPT_MASK_OFF);
+				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
+					    BIT(INTERRUPT_ENABLE_OFF));
 				dev_dbg(&gpio_dev->pdev->dev,
 					"Disabling spurious GPIO IRQ %d\n",
 					irqnr + i);
@@ -722,7 +723,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 			regval = readl(amd_gpio_pin_reg(gpio_dev, pin));
 			if (!gpiochip_line_is_irq(gc, pin)) {
-				regval &= ~BIT(INTERRUPT_MASK_OFF);
+				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
+					    BIT(INTERRUPT_ENABLE_OFF));
 				dev_dbg(&gpio_dev->pdev->dev,
 					"Disabling spurious GPIO IRQ %d\n",
 					pin);
@@ -735,7 +737,7 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 	}
 
 	/* did not cause wake on resume context for shared IRQ */
-	if (irq < 0)
+	if (irq < 0 || !ret)
 		return false;
 
 	/* Signal EOI to the GPIO unit */
-- 
2.56.0

Re: [PATCH] pinctrl: amd: Clear interrupt enable on spurious IRQ and skip unhandled EOI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: amd: missing EOI in do_amd_gpio_irq_handler() on spurious IRQ
--

commit df92227f2b1cea5ec26fd7063440df3f2e4656a4
Author: Munique Feitoza <muniquefeitoz4@gmail.com>

pinctrl: amd: Clear interrupt enable on spurious IRQ and skip unhandled EOI

This commit clears BIT(INTERRUPT_ENABLE_OFF) alongside BIT(INTERRUPT_MASK_OFF)
when disabling a spurious GPIO IRQ in do_amd_gpio_irq_handler(), and returns
early before writing EOI_MASK to WAKE_INT_MASTER_REG when ret is false.

> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 9fa752f552d43..42844094916a7 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -671,7 +671,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
>  			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>  			regval = readl(amd_gpio_pin_reg(gpio_dev, irqnr + i));
>  			if (!gpiochip_line_is_irq(gc, irqnr + i)) {
> -				regval &= ~BIT(INTERRUPT_MASK_OFF);
> +				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
> +					    BIT(INTERRUPT_ENABLE_OFF));
>  				dev_dbg(&gpio_dev->pdev->dev,
>  					"Disabling spurious GPIO IRQ %d\n",
>  					irqnr + i);
> @@ -722,7 +723,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
>  			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>  			regval = readl(amd_gpio_pin_reg(gpio_dev, pin));
>  			if (!gpiochip_line_is_irq(gc, pin)) {
> -				regval &= ~BIT(INTERRUPT_MASK_OFF);
> +				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
> +					    BIT(INTERRUPT_ENABLE_OFF));
>  				dev_dbg(&gpio_dev->pdev->dev,
>  					"Disabling spurious GPIO IRQ %d\n",
>  					pin);
> @@ -735,7 +737,7 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
>  	}
>  
>  	/* did not cause wake on resume context for shared IRQ */
> -	if (irq < 0)
> +	if (irq < 0 || !ret)
>  		return false;

[Severity: High]
Does skipping the EOI write when !ret is true leave the AMD GPIO controller
with an unacknowledged interrupt after a spurious GPIO IRQ?

When an unmasked pin raises an interrupt while gpiochip_line_is_irq() is false
(for instance, at boot before a client driver such as i2c-hid probes as in
commit d21b8adbd475 ("pinctrl: amd: Disable and mask interrupts on probe"), or
after suspend/resume when firmware leaves an unused pin unmasked as in commit
0cf9e48ff22e ("pinctrl: amd: Disable and mask interrupts on resume")),
do_amd_gpio_irq_handler() clears and disables the pin interrupt in the
!gpiochip_line_is_irq() branch on the main or RGPIO bank without setting
ret = true:

drivers/pinctrl/pinctrl-amd.c:do_amd_gpio_irq_handler() {
	...
			if (!gpiochip_line_is_irq(gc, irqnr + i)) {
				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
					    BIT(INTERRUPT_ENABLE_OFF));
				dev_dbg(&gpio_dev->pdev->dev,
					"Disabling spurious GPIO IRQ %d\n",
					irqnr + i);
			} else {
				ret = true;
			}
			writel(regval, amd_gpio_pin_reg(gpio_dev, irqnr + i));
	...
}

If no other registered GPIO IRQ is active during the same invocation, ret
remains false even though the AMD GPIO hardware asserted the interrupt line.

Returning early at if (irq < 0 || !ret) skips writing EOI_MASK to
WAKE_INT_MASTER_REG:

	/* did not cause wake on resume context for shared IRQ */
	if (irq < 0 || !ret)
		return false;

	/* Signal EOI to the GPIO unit */
	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
	regval = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
	regval |= EOI_MASK;
	writel(regval, gpio_dev->base + WAKE_INT_MASTER_REG);
	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);

Because the flow handlers used by amd_gpio_irqchip (handle_edge_irq,
handle_level_irq, and handle_simple_irq) do not call .irq_eoi,
do_amd_gpio_irq_handler() is the only place that signals EOI to the GPIO
unit. Wouldn't skipping EOI_MASK after servicing a spurious GPIO interrupt
prevent the controller from delivering subsequent valid GPIO interrupts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030195025.5153-1-muniquefeitoz4@gmail.com?part=1

[PATCH v2] pinctrl: amd: Clear interrupt enable bit on spurious GPIO IRQ

[Munique Feitoza]

When do_amd_gpio_irq_handler() detects a spurious interrupt on a pin
that is not configured as an IRQ line (!gpiochip_line_is_irq()), it
currently only clears BIT(INTERRUPT_MASK_OFF) while leaving
BIT(INTERRUPT_ENABLE_OFF) set. Unlike amd_gpio_irq_disable(), leaving
INTERRUPT_ENABLE_OFF enabled allows the pin interrupt status bit to
keep asserting in WAKE_INT_STATUS_REG0/1.

Clear both INTERRUPT_MASK_OFF and INTERRUPT_ENABLE_OFF when disabling
a spurious GPIO IRQ on both the main and RGPIO banks.

Signed-off-by: Munique Feitoza <muniquefeitoz4@gmail.com>
---
Changes in v2:
- Drop the early return on !ret before writing EOI_MASK to
  WAKE_INT_MASTER_REG so that spurious GPIO interrupts disabled in the
  !gpiochip_line_is_irq() path still receive an EOI signal on the
  controller.

 drivers/pinctrl/pinctrl-amd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9fa752f55..92a08f240 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -671,7 +671,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 			regval = readl(amd_gpio_pin_reg(gpio_dev, irqnr + i));
 			if (!gpiochip_line_is_irq(gc, irqnr + i)) {
-				regval &= ~BIT(INTERRUPT_MASK_OFF);
+				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
+					    BIT(INTERRUPT_ENABLE_OFF));
 				dev_dbg(&gpio_dev->pdev->dev,
 					"Disabling spurious GPIO IRQ %d\n",
 					irqnr + i);
@@ -722,7 +723,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 			regval = readl(amd_gpio_pin_reg(gpio_dev, pin));
 			if (!gpiochip_line_is_irq(gc, pin)) {
-				regval &= ~BIT(INTERRUPT_MASK_OFF);
+				regval &= ~(BIT(INTERRUPT_MASK_OFF) |
+					    BIT(INTERRUPT_ENABLE_OFF));
 				dev_dbg(&gpio_dev->pdev->dev,
 					"Disabling spurious GPIO IRQ %d\n",
 					pin);
-- 
2.56.0