Re: [PATCH 1/3] pinctrl: airoha: limit GPIO interrupts to interrupt-capable pins

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The shared parent interrupt is requested before the gpiochip and its IRQ domain are fully registered, creating a race condition that loses hardware interrupts during probe.
- [High] The interrupt handler manually clears the hardware status bit after the child handler completes, creating a race condition that drops edge-triggered interrupts.
- [High] The driver acquires a sleepable spinlock inside IRQ chip callbacks while holding a raw_spinlock_t, violating PREEMPT_RT lock nesting rules.
- [High] The interrupt handler loops over banks bounded by `num_irq_pins` but fails to mask out unbacked bits within the final bank, neutralizing spurious interrupt mitigation.
--

--- Patch [1]: [PATCH 1/3] pinctrl: airoha: limit GPIO interrupts to interrupt-capable pins ---
commit 540e2cd0ad7d06a01aca006b216ac539740ee038
Author: Ahmed Naseef <[email protected]>
pinctrl: airoha: limit GPIO interrupts to interrupt-capable pins

This patch limits GPIO interrupts to interrupt-capable pins by introducing a
num_irq_pins field to the SoC match data. It restricts the interrupt callbacks
and the loop in the interrupt handler, and feeds the count to gpiolib to
prevent requesting invalid interrupts.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index f505a3f69c5d1..52a768c859b71 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -213,7 +213,7 @@ static void airoha_irq_unmask(struct irq_data *data)
>  	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
>  	u32 val = BIT(2 * offset);
>  
> -	if (WARN_ON_ONCE(data->hwirq >= AIROHA_NUM_PINS))
> +	if (WARN_ON_ONCE(data->hwirq >= pinctrl->num_irq_pins))
>  		return;
>  
>  	gpiochip_enable_irq(gc, irqd_to_hwirq(data));
>  	switch (irqd_get_trigger_type(data)) {
>  	case IRQ_TYPE_LEVEL_LOW:
>  		val = val << 1;
>  		fallthrough;
>  	case IRQ_TYPE_LEVEL_HIGH:
>  		regmap_update_bits(pinctrl->regmap, gpio_regs->level[index],
>  				   mask, val);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
regmap_update_bits() here cause a lock nesting violation on PREEMPT_RT
kernels?

The IRQ chip callbacks like airoha_irq_unmask() are invoked by the IRQ core
with the descriptor lock (desc->lock) held, which is a raw_spinlock_t.

The syscon regmap (pinctrl->regmap) uses a standard spinlock_t that becomes
a sleepable RT-mutex on PREEMPT_RT. Acquiring a sleepable lock under a raw
spinlock leads to a kernel panic.

[ ... ]

> @@ -304,9 +307,11 @@ static irqreturn_t airoha_irq_handler(int irq, void *data)
>  {
>  	struct airoha_pinctrl *pinctrl = data;
>  	bool handled = false;
> +	unsigned int nbanks;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(irq_status_regs); i++) {
> +	nbanks = DIV_ROUND_UP(pinctrl->num_irq_pins, AIROHA_PIN_BANK_SIZE);
> +	for (i = 0; i < nbanks; i++) {
>  		struct gpio_irq_chip *girq = &pinctrl->gpiochip.irq;
>  		u32 regmap;
>  		unsigned long status;
>  		int irq;
>  
>  		if (regmap_read(pinctrl->regmap, pinctrl->gpio_regs->status[i],
>  				&regmap))
>  			continue;
>  
>  		status = regmap;
>  		for_each_set_bit(irq, &status, AIROHA_PIN_BANK_SIZE) {
>  			u32 offset = irq + i * AIROHA_PIN_BANK_SIZE;
>  
>  			generic_handle_domain_irq(girq->domain, offset);
>  			regmap_write(pinctrl->regmap,
>  				     pinctrl->gpio_regs->status[i], BIT(irq));

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the hardware
status bit after the child handler completes cause edge-triggered interrupts
to be dropped?

In airoha_irq_handler(), after generic_handle_domain_irq() returns, the
handler manually clears the hardware status bit using regmap_write().
However, the IRQ core (such as handle_edge_irq) already invokes the chip's
irq_ack callback to clear this bit before executing the child handler.

By clearing the bit a second time after the child handler returns, any new
edge-triggered interrupts that arrived while the child handler was executing
would be silently erased.

>  		}
>  		handled |= !!status;

[Severity: High]
This is a pre-existing issue, but does failing to mask out unbacked bits in
the final bank neutralize the spurious interrupt mitigation?

While nbanks limits the outer loop, on EN7528 num_irq_pins is 16, so nbanks
is 1. The loop reads status[0] and for_each_set_bit() iterates over all
32 bits.

If any unbacked hardware bit (16-31) reads as 1, generic_handle_domain_irq()
is called (and safely returns -EINVAL), but handled incorrectly evaluates to
true. This would cause IRQ_HANDLED to be returned for spurious interrupts on
the shared line, defeating the kernel's spurious interrupt storm mitigation.

[ ... ]

> @@ -362,6 +383,7 @@ static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
>  
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_bad_irq;
> +	girq->init_valid_mask = airoha_gpio_init_valid_mask;
>  	gpio_irq_chip_set_chip(girq, &airoha_gpio_irq_chip);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
>  
>  	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
>  				dev_name(dev), pinctrl);
>  	if (err)
>  		return err;
>  
>  	return devm_gpiochip_add_data(dev, gc, pinctrl);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does requesting the shared
parent interrupt before registering the gpiochip cause a race condition
during probe?

The driver requests the shared parent IRQ via devm_request_irq() before
calling devm_gpiochip_add_data().

If the shared IRQ line is asserted by another device before
devm_gpiochip_add_data() completes, airoha_irq_handler() will run, read the
hardware status, and call generic_handle_domain_irq() with an uninitialized
girq->domain. The core lookup fails, but the handler proceeds to clear the
hardware status and returns IRQ_HANDLED, dropping the interrupt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1