Re: [PATCH v11 16/37] pinctrl: airoha: add set_direction() helper for gpio_chip

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added `airoha_gpio_get_direction` and `airoha_gpio_set_direction` helpers introduce a fatal NULL pointer dereference if pin configurations are applied during `pinctrl_enable`.
--

--- Patch [16]: [PATCH v11 16/37] pinctrl: airoha: add set_direction() helper for gpio_chip ---
commit e29fc714c63ce8cf69a2d201c9cfc30258714834
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: add set_direction() helper for gpio_chip

This patch adds a set_direction() helper for the gpio_chip abstraction and
refactors existing functions to use it, intending no functional changes.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index df1694296046c..e4ccf357df892 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c

[ ... ]

> @@ -2539,12 +2539,42 @@ static int airoha_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
>  	return val & mask ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
>  }
>  
> +static int airoha_gpio_set_direction(struct gpio_chip *chip, unsigned int gpio,
> +				     bool input)
> +{
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(chip);

[Severity: High]
Will this result in a NULL pointer dereference during device probe if pinctrl
hogs are defined?

If the device tree defines hogs, they are applied when pinctrl_enable() is
called during probe. Looking at airoha_pinctrl_probe():

airoha_pinctrl_probe() {
	...
	err = pinctrl_enable(pinctrl->ctrl);
	...
	return airoha_pinctrl_add_gpiochip(pinctrl, pdev);
}

When pinctrl_enable() executes, it ends up calling the newly modified
airoha_pinmux_set_direction(), which delegates to airoha_gpio_set_direction().

At this stage, airoha_pinctrl_add_gpiochip() has not yet run, meaning
devm_gpiochip_add_data() hasn't been called, and chip->gpiodev is still NULL.

Because gpiochip_get_data() dereferences chip->gpiodev->data, can this lead
to a crash on boot? Could the pinctrl context be derived using container_of()
instead, or perhaps the gpiochip could be registered before enabling pinctrl?

> +	u32 mask, index;
> +	int err;

[ ... ]

> @@ -2761,27 +2791,13 @@ static int airoha_pinmux_set_direction(struct pinctrl_dev *pctrl_dev,
>  					unsigned int p, bool input)
>  {
>  	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> -	u32 mask, index;
> -	int err, pin;
> +	int pin;
>  
>  	pin = airoha_convert_pin_to_reg_offset(pctrl_dev, range, p);
>  	if (pin < 0)
>  		return pin;
>  
> -	/* set output enable */
> -	mask = BIT(pin % AIROHA_PIN_BANK_SIZE);
> -	index = pin / AIROHA_PIN_BANK_SIZE;
> -	err = regmap_update_bits(pinctrl->regmap, pinctrl->gpiochip.out[index],
> -				 mask, !input ? mask : 0);
> -	if (err)
> -		return err;
> -
> -	/* set direction */
> -	mask = BIT(2 * (pin % AIROHA_REG_GPIOCTRL_NUM_PIN));
> -	index = pin / AIROHA_REG_GPIOCTRL_NUM_PIN;
> -	return regmap_update_bits(pinctrl->regmap,
> -				  pinctrl->gpiochip.dir[index], mask,
> -				  !input ? mask : 0);
> +	return airoha_gpio_set_direction(&pinctrl->gpiochip.chip, pin, input);
>  }

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