Re: [PATCH v2 07/10] pinctrl: ambarella: add Ambarella pin controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Improper devres cleanup ordering leaves the GPIO chip with a destroyed IRQ domain, leading to Use-After-Free.
- [High] Inconsistent enforcement of hsm_domain_id access restrictions for IOMUX registers causes bus faults.
- [High] The device tree node pointer is assigned to bank->gc.fwnode without taking a reference, leading to a Use-After-Free.
- [High] Calling regmap_update_bits() while holding a raw_spinlock_t causes a lock nesting bug on PREEMPT_RT.
- [High] Performing regmap accesses in _noirq power management callbacks causes lockdep BUGs on PREEMPT_RT.
- [Medium] Improper register restoration order during resume causes glitches on GPIO output pins and pull resistors.
- [Medium] amb_pinctrl_suspend_noirq overwrites GPIO_IE_OFFSET with irq_wake_mask, disabling IRQF_NO_SUSPEND interrupts.
- [Medium] The driver fails to properly handle clk_au_dedicated_pin on device trees with non-contiguous GPIO banks.
- [Medium] amb_gpio_irq_enable unconditionally forces the pin direction to INPUT, breaking open-drain output IRQs.
--

--- Patch [7]: [PATCH v2 07/10] pinctrl: ambarella: add Ambarella pin controller ---
commit e1626036cbd93218655297fbd8b45ae82567717d
Author: Long Zhao <longzhao@ambarella.com>

pinctrl: ambarella: add Ambarella pin controller
    
Add the Ambarella pinmux/GPIO controller driver used by CV75.

> diff --git a/drivers/pinctrl/pinctrl-ambarella.c b/drivers/pinctrl/pinctrl-ambarella.c
> index 0000000000000..bb79e06c4160f 100644
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-ambarella.c

[ ... ]

