Re: [PATCH AUTOSEL 6.18-5.10] gpio: mvebu: keep resume masks within the irqchip cache

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: mvebu: TOCTOU race in mvebu_gpio_resume() after dropping gc->lock
- [High] gpio: mvebu: wrong regmap used for Armada XP masks in mvebu_gpio_resume()
--

commit 3f933b6bd33ac55ae181080e42e681c5eace741e
Author: Rosen Penev <rosenp@gmail.com>

gpio: mvebu: keep resume masks within the irqchip cache

This commit updates mvebu_gpio_resume() to mask the saved edge and level
interrupt mask registers against the generic irqchip mask_cache_priv values
before writing them back to hardware on resume.

Link: https://patch.msgid.link/20260915004406.115230-1-rosenp@gmail.com

> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index ac799fced950e..07af0f38252d6 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -1006,6 +1006,8 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
>  static int mvebu_gpio_resume(struct platform_device *pdev)
>  {
>  	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
> +	u32 edge_cache = ~0U, level_cache = ~0U;
> +	unsigned long flags;
>  	int i;
>  
>  	regmap_write(mvchip->regs, GPIO_OUT_OFF + mvchip->offset,
> @@ -1017,32 +1019,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]
Can dropping gc->lock and restoring local interrupts before writing the
hardware mask registers race with the irqchip mask and unmask callbacks?

The irqchip callbacks such as mvebu_gpio_level_irq_mask() and
mvebu_gpio_level_irq_unmask() hold gc->lock across both updating
ct->mask_cache_priv and updating the hardware mask register:

drivers/gpio/gpio-mvebu.c:mvebu_gpio_level_irq_mask() {
	guard(raw_spinlock)(&gc->lock);
	ct->mask_cache_priv &= ~mask;
	mvebu_gpio_write_level_mask(mvchip, ct->mask_cache_priv);
}

Since mvebu_gpio_resume() runs during dpm_resume() after dpm_resume_noirq() ->
resume_device_irqs() has re-enabled device interrupts, if a level GPIO
interrupt fires in the window between raw_spin_unlock_irqrestore() and
regmap_write(), the interrupt path:

  mvebu_gpio_irq_handler()
    -> handle_level_irq()
      -> mask_ack_irq()
        -> mvebu_gpio_level_irq_mask()

will clear the bit in ct->mask_cache_priv, mask the hardware register, and set
IRQD_IRQ_MASKED in genirq. When mvebu_gpio_resume() then calls regmap_write()
with its stale level_cache snapshot where the bit is still 1, won't it unmask
the asserted level line in hardware behind genirq's back and cause the same
unhandled level interrupt storm on resume?

Similarly, if an in-flight IRQF_ONESHOT threaded handler finishes via
irq_finalize_oneshot() -> unmask_threaded_irq() or another CPU calls
enable_irq() -> unmask_irq() during that window, the unmask callback will set
the bit in ct->mask_cache_priv and unmask the hardware register under
gc->lock. Won't mvebu_gpio_resume() then overwrite the hardware register with
a stale 0 bit from level_cache or edge_cache, leaving the interrupt
permanently masked in hardware while genirq considers it unmasked?

> +
>  	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);
>  		regmap_write(mvchip->regs, GPIO_LEVEL_MASK_OFF + mvchip->offset,
> -			     mvchip->level_mask_regs[0]);
> +			     mvchip->level_mask_regs[0] & level_cache);
>  		break;
>  	case MVEBU_GPIO_SOC_VARIANT_MV78200:
>  		for (i = 0; i < 2; i++) {
>  			regmap_write(mvchip->regs,
>  				     GPIO_EDGE_MASK_MV78200_OFF(i),
> -				     mvchip->edge_mask_regs[i]);
> +				     mvchip->edge_mask_regs[i] & edge_cache);
>  			regmap_write(mvchip->regs,
>  				     GPIO_LEVEL_MASK_MV78200_OFF(i),
> -				     mvchip->level_mask_regs[i]);
> +				     mvchip->level_mask_regs[i] & level_cache);
>  		}
>  		break;
>  	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
>  		for (i = 0; i < 4; i++) {
>  			regmap_write(mvchip->regs,
>  				     GPIO_EDGE_MASK_ARMADAXP_OFF(i),
> -				     mvchip->edge_mask_regs[i]);
> +				     mvchip->edge_mask_regs[i] & edge_cache);
>  			regmap_write(mvchip->regs,
>  				     GPIO_LEVEL_MASK_ARMADAXP_OFF(i),
> -				     mvchip->level_mask_regs[i]);
> +				     mvchip->level_mask_regs[i] & level_cache);
>  		}
>  		break;

[Severity: High]
On MVEBU_GPIO_SOC_VARIANT_ARMADAXP, are these masked writes targeting the
wrong regmap in mvebu_gpio_resume() (and similarly in mvebu_gpio_suspend())?

In mvebu_gpioreg_edge_mask(), mvebu_gpioreg_level_mask(), and
mvebu_gpio_probe(), the per-CPU mask registers at
GPIO_EDGE_MASK_ARMADAXP_OFF(cpu) and GPIO_LEVEL_MASK_ARMADAXP_OFF(cpu) live in
mvchip->percpu_regs rather than mvchip->regs:

drivers/gpio/gpio-mvebu.c:mvebu_gpioreg_edge_mask() {
	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
		cpu = smp_processor_id();
		*map = mvchip->percpu_regs;
		*offset = GPIO_EDGE_MASK_ARMADAXP_OFF(cpu);
		break;
}

drivers/gpio/gpio-mvebu.c:mvebu_gpio_probe() {
	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
		regmap_write(mvchip->regs, GPIO_EDGE_CAUSE_OFF, 0);
		regmap_write(mvchip->regs, GPIO_EDGE_MASK_OFF, 0);
		regmap_write(mvchip->regs, GPIO_LEVEL_MASK_OFF, 0);
		for (cpu = 0; cpu < 4; cpu++) {
			regmap_write(mvchip->percpu_regs,
				     GPIO_EDGE_CAUSE_ARMADAXP_OFF(cpu), 0);
			regmap_write(mvchip->percpu_regs,
				     GPIO_EDGE_MASK_ARMADAXP_OFF(cpu), 0);
			regmap_write(mvchip->percpu_regs,
				     GPIO_LEVEL_MASK_ARMADAXP_OFF(cpu), 0);
		}
		break;
}

By passing mvchip->regs instead of mvchip->percpu_regs here, won't
mvebu_gpio_resume() leave the per-CPU mask registers in mvchip->percpu_regs
unupdated while writing the edge_cache and level_cache masked values over the
global GPIO registers at offsets 0x10..0x2c in mvchip->regs (such as
GPIO_EDGE_CAUSE_OFF at 0x14, GPIO_LEVEL_MASK_OFF at 0x1c, and
GPIO_BLINK_CNT_SELECT_OFF at 0x20)?

>  	default:

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-150-sashal@kernel.org?part=1