Re: [PATCH v10 22/36] pinctrl: airoha: statically allocate gpio regs structure

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

New issues:
- [Low] The statically allocated `airoha_gpiochip_regs` structure is not marked as `const`.

Pre-existing issues:
- [High] Redundant clearing of the interrupt status bit in `airoha_irq_handler` creates a race condition for edge-triggered interrupts, potentially causing them to be lost.
- [High] Interrupt handler is requested before the gpiochip is fully initialized, creating a window for a Use-After-Free of the irqdomain during device teardown.
--

--- Patch [22]: [PATCH v10 22/36] pinctrl: airoha: statically allocate gpio regs structure ---
commit 9c7301e7610a12f2a7f33d437ec5e58ef02929f3
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: statically allocate gpio regs structure

This commit refactors the Airoha pinctrl driver to collect all GPIO register
arrays into a single statically allocated structure. The driver is updated
to reference this new structure via a pointer, simplifying the gpiochip setup.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 8e418f27fd8bc..e8d5252b17f30 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -339,6 +350,15 @@ static const u32 irq_edge_regs[] = {
>  	REG_GPIO_INT_EDGE3
>  };
>  
> +static struct airoha_gpiochip_regs airoha_gpiochip_regs = {

[Severity: Low]
Could this structure be marked as const? It appears to only serve as a
read-only lookup table of pointer arrays, so marking it const would move
it to the read-only .rodata section.

> +	.data = gpio_data_regs,
> +	.dir = gpio_dir_regs,
> +	.out = gpio_out_regs,
> +	.status = irq_status_regs,
> +	.level = irq_level_regs,
> +	.edge = irq_edge_regs,
> +};
> +
[ ... ]
> @@ -2698,7 +2708,7 @@ static irqreturn_t airoha_irq_handler(int irq, void *data)
>  
>  			generic_handle_domain_irq(girq->domain, offset);
>  			regmap_write(pinctrl->regmap,
> -				     pinctrl->gpiochip.status[i], BIT(irq));

[Severity: High]
This is a pre-existing issue, but does this regmap_write() to clear the status
bit create a race condition for edge-triggered interrupts?

The core IRQ subsystem already clears the hardware interrupt status bit before
invoking the flow handler via airoha_irq_ack(). If a new edge-triggered
interrupt arrives on the same pin while generic_handle_domain_irq() is
executing, the hardware will latch the new event.

Would clearing the status bit a second time here wipe out the newly latched
interrupt before it can be processed?

> +				     pinctrl->gpio_regs->status[i], BIT(irq));
>  		}
>  		handled |= !!status;
>  	}
> @@ -2719,8 +2729,7 @@ static const struct irq_chip airoha_gpio_irq_chip = {
>  static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,
>  					struct platform_device *pdev)
>  {
> -	struct airoha_pinctrl_gpiochip *chip = &pinctrl->gpiochip;
> -	struct gpio_chip *gc = &chip->chip;
> +	struct gpio_chip *gc = &pinctrl->gpiochip;

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
risk during device teardown based on the initialization order in this function?

Further down in airoha_pinctrl_add_gpiochip(), devm_request_irq() is called
before devm_gpiochip_add_data():

	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
				dev_name(dev), pinctrl);
	if (err) {
		dev_err(dev, "error requesting irq %d: %d\n", irq, err);
		return err;
	}

	return devm_gpiochip_add_data(dev, gc, pinctrl);

Since devres cleans up in reverse order, devm_gpiochip_add_data() will be
undone first, destroying the irq domain while the shared interrupt handler
remains active. If a shared interrupt fires during this window,
airoha_irq_handler() will pass a freed girq->domain to
generic_handle_domain_irq().

Should devm_request_irq() be called after devm_gpiochip_add_data() to ensure
safe teardown?

>  	struct gpio_irq_chip *girq = &gc->irq;
>  	struct device *dev = &pdev->dev;
>  	int irq, err;

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