Re: [PATCH v9 18/36] pinctrl: airoha: add missed IRQ resource helpers

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

Pre-existing issues:
- [High] The irqchip is marked as `IRQCHIP_IMMUTABLE`, but its `.irq_mask` and `.irq_unmask` callbacks fail to call `gpiochip_disable_irq()` and `gpiochip_enable_irq()` respectively.
- [High] The IRQ handler acknowledges (ACKs) the interrupt by clearing the status register after the generic handler logic has executed, leading to lost edge interrupts.
- [High] The driver requests the parent IRQ before the GPIO chip's IRQ domain is registered, creating a race condition if an interrupt fires immediately.
- [High] The irqchip callbacks `airoha_irq_mask` and `airoha_irq_unmask` call syscon regmap functions which acquire a `spinlock_t`, but they are invoked with the irq descriptor's `raw_spinlock_t` held.
--

--- Patch [18]: [PATCH v9 18/36] pinctrl: airoha: add missed IRQ resource helpers ---
commit 0adbdce0f6e107b50fe1d5a5369eb0d66b97d6e1
Author: Mikhail Kshevetskiy <[email protected]>
Subject: pinctrl: airoha: add missed IRQ resource helpers

This patch adds GPIOCHIP_IRQ_RESOURCE_HELPERS to the airoha_gpio_irq_chip
structure. This ensures .irq_request_resources is hooked up, allowing gpiolib
to properly set GPIOD_FLAG_USED_AS_IRQ. This prevents userspace or other
drivers from incorrectly reconfiguring active IRQ pins as outputs.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 8c0a28eec0a17..261180b656392 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -2689,6 +2689,7 @@ static const struct irq_chip airoha_gpio_irq_chip = {
>  	.irq_mask_ack = airoha_irq_mask,
>  	.irq_set_type = airoha_irq_type,
>  	.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_IMMUTABLE,

[Severity: High]
This is a pre-existing issue, but since this chip is marked as
IRQCHIP_IMMUTABLE, should the airoha_irq_unmask() and airoha_irq_mask()
callbacks call gpiochip_enable_irq() and gpiochip_disable_irq() respectively?

Without these calls, the gpiolib core tracking of FLAG_IRQ_IS_ENABLED can
become desynchronized from the actual hardware state during dynamic unmasking
or masking of GPIO interrupts:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_unmask() {
        ...
        case IRQ_TYPE_LEVEL_HIGH:
                regmap_update_bits(pinctrl->regmap, gpiochip->level[index],
                                   mask, val);
                break;
        ...
}

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_mask() {
        ...
        regmap_clear_bits(pinctrl->regmap, gpiochip->level[index], mask);
        regmap_clear_bits(pinctrl->regmap, gpiochip->edge[index], mask);
}

[Severity: High]
This isn't a bug introduced by this patch, but does the irq_mask and
irq_unmask implementation risk deadlocks on PREEMPT_RT kernels?

The airoha_irq_mask() and airoha_irq_unmask() callbacks invoke syscon regmap
functions (like regmap_update_bits and regmap_clear_bits) which acquire a
spinlock_t (a sleepable rt_mutex on PREEMPT_RT). 

Since these irqchip callbacks are invoked with the irq descriptor's
raw_spinlock_t held, this creates a wait-context hierarchy violation 
(LD_WAIT_SPIN > LD_WAIT_CONFIG).

> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
>  static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,

[Severity: High]
This is a pre-existing issue, but does using handle_simple_irq instead of
handle_edge_irq lead to lost edge interrupts?

In airoha_irq_handler(), the hardware interrupt is acknowledged by clearing
the status register after the generic handler executes:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_handler() {
        ...
                for_each_set_bit(irq, &status, AIROHA_PIN_BANK_SIZE) {
                        ...
                        generic_handle_irq(irq_find_mapping(girq->domain,
                                                            offset));
                        regmap_write(pinctrl->regmap,
                                     pinctrl->gpiochip.status[i], BIT(irq));
                }
        ...
}

If a new edge interrupt triggers on the same GPIO pin while
handle_simple_irq() is executing, the unconditional clear (W1C) afterwards
could wipe the newly latched interrupt.

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
during initialization?

The driver requests the parent IRQ via devm_request_irq() before the GPIO
chip and its IRQ domain are fully initialized by devm_gpiochip_add_data():

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_add_gpiochip() {
        ...
        err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
                                dev_name(dev), pinctrl);
        if (err) {
                ...
        }

        return devm_gpiochip_add_data(dev, gc, pinctrl);
}

If an interrupt triggers immediately after devm_request_irq(), the handler
executes with a NULL girq->domain. This causes irq_find_mapping() to fail
and return 0, potentially handling an invalid IRQ 0 and dropping the interrupt.

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