Re: [PATCH v11 19/37] 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 immutable IRQ chip `airoha_gpio_irq_chip` is missing required `gpiochip_disable_irq()` and `gpiochip_enable_irq()` calls in its `.irq_mask` and `.irq_unmask` callbacks.
- [High] The driver hardcodes `handle_simple_irq` for all GPIO interrupts and incorrectly clears the hardware interrupt status after the handler runs, breaking edge and level interrupt semantics.
- [High] The IRQ handler is requested before the gpiochip is fully initialized, creating a race condition where a shared interrupt can cause a null domain dereference.
- [High] The IRQ chip callbacks use a syscon regmap, which relies on a sleepable spinlock on PREEMPT_RT, while being called under a raw spinlock.
--

--- Patch [19]: [PATCH v11 19/37] pinctrl: airoha: add missed IRQ resource helpers ---
commit b705513e6d14523fbe92901715e6323201dc334f
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: add missed IRQ resource helpers

Without hooking .irq_request_resources, gpiolib cannot set
GPIOD_FLAG_USED_AS_IRQ. This breaks pin direction locking and can allow
userspace or another driver to reconfigure an active IRQ pin as an output

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 9b880fd20a1a0..f3078f9b14a76 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,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  

[Severity: High]
This is a pre-existing issue, but missing required gpiochip_disable_irq() and
gpiochip_enable_irq() calls in airoha_irq_mask() and airoha_irq_unmask() means
the GPIOD_FLAG_IRQ_IS_ENABLED flag is never set when an IRQ is unmasked.

Without these calls:

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

Can userspace or another driver incorrectly reconfigure the active IRQ pin as
an output because gpiolib believes the IRQ is masked or disabled?

[Severity: High]
This is a pre-existing issue, but does using syscon regmap inside the IRQ chip
callbacks cause a lock context violation on PREEMPT_RT?

The callbacks are invoked under desc->lock which is a raw spinlock:

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

However, the regmap is retrieved from the syscon parent in
airoha_pinctrl_probe(), which defaults to standard (sleepable) spinlocks.
Could this result in sleeping in atomic context?

>  static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to unconditionally
use handle_simple_irq for all pins and clear the status bit after the handler
finishes?

In airoha_pinctrl_add_gpiochip(), handle_simple_irq is assigned to all pins:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_add_gpiochip() {
...
	girq->default_type = IRQ_TYPE_NONE;
	girq->handler = handle_simple_irq;
...
}

However, in airoha_irq_handler(), the hardware status bit is cleared after
generic_handle_irq() returns:

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

For edge-triggered interrupts, clearing the hardware status bit after the
handler returns means that any new edge occurring during handler execution
might be permanently lost. For level-triggered interrupts, handle_simple_irq
does not mask the interrupt line during execution. Can this lead to dropped
interrupts or hard interrupt storms if threaded handlers are used?

[Severity: High]
This is a pre-existing issue, but is there a race condition where a shared
interrupt can cause a NULL domain dereference?

The IRQ is requested with IRQF_SHARED before the gpiochip and its irqdomain
are added:

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) {
		dev_err(dev, "error requesting irq %d: %d\n", irq, err);
		return err;
	}

	return devm_gpiochip_add_data(dev, gc, pinctrl);
}

If the shared IRQ line is asserted, the interrupt handler runs immediately.
Will it dereference girq->domain which is still NULL in airoha_irq_handler(),
leading to a kernel crash?

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