Re: [PATCH] gpio: Use IRQ trigger mask helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing spinlock serialization for shared bank registers in IRQ chip callbacks.
--

--- Patch [1]: [PATCH] gpio: Use IRQ trigger mask helpers ---
commit a6c81ea868de6b9a08df67a583f7641fb4809834
Author: Maulik Shah <maulik.shah@oss.qualcomm.com>

    gpio: Use IRQ trigger mask helpers
    
    Use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH instead of open-coded
    trigger type combinations in irqchip callbacks.
    
    No functional change intended.

[ ... ]

> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
> index 5d61053e0596a..2b5da973f51fe 100644
> --- a/drivers/gpio/gpio-pxa.c
> +++ b/drivers/gpio/gpio-pxa.c
> @@ -406,7 +406,7 @@ static int pxa_gpio_irq_type(struct irq_data *d, unsigned int type)
>  		if (__gpio_is_occupied(pchip, gpio))
>  			return 0;
>  
> -		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
> +		type = IRQ_TYPE_EDGE_BOTH;
>  	}
>  
>  	gpdr = readl_relaxed(c->regbase + GPDR_OFFSET);
>  
>  	if (__gpio_is_inverted(gpio))
>  		writel_relaxed(gpdr | mask,  c->regbase + GPDR_OFFSET);
>  	else
>  		writel_relaxed(gpdr & ~mask, c->regbase + GPDR_OFFSET);

[Severity: High]
This is a pre-existing issue, but since you are touching this function,
is it safe to perform these read-modify-write operations on the GPDR register 
without a spinlock? 

It seems this could race with other IRQs in the same bank and with 
gpio_direction operations. For instance, pxa_gpio_irq_type() races with 
pxa_gpio_direction_input() and pxa_gpio_direction_output() over the GPDR 
register, as the latter use gpio_lock but the former does not.

A similar issue appears in pxa_mask_muxed_gpio():

pxa_mask_muxed_gpio() {
    ...
    b->irq_mask &= ~GPIO_bit(gpio);

    grer = readl_relaxed(base + GRER_OFFSET) & ~GPIO_bit(gpio);
    gfer = readl_relaxed(base + GFER_OFFSET) & ~GPIO_bit(gpio);
    writel_relaxed(grer, base + GRER_OFFSET);
    writel_relaxed(gfer, base + GFER_OFFSET);
    ...
}

and pxa_unmask_muxed_gpio():

pxa_unmask_muxed_gpio() {
    ...
    c->irq_mask |= GPIO_bit(gpio);
    update_edge_detect(c);
    ...
}

Can this lack of serialization lead to data corruption of the GPIO direction
configuration (GPDR) and interrupt edge detection configuration, potentially
causing improperly configured pins or permanent loss of interrupts on SMP
systems when called concurrently for different GPIOs within the same bank?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802-b4b4-irq-trigger-mask-gpio-v1-1-948973636c84@oss.qualcomm.com?part=1