> @@ -462,13 +462,13 @@ static int amb_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  	bank = gpio_bank->hw_id;
>  	offset = pin - gpio_bank->pin_base;
>  
>  	raw_spin_lock_irqsave(&soc->lock, flags);
>  	for (i = 0; i < num_configs; i++) {
>  		config = configs[i];
>  		param = pinconf_to_config_param(config);
>  		arg = pinconf_to_config_argument(config);
>  
>  		switch (param) {
>  		case PIN_CONFIG_BIAS_DISABLE:
>  			regmap_update_bits(soc->pull_regmap,
>  					   soc->data->pull_en[bank], BIT(offset), 0);

[Severity: High]
The syscon regmaps use a standard spinlock_t internally, which maps to a
sleepable rt_mutex on PREEMPT_RT. 

Since raw_spin_lock_irqsave(&soc->lock, flags) is held, does calling
regmap_update_bits() here trigger a "sleeping in atomic context" BUG
on PREEMPT_RT?

[ ... ]

> @@ -746,13 +746,13 @@ static int amb_gpio_direction_output(struct gpio_chip *gc, unsigned int pin, int
>  	writel_relaxed(data | BIT(pin), bank->base + GPIO_DIR_OFFSET);
>  
>  	raw_spin_unlock_irqrestore(&soc->lock, flags);
>  
>  	return 0;
>  }
>  
>  /* gpiolib gpio_to_irq callback function */
>  static int amb_gpio_to_irq(struct gpio_chip *gc, unsigned int pin)
>  {
>  	struct amb_gpio_bank *bank = gpiochip_get_data(gc);
>  
>  	return irq_create_mapping(bank->domain, pin);
>  }

[ ... ]

> @@ -1181,24 +1181,24 @@ static int amb_gpio_register(struct amb_pinctrl_soc_data *soc)
>  
>  		bank->domain = irq_domain_add_linear(to_of_node(bank->gc.fwnode),
>  						    bank->gc.ngpio,
>  						    &amb_gpio_irq_domain_ops,
>  						    bank);
>  		if (!bank->domain)
>  			return dev_err_probe(soc->dev, -ENODEV,
>  					     "GPIO%u: failed to create irqdomain\n",
>  					     i);
>  
>  		rval = devm_gpiochip_add_data(soc->dev, &bank->gc, bank);
>  		if (rval) {
>  			irq_domain_remove(bank->domain);
>  			bank->domain = NULL;
>  			return dev_err_probe(soc->dev, rval,
>  					     "GPIO%u: gpiochip registration failed\n",
>  					     i);
>  		}
>  
>  		writel_relaxed(0xffffffff, bank->base + GPIO_ENABLE_OFFSET);
>  		writel_relaxed(0x00000000, bank->base + GPIO_AFSEL_OFFSET);
>  		writel_relaxed(0x00000000, bank->base + GPIO_MASK_OFFSET);
>  
>  		irq_set_irq_type(bank->irq, IRQ_TYPE_LEVEL_HIGH);
>  		irq_set_chained_handler_and_data(bank->irq,
>  						 amb_gpio_handle_irq, bank);
>  
>  		rval = devm_add_action_or_reset(soc->dev,
>  					       amb_gpio_irq_cleanup, bank);

[Severity: High]
The devres cleanup action amb_gpio_irq_cleanup() is registered after the
gpiochip is added. Since devres actions execute in reverse order, the IRQ
domain will be destroyed before the gpiochip is removed.

Could this leave the gpiochip with a NULL bank->domain during unbind,
causing a use-after-free if amb_gpio_to_irq() is called?

[ ... ]

> @@ -841,13 +841,13 @@ static void amb_gpio_irq_enable(struct irq_data *data)
>  
>  	offset = irqd_to_hwirq(data);
>  
>  	gpiochip_enable_irq(gc, offset);
>  
>  	raw_spin_lock_irqsave(&soc->lock, flags);
>  
>  	val = readl_relaxed(gpio_base + GPIO_DIR_OFFSET);
>  	val &= ~(0x1 << offset);
>  	writel_relaxed(val, gpio_base + GPIO_DIR_OFFSET);

[Severity: Medium]
The gpiolib core supports open-drain outputs as interrupt sources. Will
unconditionally clearing the direction bit here force the pin to be an
input, breaking any open-drain output IRQs?

>  
>  	for (i = 0; i < 3; i++) {
>  		val = readl_relaxed(iomux_base + IOMUX_OFFSET(bank->hw_id, i));
>  		val &= ~(0x1 << offset);
>  		writel_relaxed(val, iomux_base + IOMUX_OFFSET(bank->hw_id, i));
>  	}

[Severity: High]
The driver restricts IOMUX register access in amb_pinmux_set_altfunc()
to only the safety domain (hsm_domain_id == 0).

Does this loop bypass that check? Can unprotected writes here trigger a bus
fault on platforms where hsm_domain_id != 0?

[ ... ]

> @@ -996,9 +996,9 @@ static int amb_gpio_parse_dt(struct amb_pinctrl_soc_data *soc)
>  			if (soc->banks[i].hw_id == hw_id)
>  				return dev_err_probe(soc->dev, -EINVAL,
>  						     "%pOF: duplicate GPIO bank\n",
>  						     np);
>  		}
>  
>  		bank = &soc->banks[soc->bank_num];
>  		bank->soc = soc;
>  		bank->pin_base = args.args[1];
>  		bank->hw_id = hw_id;
>  		bank->gc.ngpio = args.args[2];
>  		bank->gc.fwnode = of_fwnode_handle(np);

[Severity: High]
The for_each_available_child_of_node_scoped() loop automatically drops the
reference to the np node when it goes out of scope.

Will assigning it to bank->gc.fwnode without taking an additional reference
via of_node_get() lead to a use-after-free later?

[ ... ]

> @@ -1300,9 +1300,9 @@ static int amb_pinctrl_probe(struct platform_device *pdev)
>  	for (i = 0; i < soc->bank_num; i++) {
>  		unsigned int pin;
>  
>  		for (pin = soc->banks[i].pin_base;
>  		     pin < soc->banks[i].pin_base + soc->banks[i].gc.ngpio;
>  		     pin++)
>  			clear_bit(pin, soc->used);
>  	}
>  
>  	if (soc->data->clk_au_dedicated_pin >= gpio_pins &&
>  	    soc->data->clk_au_dedicated_pin < AMBA_MAX_PINS)
>  		clear_bit(soc->data->clk_au_dedicated_pin, soc->used);

[Severity: Medium]
If the device tree contains sparse, non-contiguous GPIO banks, the dedicated
pin might fall into a "hole" (a missing bank) that has an ID less than
the highest pin ID returned by amb_gpio_pins_end(soc).

If this happens, will gpio_pins incorrectly skip this dedicated pin, leaving
it marked busy and preventing it from being suspended?

[ ... ]

> @@ -1390,15 +1390,15 @@ static int amb_pinctrl_suspend_noirq(struct device *dev)
>  	struct amb_pinctrl_soc_data *soc = dev_get_drvdata(dev);
>  	u32 bank, hw, i;
>  
>  	for (i = 0; i < soc->bank_num; i++) {
>  		hw = soc->banks[i].hw_id;
>  
>  		regmap_read(soc->pull_regmap, soc->data->pull_en[hw], &soc->pm[i].pull[0]);
>  		regmap_read(soc->pull_regmap, soc->data->pull_dir[hw], &soc->pm[i].pull[1]);
>  
>  		regmap_read(soc->ds_regmap, soc->data->ds0[hw], &soc->pm[i].ds[0]);

[Severity: High]
The amb_pinctrl_suspend_noirq() callback executes with hardirqs disabled.
Will reading the syscon regmaps here attempt to acquire a sleepable rt_mutex
on PREEMPT_RT, triggering a "sleeping in atomic context" BUG?

>  		regmap_read(soc->ds_regmap, soc->data->ds1[hw], &soc->pm[i].ds[1]);
>  		if (soc->data->have_ds2)
>  			regmap_read(soc->ds_regmap, soc->data->ds2[hw], &soc->pm[i].ds[2]);
>  
>  		soc->pm[i].iomux[0] = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(hw, 0));
>  		soc->pm[i].iomux[1] = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(hw, 1));
>  		soc->pm[i].iomux[2] = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(hw, 2));

