Re: [PATCH v8 15/35] pinctrl: airoha: fix getting gpiochip/pinctrl pointers in the IRQ handling code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver incorrectly uses `handle_simple_irq` for all GPIO interrupts and manually clears the interrupt status after the handler runs. This causes a race condition where edge-triggered interrupts that occur during handler execution are permanently lost.
--

--- Patch [15]: [PATCH v8 15/35] pinctrl: airoha: fix getting gpiochip/pinctrl pointers in the IRQ handling code ---
commit 62d50e3b85fb87d9a4b824ca2d986441454c0ea4
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

    pinctrl: airoha: fix getting gpiochip/pinctrl pointers in the IRQ handling code
    
This patch fixes the initialization logic for the gpiochip and pinctrl 
pointers in the IRQ mask, unmask, and type callbacks. It corrects the 
previous logic which assigned a pointer of struct gpio_chip to a variable 
of struct airoha_pinctrl_gpiochip.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 67ee205e2b5b4..3feddec2e29e3 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c

[ ... ]

> @@ -2616,9 +2613,10 @@ static void airoha_irq_mask(struct irq_data *data)
>  
>  static int airoha_irq_type(struct irq_data *data, unsigned int type)
>  {
> -	struct airoha_pinctrl_gpiochip *gpiochip;
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(gc);
> +	struct airoha_pinctrl_gpiochip *gpiochip = &pinctrl->gpiochip;
>  
> -	gpiochip = irq_data_get_irq_chip_data(data);
>  	if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
>  		return -EINVAL;
>  

[Severity: High]
This is a pre-existing issue, but is it possible that edge-triggered interrupts
are being lost due to incorrect flow control?

Looking at airoha_irq_type(), it doesn't call irq_set_handler_locked() to 
configure handle_edge_irq or handle_level_irq based on the trigger type:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_type() {
    ...
	gpiochip->irq_type[data->hwirq] = type & IRQ_TYPE_SENSE_MASK;

	return 0;
}

Because of this, airoha_pinctrl_add_gpiochip() forces all interrupts to use
handle_simple_irq:

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

This lacks proper edge/level flow control. When an interrupt fires,
airoha_irq_handler() manually clears the status after generic_handle_irq():

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

If a second edge occurs on the same GPIO while the handler is executing, the
status register is latched again. However, the regmap_write() clears this
newly latched status before it can be handled.

Should airoha_irq_type() set the appropriate IRQ flow handler, and could
the driver rely on the core IRQ framework rather than manually clearing the
status after the handler runs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727174141.664933-1-mikhail.kshevetskiy@iopsys.eu?part=15