Re: [PATCHv2] gpio: mvebu: keep resume masks within the irqchip cache

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: mvebu: TOCTOU race in resume causes interrupt storm
--

commit 6b10f9bb173190e25a679e67f7992002ca12fe2f
Author: Rosen Penev <rosenp@xxxxxxxxx>

gpio: mvebu: keep resume masks within the irqchip cache

This commit fixes a resume hang in mvebu_gpio_resume by masking restored
values with the matching irqchip mask cache. It reads the caches under
gc->lock to keep them consistent with mask/unmask handlers.

> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 93b8a08b04b9f..a2796240fe14b 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c

[ ... ]

> @@ -1045,32 +1047,51 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
>  	regmap_write(mvchip->regs, GPIO_IN_POL_OFF + mvchip->offset,
>  		     mvchip->in_pol_reg);
>  
> +	/*
> +	 * genirq skips mask_irq() for a line it already considers masked, so
> +	 * unmasking one behind its back leaves an asserted level line that
> +	 * nobody masks. Restore only bits the irqchip cache still has set.
> +	 *
> +	 * Snapshot the caches under the raw spinlock, but release it before
> +	 * the regmap writes below: regmap_write() takes a sleepable lock on
> +	 * PREEMPT_RT.
> +	 */
> +	if (mvchip->domain) {
> +		struct irq_chip_generic *gc;
> +
> +		gc = irq_get_domain_generic_chip(mvchip->domain, 0);
> +		raw_spin_lock_irqsave(&gc->lock, flags);
> +		level_cache = gc->chip_types[0].mask_cache_priv;
> +		edge_cache = gc->chip_types[1].mask_cache_priv;
> +		raw_spin_unlock_irqrestore(&gc->lock, flags);

[Severity: High]
Does releasing the lock here create a race window?

If a level-triggered interrupt fires immediately after the lock is released,
the IRQ handler runs. Genirq would acquire the lock, mask the hardware, and
clear the bit in mask_cache_priv.

> +	}
> +
>  	switch (mvchip->soc_variant) {
>  	case MVEBU_GPIO_SOC_VARIANT_ORION:
>  	case MVEBU_GPIO_SOC_VARIANT_A8K:
>  		regmap_write(mvchip->regs, GPIO_EDGE_MASK_OFF + mvchip->offset,
> -			     mvchip->edge_mask_regs[0]);
> +			     mvchip->edge_mask_regs[0] & edge_cache);

[Severity: High]
Could writing the stale snapshot here unmask an IRQ that genirq just masked?

If the race described above occurs, mvebu_gpio_resume() continues and writes
its stale snapshot (with the bit set) to the hardware mask register. This
would unmask the line in hardware while genirq's irq_data maintains the
IRQD_IRQ_MASKED flag.

When the interrupt inevitably fires again, could genirq skip masking it
because it is already marked masked, leading to an unhandled infinite
interrupt storm and kernel lockup?


[Severity: High]
This is a pre-existing issue, but does the bitwise AND permanently drop IRQs
that were unmasked during early resume?

If a driver disables an IRQ before suspend, the snapshot bit in
edge_mask_regs is 0. If the driver then calls enable_irq() during an early
resume phase (like .resume_noirq), genirq correctly unmasks the hardware and
sets the cache bit to 1.

When mvebu_gpio_resume() later executes in the .resume phase, it calculates
0 (snapshot) & 1 (cache) = 0 and writes 0 to the hardware. Does this leave
the hardware permanently masked while genirq considers it unmasked, silently
breaking the interrupt?

>  		regmap_write(mvchip->regs, GPIO_LEVEL_MASK_OFF + mvchip->offset,
> -			     mvchip->level_mask_regs[0]);
> +			     mvchip->level_mask_regs[0] & level_cache);
>  		break;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915004406.115230-1-rosenp@xxxxxxxxx?part=1