Re: [PATCH v11 21/37] pinctrl: airoha: fix edge-triggered interrupts handling

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double-clearing of the interrupt status creates a race condition that leads to permanently lost edge interrupts.
- [High] Accessing a syscon regmap from within IRQ flow handler callbacks violates raw lock nesting rules, causing deadlocks on PREEMPT_RT.
--

--- Patch [21]: [PATCH v11 21/37] pinctrl: airoha: fix edge-triggered interrupts handling ---
commit 1d404909643557ff6d1ca5162d1185d537256a4a
Author: Mikhail Kshevetskiy <[email protected]>
pinctrl: airoha: fix edge-triggered interrupts handling

This patch updates the airoha pinctrl driver to correct edge-triggered 
interrupt handling. It introduces a custom irq_ack callback and explicitly 
configures the flow handlers for edge and level interrupts.

> 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

[ ... ]

> +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]
Does this regmap_write call violate wait-type lock nesting rules on
preempt-rt systems?

With the changes to airoha_irq_type setting the flow handler to
handle_edge_irq, airoha_irq_ack is now invoked from the fast-path where a
raw_spinlock_t is held.

However, regmap_write on a syscon regmap acquires a standard spinlock_t,
which is a sleepable lock on preempt-rt kernels. Can this lead to
deadlocks or lockdep warnings?

> +}
> +
>  static int airoha_irq_type(struct irq_data *data, unsigned int type)

[ ... ]

> @@ -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]
Is it possible for this manual regmap_write to cause newly latched edge 
interrupts to be permanently lost?

Because handle_edge_irq now correctly calls airoha_irq_ack to clear the
status bit early, a new edge interrupt can latch during handler execution.

After generic_handle_domain_irq returns, this trailing regmap_write clears
the same status bit again unconditionally. If a new edge interrupt arrives
while the domain handler is executing, won't this second clear wipe it out?

>  		}

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