Re: [PATCH v9 12/15] gpio: pl061: add Ambarella register-layout variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: pl061: output state corruption via RMW on Ambarella data register
--

commit b4f961be1d109a1753acf06716b7c68488ad35bd
Author: Long Zhao <longzhao@ambarella.com>

    gpio: pl061: add Ambarella register-layout variant
    
    Add a 32-bit, 32-line layout with DAT/DIR/IS/IBE/IEV/IE/MIS/IC plus
    MASK and ENABLE. Probe unmasks the bank and enables the controller.
    IRQ type changes also clear the pending bit. This derivative does not
    need write-after-direction or system suspend save/restore.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 08d45f8923c2a..bbb9eeeb074aa 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c

[ ... ]

> @@ -318,6 +330,18 @@ static bool pl061_arm_volatile_reg(struct device *dev, unsigned int reg)
>  	}
>  }
>  
> +static bool pl061_ambarella_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AMBARELLA_DAT:
> +	case AMBARELLA_MIS:
> +	case AMBARELLA_IC:
> +		return true;

[Severity: High]
Does marking AMBARELLA_DAT as volatile expose the driver to data corruption
during read-modify-write operations?

Because AMBARELLA_DAT is marked volatile and .cache_type is omitted below
in pl061_ambarella_regmap_config (defaulting to REGCACHE_NONE), RMW
operations on AMBARELLA_DAT (like via gpio_set_value) will bypass the cache.

Since this is a full 32-bit width register, a RMW operation reads the
physical state of all 32 pins and writes it back. This could overwrite the
output flip-flops of all other pins with their current physical state
instead of their driven values.

Unlike the ARM variant which leverages address-based masking to safely set
individual bits, it looks like Ambarella relies on a single 32-bit register
and would strictly require regmap caching (e.g., REGCACHE_FLAT) to perform
safe RMW operations.

> +	default:
> +		return false;
> +	}
> +}
> +
>  static const struct regmap_config pl061_arm_regmap_config = {
>  	.reg_bits = 16,
>  	.val_bits = 8,

[ ... ]

This is a pre-existing issue, but I noticed a potential interrupt storm due
to the incorrect hardware initialization order in pl061_probe():

drivers/gpio/gpio-pl061.c:pl061_probe() {
    ...
	if (regs->enable) {
		ret = regmap_write(pl061->regmap, regs->enable, ~0U);
		if (ret)
			return ret;
	}
	if (regs->mask) {
		ret = regmap_write(pl061->regmap, regs->mask, ~0U);
		if (ret)
			return ret;
	}

	/*
	 * irq_chip support
	 */
	regmap_write(pl061->regmap, regs->ie, 0); /* disable irqs */
    ...
}

[Severity: Medium]
Can this sequence trigger an interrupt storm that permanently disables the
parent IRQ line?

The hardware controller is unmasked and enabled globally (via regs->enable
and regs->mask) before its local interrupts are masked (regs->ie = 0). If
the bootloader or kexec leaves the device with pending interrupts and IE
set, enabling the controller immediately asserts the IRQ line before the
driver has registered the chained handler.


This is also a pre-existing issue, but I noticed a potential use-after-free
of the irq_domain on driver teardown in pl061_probe():

drivers/gpio/gpio-pl061.c:pl061_probe() {
    ...
	gpio_regmap = devm_gpio_regmap_register(dev, &config);
	if (IS_ERR(gpio_regmap)) {
    ...

	if (irq) {
		ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);
		if (ret)
			return ret;

		irq_set_chained_handler_and_data(irq, pl061_irq_handler, pl061);
	}
    ...
}

[Severity: High]
Does registering the IRQ teardown action after registering the gpio_regmap
cause a use-after-free?

During driver unbind or probe failure, devm actions execute in LIFO order.
Because devm_add_action_or_reset(dev, pl061_remove_irq) is called after
devm_gpio_regmap_register(), the pl061_remove_irq action executes first and
frees pl061->irq_domain.

Subsequently, devm_gpio_regmap_unregister calls gpiochip_remove(), which
iterates over active IRQs and calls gpiod_to_irq(), dereferencing the
already-freed domain. This could be reached by holding a GPIO line event FD
open from userspace while unbinding the driver via sysfs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-cv75-v5-v9-0-486cc289a4b8@ambarella.com?part=12