Re: [PATCH v5 08/12] gpio: pl061: add Ambarella CV75 register layout variant

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware state corruption: `pl061_resume` forcefully claims all pins as GPIOs, glitching active alternate functions.
- [High] Unlocked restoration of the `mask` register in `pl061_resume` races with consumer IRQ handlers.
- [High] Silent removal of implicit memory barriers by substituting `_relaxed` I/O accessors.
--

--- Patch [8]: [PATCH v5 08/12] gpio: pl061: add Ambarella CV75 register layout variant ---
commit 507564f23a0859a6fc4454b94d3ad422721665ac
Author: Long Zhao <[email protected]>

gpio: pl061: add Ambarella CV75 register layout variant

This commit extends gpio-pl061 with a per-variant register layout. This 
allows the Ambarella CV75 GPIO banks to reuse the PL061 driver instead of 
adding a duplicate driver.

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 919cf86fd590f..7002ddbc97138 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -50,15 +50,17 @@
[ ... ]
> +static u32 pl061_read(struct pl061 *pl061, u32 offset)
> +{
> +	if (pl061->variant->access_32bit)
> +		return readl_relaxed(pl061->base + offset);
> +
> +	return readb_relaxed(pl061->base + offset);
> +}
> +
> +static void pl061_write(struct pl061 *pl061, u32 value, u32 offset)
> +{
> +	if (pl061->variant->access_32bit)
> +		writel_relaxed(value, pl061->base + offset);
> +	else
> +		writeb_relaxed(value, pl061->base + offset);
> +}

[Severity: High]
Does this substitution of standard I/O accessors with their relaxed variants
silently remove implicit memory barriers?

Previously, the driver used readb() and writeb(), which include memory
barriers. By switching to the relaxed accessors, if a driver updates a DMA
buffer and then calls gpiod_set_value() to trigger the hardware, the CPU
could reorder the GPIO write before the memory write. 

Could this cause the device to observe the GPIO edge before the DMA data
actually reaches memory?

[ ... ]
> @@ -389,7 +512,7 @@ static int pl061_resume(struct device *dev)
>  	struct pl061 *pl061 = dev_get_drvdata(dev);
>  	int offset;
>  
> -	for (offset = 0; offset < PL061_GPIO_NR; offset++) {
> +	for (offset = 0; offset < pl061->gc.ngpio; offset++) {
>  		if (pl061->csave_regs.gpio_dir & (BIT(offset)))
>  			pl061_direction_output(&pl061->gc, offset,
>  					pl061->csave_regs.gpio_data &
>  					(BIT(offset)));
>  		else
>  			pl061_direction_input(&pl061->gc, offset);
>  	}

[Severity: High]
Can this loop corrupt the hardware state of alternate function pins during
system resume?

This loop unconditionally calls pl061_direction_output() or
pl061_direction_input() on all pins. Since this patch now makes those
functions call pl061_claim_gpio(), which clears the alternate function select
(afsel) bit:

pl061_direction_output() -> pl061_claim_gpio():
    afsel = pl061_read(pl061, pl061->variant->afsel);
    if (afsel & BIT(offset))
        pl061_write(pl061, afsel & ~BIT(offset),
                    pl061->variant->afsel);

Won't this forcefully claim active alternate function pins (like UART or SPI)
as GPIOs and momentarily glitch their interfaces until afsel is eventually
restored below?

[ ... ]
> -	writeb(pl061->csave_regs.gpio_is, pl061->base + GPIOIS);
> -	writeb(pl061->csave_regs.gpio_ibe, pl061->base + GPIOIBE);
> -	writeb(pl061->csave_regs.gpio_iev, pl061->base + GPIOIEV);
> -	writeb(pl061->csave_regs.gpio_ie, pl061->base + GPIOIE);
> +	pl061_write(pl061, pl061->csave_regs.gpio_is,
> +		    pl061->variant->is);
> +	pl061_write(pl061, pl061->csave_regs.gpio_ibe,
> +		    pl061->variant->ibe);
> +	pl061_write(pl061, pl061->csave_regs.gpio_iev,
> +		    pl061->variant->iev);
> +	pl061_write(pl061, pl061->csave_regs.gpio_ie,
> +		    pl061->variant->ie);
> +	if (pl061->variant->afsel != PL061_REG_NONE)
> +		pl061_write(pl061, pl061->csave_regs.gpio_afsel,
> +			    pl061->variant->afsel);
> +	if (pl061->variant->mask != PL061_REG_NONE)
> +		pl061_write(pl061, pl061->csave_regs.gpio_mask,
> +			    pl061->variant->mask);

[Severity: High]
Is there a race condition between restoring the interrupt enable register and
the mask register?

When pl061_resume() restores gpio_ie, it immediately enables device
interrupts. It then proceeds to restore the mask register without holding
pl061->lock.

If a consumer IRQ handler executes concurrently and accesses a GPIO, it will
acquire the lock and write its target bit to the mask register, which acts as
an address pointer in pl061_set_data(). The unlocked pl061_resume() could
then overwrite this mask register just before the IRQ handler accesses the
data register, potentially misdirecting the GPIO operation.

Should the mask be restored before gpio_ie, or should this section be
protected by the lock?

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