Re: [PATCH v11 20/37] pinctrl: airoha: fix IRQ mask/unmask code

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

Pre-existing issues:
- [High] Lost edge-triggered interrupts due to clearing interrupt status after running the handler and using handle_simple_irq.
- [High] Sleeping in atomic context on PREEMPT_RT due to regmap calls within irq_mask/irq_unmask.
--

--- Patch [20]: [PATCH v11 20/37] pinctrl: airoha: fix IRQ mask/unmask code ---
commit 6fffeeb5d12a43c2d9965d0cb9e5d1e4595a6f8f
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: fix IRQ mask/unmask code
    
This commit adds missing gpiochip_enable_irq() and gpiochip_disable_irq()
calls to the IRQ mask and unmask functions. This ensures gpiolib correctly
sets the GPIOD_FLAG_IRQ_IS_ENABLED bit.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index f3078f9b14a76..4859fcebac7de 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -2595,6 +2595,7 @@ static void airoha_irq_unmask(struct irq_data *data)
>  	if (WARN_ON_ONCE(data->hwirq >= ARRAY_SIZE(gpiochip->irq_type)))
>  		return;
>  
> +	gpiochip_enable_irq(gc, irqd_to_hwirq(data));
>  	switch (gpiochip->irq_type[data->hwirq]) {
>  	case IRQ_TYPE_LEVEL_LOW:
>  		val = val << 1;

[ ... ]

> @@ -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 calling regmap_clear_bits() here
cause a sleep in atomic context on PREEMPT_RT?

Since airoha_irq_mask() is called by the core irq framework with the raw
desc->lock held, using a syscon regmap which defaults to a sleepable
spinlock on PREEMPT_RT could lead to a lockdep splat or crash. This also
applies to the regmap_update_bits() call in airoha_irq_unmask().

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

[Severity: High]
This isn't a bug introduced by this patch, but does the driver lose
edge-triggered interrupts in the handler?

Looking at airoha_irq_handler(), the hardware status is cleared via
regmap_write() after the handler has executed:

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

Also, airoha_pinctrl_add_gpiochip() hardcodes handle_simple_irq for all
interrupt types:

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

If a second edge happens while the interrupt is being serviced, wouldn't the
hardware latch the status, only for it to be wiped out by the regmap_write()
after generic_handle_irq() returns without being serviced?

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