Re: [PATCH v3 2/3] gpio: pl061: add Ambarella CV75 register layout variant

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The probe function unconditionally clears the entire `afsel` (alternate function select) register for the whole GPIO bank, breaking bootloader pinmux state.
- [High] Silent removal of memory barriers from I/O accesses by switching to `_relaxed` accessors.
- [High] The newly added `mask` register is restored in `pl061_resume` without acquiring `pl061->lock`, breaking the atomicity of the `mask` and `data` register sequence.
--

--- Patch [2]: [PATCH v3 2/3] gpio: pl061: add Ambarella CV75 register layout variant ---
commit fe14f60fc7b7fd8cd63e65496225f1d2973a8ad1
Author: Long Zhao <[email protected]>

gpio: pl061: add Ambarella CV75 register layout variant

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

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 919cf86fd590f..d9e5a9f25681c 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -26,41 +26,94 @@
[ ... ]
> +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 switching from readb/writeb to the _relaxed accessors introduce memory
ordering regressions?

Since the original driver used the standard non-relaxed I/O accessors, this
change drops the implicit memory barriers on all accesses. Can this cause
CPU reordering of GPIO writes ahead of memory writes for drivers that depend
on memory writes being visible before signaling hardware?

[ ... ]
> @@ -335,7 +402,14 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  	/*
>  	 * irq_chip support
>  	 */
> -	writeb(0, pl061->base + GPIOIE); /* disable irqs */
> +	pl061_write(pl061, 0, pl061->variant->ie); /* disable irqs */
> +	if (pl061->variant->enable != PL061_REG_NONE)
> +		pl061_write(pl061, GENMASK(pl061->gc.ngpio - 1, 0),
> +			    pl061->variant->enable);
> +	if (pl061->variant->afsel != PL061_REG_NONE)
> +		pl061_write(pl061, 0, pl061->variant->afsel);

[Severity: High]
Is it safe to write 0 to the entire afsel register during initialization?

By unconditionally zeroing out the alternate function selection during probe,
does this blindly force all pins into GPIO mode and disconnect any active
peripherals configured by the bootloader or early firmware?

[ ... ]
> @@ -398,20 +489,75 @@ static int pl061_resume(struct device *dev)
[ ... ]
> +	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]
Should pl061_resume hold pl061->lock when restoring the mask register?

Writing to the mask register without the lock breaks atomicity between mask
and data accesses. If an interrupt fires here and triggers a concurrent
pl061_set_data on another CPU, could the resume function overwrite the
mask register before the concurrent thread completes its data write, causing
it to update the wrong pins?

> +	if (pl061->variant->enable != PL061_REG_NONE)
> +		pl061_write(pl061, GENMASK(pl061->gc.ngpio - 1, 0),
> +			    pl061->variant->enable);
>  
>  	return 0;
>  }

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