Re: [PATCH v10 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 creates a race condition that silently drops newly latched interrupts.

Pre-existing issues:
- [High] Wait context violation on PREEMPT_RT kernels due to regmap accesses under raw spinlock.
--

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

pinctrl: airoha: fix edge-triggered interrupts handling
    
This patch fixes the handling of edge-triggered interrupts in the Airoha
pinctrl driver. It defines an irq_ack() handler and configures the correct
high-level interrupt handlers based on the interrupt type.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 4859fcebac7de..960b0731459cd 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)
[ ... ]
> +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 will this regmap access cause a wait
context violation on preempt-rt kernels?

The .irq_ack callback is invoked by handle_edge_irq() while holding
the irq descriptor's raw_spinlock_t. If this syscon-based regmap does not
have use_raw_spinlock explicitly enabled, its standard spinlock_t becomes
a sleepable rt_mutex on preempt-rt, which is illegal to acquire under 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 introduce a regression by double-acknowledging the interrupt,
creating a race condition that could silently drop newly latched edge
interrupts?

For edge interrupts, generic_handle_domain_irq() calls handle_edge_irq(),
which calls airoha_irq_ack() to clear the hardware status bit before
executing the device action handler. If a new edge interrupt arrives during
the action handler, the hardware latches the event by setting the status bit
again. 

Once generic_handle_domain_irq() returns, this unconditional regmap_write()
clears the status bit a second time, which would wipe out the newly latched
interrupt before the interrupt subsystem can process it.

>  		}

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