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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Applying device tree hogs during `pinctrl_enable()` causes a kernel panic due to a NULL pointer dereference.
- [High] Double-ACK of W1C interrupt status register leads to lost edge interrupts.
- [Low] The statically allocated 'airoha_gpiochip_regs' structure is not marked as const.
--

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

pinctrl: airoha: statically allocate gpio regs structure
    
just a small refactoring to collect all gpio register information
in the one statically allocated structure.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 3dc9b3cb0051f..c4d16c5636ef8 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 = {
> +	.data = gpio_data_regs,
> +	.dir = gpio_dir_regs,
> +	.out = gpio_out_regs,
> +	.status = irq_status_regs,
> +	.level = irq_level_regs,
> +	.edge = irq_edge_regs,
> +};
> +

[Severity: Low]
Should this statically allocated structure be marked as const?

Failing to mark both the static structure and the corresponding gpio_regs
pointer in struct airoha_pinctrl as const prevents the compiler from
placing this read-only data in the .rodata section.

[ ... ]

> @@ -411,7 +418,8 @@ struct airoha_pinctrl {
>  	struct regmap *chip_scu;
>  	struct regmap *regmap;
>  
> -	struct airoha_pinctrl_gpiochip gpiochip;
> +	struct gpio_chip gpiochip;
> +	struct airoha_gpiochip_regs *gpio_regs;
>  };

[Severity: Low]
Should this gpio_regs pointer field also be marked as pointing to a const
structure for the same reason?

[ ... ]

> @@ -2683,12 +2693,12 @@ static irqreturn_t airoha_irq_handler(int irq, void *data)
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(irq_status_regs); i++) {
> -		struct gpio_irq_chip *girq = &pinctrl->gpiochip.chip.irq;
> +		struct gpio_irq_chip *girq = &pinctrl->gpiochip.irq;
>  		u32 regmap;
>  		unsigned long status;
>  		int irq;
>  
> -		if (regmap_read(pinctrl->regmap, pinctrl->gpiochip.status[i],
> +		if (regmap_read(pinctrl->regmap, pinctrl->gpio_regs->status[i],
>  				&regmap))
>  			continue;
>  
> @@ -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));
>  		}
>  		handled |= !!status;
>  	}

[Severity: High]
Can this explicit W1C write clear newly latched interrupts that arrive
during generic_handle_domain_irq() and lose them?

An earlier patch in this series added airoha_irq_ack as the .irq_ack
callback for airoha_gpio_irq_chip, which handles acking by writing to the
W1C (write-one-to-clear) interrupt status register.

For edge-triggered interrupts, the IRQ core calls irq_ack before invoking
the device's interrupt handler. If a new edge interrupt arrives while the
handler is running, the hardware will latch the status bit again.

Will leaving this redundant manual W1C register write unconditionally clear
this newly set status bit without handling the interrupt?

[ ... ]

> @@ -2820,7 +2829,7 @@ static int airoha_pinmux_set_direction(struct pinctrl_dev *pctrl_dev,
>  	if (pin < 0)
>  		return pin;
>  
> -	return airoha_gpio_set_direction(&pinctrl->gpiochip.chip, pin, input);
> +	return airoha_gpio_set_direction(&pinctrl->gpiochip, pin, input);
>  }

[Severity: Critical]
Does this sequence cause a kernel panic due to a NULL pointer dereference
during probe?

If the device tree contains pinctrl-0 hogs with input-enable or output-enable,
it triggers airoha_pinconf_set() when pinctrl_enable() is called in
airoha_pinctrl_probe().

airoha_pinconf_set() calls airoha_pinmux_set_direction(), which uses this
airoha_gpio_set_direction() helper.

Because pinctrl_enable() is called before airoha_pinctrl_add_gpiochip(),
chip->gpiodev is still NULL. When airoha_gpio_set_direction() calls:

    struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);

Will this return NULL and lead to an immediate dereference when accessing
pinctrl->regmap?

This regression appears to have been introduced earlier in the series when
the set_direction helper replaced safe direct driver data retrieval.

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