Re: [PATCH v9 20/36] pinctrl: airoha: fix edge-triggered interrupts handling

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

New issues:
- [High] Double-acknowledgment of edge-triggered interrupts in `airoha_irq_handler` causes permanently lost interrupts.

Pre-existing issues:
- [High] Calling `regmap_write()` / `regmap_clear_bits()` under a raw spinlock leads to invalid lock nesting and sleep-in-atomic on PREEMPT_RT.
--

--- Patch [20]: [PATCH v9 20/36] pinctrl: airoha: fix edge-triggered interrupts handling ---
commit 1e0dfc059b036a31732f2260cc6e2e46d9e2bedf
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: fix edge-triggered interrupts handling

This patch adds an irq_ack handler and configures edge-triggered
interrupts to use handle_edge_irq instead of the simple handler.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 42ae588e6343b..4929e90d19a82 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -2628,11 +2628,28 @@ static void airoha_irq_mask(struct irq_data *data)
>  	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
>  
> +	if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
> +		return;
> +
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->level[index], mask);
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->edge[index], mask);
>  	gpiochip_disable_irq(gc, irqd_to_hwirq(data));
>  }
>  
> +static void airoha_irq_ack(struct irq_data *data)
> +{
> +	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;
> +	u8 offset = data->hwirq % AIROHA_PIN_BANK_SIZE;
> +	u8 index = data->hwirq / AIROHA_PIN_BANK_SIZE;
> +
> +	if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
> +		return;
> +
> +	regmap_write(pinctrl->regmap, gpiochip->status[index], BIT(offset));

[Severity: High]
This is a pre-existing issue, but does using regmap_write() inside
irq_ack() (and similarly in irq_mask()) cause an invalid lock nesting on
preempt-rt systems?

The irq core invokes these callbacks with the descriptor's raw spinlock
held. Since pinctrl->regmap is a standard syscon, it uses a standard
spinlock_t internally. On preempt-rt systems, spinlock_t becomes a
sleepable rt_mutex, which could lead to sleeping while holding a raw
spinlock.

> +}

[ ... ]

> @@ -2673,8 +2702,7 @@ static irqreturn_t airoha_irq_handler(int irq, void *data)
>  		for_each_set_bit(irq, &status, AIROHA_PIN_BANK_SIZE) {
>  			u32 offset = irq + i * AIROHA_PIN_BANK_SIZE;
>  
> -			generic_handle_irq(irq_find_mapping(girq->domain,
> -							    offset));
> +			generic_handle_domain_irq(girq->domain, offset);
>  			regmap_write(pinctrl->regmap,
>  				     pinctrl->gpiochip.status[i], BIT(irq));

[Severity: High]
Does this trailing regmap_write() clear newly latched interrupts?

Since this patch introduces airoha_irq_ack() which acknowledges the
hardware interrupt early during handle_edge_irq(), clearing the status bit
again here could wipe out new edge events that latched while the
handler was running.

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