[Severity: High]
Similar to amb_gpio_irq_enable(), is this missing a check to ensure
hsm_domain_id == 0? Could these IOMUX reads trigger a bus fault in
non-safety domains?

[ ... ]

> @@ -1404,9 +1404,9 @@ static int amb_pinctrl_suspend_noirq(struct device *dev)
>  		soc->pm[i].is = readl_relaxed(soc->banks[i].base + GPIO_IS_OFFSET);
>  		soc->pm[i].ibe = readl_relaxed(soc->banks[i].base + GPIO_IBE_OFFSET);
>  		soc->pm[i].iev = readl_relaxed(soc->banks[i].base + GPIO_IEV_OFFSET);
>  		soc->pm[i].ie = readl_relaxed(soc->banks[i].base + GPIO_IE_OFFSET);
>  		soc->pm[i].mask = readl_relaxed(soc->banks[i].base + GPIO_MASK_OFFSET);
>  		writel_relaxed(0xffffffff, soc->banks[i].base + GPIO_MASK_OFFSET);
>  		soc->pm[i].data = readl_relaxed(soc->banks[i].base + GPIO_DATA_OFFSET);
>  
>  		if (soc->banks[i].irq_wake_mask)
>  			writel_relaxed(soc->banks[i].irq_wake_mask,
>  				       soc->banks[i].base + GPIO_IE_OFFSET);

[Severity: Medium]
The generic IRQ core already masks non-wake interrupts during suspend,
leaving wake and IRQF_NO_SUSPEND interrupts enabled.

Does unconditionally overwriting GPIO_IE_OFFSET with the irq_wake_mask here
unintentionally disable any configured IRQF_NO_SUSPEND interrupts?

[ ... ]

> @@ -1452,9 +1452,9 @@ static int amb_pinctrl_resume_noirq(struct device *dev)
>  		if (soc->data->have_ds2)
>  			regmap_write(soc->ds_regmap, soc->data->ds2[hw], soc->pm[i].ds[2]);
>  
>  		writel_relaxed(soc->pm[i].iomux[0], soc->iomux_base + IOMUX_OFFSET(hw, 0));
>  		writel_relaxed(soc->pm[i].iomux[1], soc->iomux_base + IOMUX_OFFSET(hw, 1));
>  		writel_relaxed(soc->pm[i].iomux[2], soc->iomux_base + IOMUX_OFFSET(hw, 2));
>  
>  		writel_relaxed(soc->pm[i].afsel, soc->banks[i].base + GPIO_AFSEL_OFFSET);
>  		writel_relaxed(soc->pm[i].dir, soc->banks[i].base + GPIO_DIR_OFFSET);
>  		/* Expose DATA writes while restoring the saved GPIO state. */
>  		writel_relaxed(0xffffffff,
>  			       soc->banks[i].base + GPIO_MASK_OFFSET);
>  		writel_relaxed(soc->pm[i].data,
>  			       soc->banks[i].base + GPIO_DATA_OFFSET);

[Severity: Medium]
The direction is restored before the data is written back. If a pin is
configured as an output driving HIGH, will setting its direction to output
while the data register still holds a default 0 momentarily drive the pin
LOW, causing a hardware glitch during resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-longzhao-upstream-cv75-v2-v2-0-6b09707c5fe9@ambarella.com?part=7