Re: [PATCH v11 23/37] pinctrl: airoha: statically allocate gpio regs structure

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

New issues:
- [Critical] Refactoring pinctrl callbacks to use gpio_chip callbacks introduces a NULL pointer dereference during probe if device tree hogs are present.
- [Medium] Removing calls to `pinctrl_gpio_direction_input` and `pinctrl_gpio_direction_output` breaks pinctrl core synchronization.

Pre-existing issues:
- [High] The IRQ handler performs an unconditional W1C (write-1-to-clear) on the status register after calling `generic_handle_domain_irq()`, which causes lost edge interrupts.
--

--- Patch [23]: [PATCH v11 23/37] pinctrl: airoha: statically allocate gpio regs structure ---
commit c84002cfd05f8b4af2f361901c228a9a5666724b
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: statically allocate gpio regs structure

This commit refactors the airoha pinctrl driver to collect all GPIO register
information into a single statically allocated structure.

> 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

[ ... ]

> @@ -2547,7 +2556,8 @@ static int airoha_gpio_set_direction(struct gpio_chip *chip, unsigned int gpio,
>  	/* set output enable */
>  	mask = BIT(gpio % AIROHA_PIN_BANK_SIZE);
>  	index = gpio / AIROHA_PIN_BANK_SIZE;

[Severity: Critical]
Does this code introduce a NULL pointer dereference during probe if device
tree hogs are present?

If pinctrl_enable() is called in airoha_pinctrl_probe() before
airoha_pinctrl_add_gpiochip() initializes the gpio_chip, applying DT hogs
will trigger pinctrl callbacks like airoha_pinmux_set_direction().

These callbacks delegate to airoha_gpio_set_direction(), which retrieves
the pinctrl context using gpiochip_get_data(chip):

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_gpio_set_direction() {
	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);
	...
}

Since chip->gpiodev is not allocated until gpiochip_add_data() is called,
dereferencing chip->gpiodev->data during gpiochip_get_data() results in a
kernel panic.

> -	err = regmap_update_bits(pinctrl->regmap, pinctrl->gpiochip.out[index],
> +	err = regmap_update_bits(pinctrl->regmap,
> +				 pinctrl->gpio_regs->out[index],
>  				 mask, !input ? mask : 0);
>  	if (err)
>  		return err;
> @@ -2557,7 +2567,7 @@ static int airoha_gpio_set_direction(struct gpio_chip *chip, unsigned int gpio,
>  	index = gpio / AIROHA_REG_GPIOCTRL_NUM_PIN;
>  
>  	return regmap_update_bits(pinctrl->regmap,
> -				  pinctrl->gpiochip.dir[index], mask,
> +				  pinctrl->gpio_regs->dir[index], mask,
>  				  !input ? mask : 0);
>  }

[Severity: Medium]
Does the omission of pinctrl_gpio_direction_input() and
pinctrl_gpio_direction_output() break pinctrl core synchronization?

When a consumer driver requests a pin direction change via gpiolib (e.g.,
gpiod_direction_input()), the driver's airoha_gpio_direction_input() callback
bypasses pinctrl_gpio_direction_input() and directly configures the hardware
register:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_gpio_direction_input() {
	return airoha_gpio_set_direction(chip, gpio, true);
}

This leaves the pinctrl core unaware of the change, leading to stale states
in debugfs and bypassed pinctrl locking and semantics.

[ ... ]

> @@ -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));
> +				     pinctrl->gpio_regs->status[i], BIT(irq));

[Severity: High]
This is a pre-existing issue, but does this unconditional clear operation
cause lost edge-triggered interrupts?

When an edge interrupt triggers, handle_edge_irq() handles it by first
calling irq_ack() (implemented via airoha_irq_ack()) to clear the status
bit. It then runs the child ISR.

If a new edge occurs during the child ISR execution, the hardware correctly
latches the new interrupt in the status register. However, when control
returns to airoha_irq_handler(), it executes this redundant regmap_write()
to clear the status bit again, which destroys the newly latched interrupt
before it can be handled.

>  		}
>  		handled |= !!status;
>  	}

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