Re: [PATCH v9 19/36] pinctrl: airoha: fix IRQ mask/unmask code

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

New issues:
- [High] gpiochip_disable_irq() and gpiochip_enable_irq() are incorrectly placed in .irq_mask and .irq_unmask, removing output protection during level-triggered IRQ handling.

Pre-existing issues:
- [High] The driver unconditionally uses handle_simple_irq for all interrupt types, leading to hard-IRQ storms for level-triggered interrupts.
- [High] Edge-triggered interrupts can be lost because the hardware status register is cleared after executing the child ISR.
- [High] regmap_update_bits/clear_bits is called within raw spinlock context without a raw spinlock-enabled regmap.
- [High] Use-After-Free and NULL pointer dereference of irqdomain due to incorrect devm registration order.
--

--- Patch [19]: [PATCH v9 19/36] pinctrl: airoha: fix IRQ mask/unmask code ---
commit a13f62731221613018aaaa7a5f563131118288c9
Author: Mikhail Kshevetskiy <[email protected]>
Subject: pinctrl: airoha: fix IRQ mask/unmask code

This commit adds gpiochip_enable_irq() and gpiochip_disable_irq() calls
to airoha_irq_unmask() and airoha_irq_mask() respectively. This ensures
gpiolib tracks the interrupt enabled state and prevents
gpiod_direction_output() from mistakenly assuming the pin is not actively
used as an interrupt.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 261180b656392..42ae588e6343b 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c

[ ... ]

> @@ -2629,6 +2630,7 @@ static void airoha_irq_mask(struct irq_data *data)
>  
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->level[index], mask);
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->edge[index], mask);

[Severity: High]
This is a pre-existing issue, but does using regmap_clear_bits() here cause
lock nesting violations on real-time systems? 

The interrupt core calls airoha_irq_mask() with a raw spinlock held. Because 
pinctrl->regmap is a syscon regmap, it does not use raw spinlocks by default.
This means it becomes a sleeping lock on real-time kernels.

Acquiring a sleeping lock while holding a raw spinlock can lead to a panic.

> +	gpiochip_disable_irq(gc, irqd_to_hwirq(data));

[Severity: High]
Does calling gpiochip_disable_irq() here remove output protection during
level-triggered interrupt handling?

When the irq core handles level-triggered interrupts, it temporarily masks
the interrupt by calling airoha_irq_mask() during handler execution. Since
gpiochip_disable_irq() is placed here, the GPIOD_FLAG_IRQ_IS_ENABLED bit is
cleared.

This would allow a concurrent call to gpiod_direction_output() to succeed
and mistakenly reconfigure the active interrupt pin as an output. Should
these calls be placed in dedicated .irq_enable and .irq_disable callbacks
instead?

>  }
>  
>  static int airoha_irq_type(struct irq_data *data, unsigned int type)

[Severity: High]
This is a pre-existing issue, but does the driver fail to handle
level-triggered interrupts properly?

In airoha_pinctrl_add_gpiochip(), the handler is set to handle_simple_irq:

    girq->handler = handle_simple_irq;

However, airoha_irq_type() doesn't call irq_set_handler_locked() to upgrade
the handler to handle_level_irq for level triggers. Since handle_simple_irq
does not mask the interrupt before executing a threaded handler, will the
unhandled hardware interrupt loop infinitely and cause an interrupt storm?

[Severity: High]
This is a pre-existing issue, but can edge-triggered interrupts be lost due
to the status clearing order?

In airoha_irq_handler(), the code clears the hardware status register after
executing the child interrupt handler:

    generic_handle_irq(irq_find_mapping(girq->domain, offset));
    regmap_write(pinctrl->regmap, pinctrl->gpiochip.status[i], BIT(irq));

If a new edge interrupt occurs for the same pin while the child handler is
running, the hardware latches the new edge. Will the subsequent
write-1-to-clear operation unconditionally clear the status bit and erase the
newly latched edge without it being processed?

[Severity: High]
This is a pre-existing issue, but is there a risk of a use-after-free or
null pointer dereference during teardown or early probe?

In airoha_pinctrl_add_gpiochip(), the shared interrupt is requested before
the gpiochip is added:

    err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
                           dev_name(dev), pinctrl);

During driver removal, devm_gpiochip_add_data() unwinds first and destroys
the irq domain. The shared interrupt remains active until
devm_request_irq() unwinds.

If an interrupt fires during this window, airoha_irq_handler() will
dereference the freed girq->domain.

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