Re: [PATCH v10 15/36] 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] `airoha_gpio_set_direction()` uses `gpiochip_get_data()` on an unregistered `gpio_chip`, causing a NULL pointer dereference during early pinctrl operations (e.g., when applying pinctrl hogs).
--

--- Patch [15]: [PATCH v10 15/36] pinctrl: airoha: add set_direction() helper for gpio_chip ---
commit 33e55fe74353aced7ea2a88787d4738053466b24
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: add set_direction() helper for gpio_chip

This commit adds a set_direction() helper for the gpio_chip abstraction
to consolidate GPIO direction configuration. It updates several functions
to use this newly defined helper instead of duplicating the logic.

> 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]
Does this code dereference a NULL pointer during early pinctrl initialization?

If airoha_pinmux_set_direction() is called while applying device tree pinctrl
hogs (which happens in airoha_pinctrl_probe() before the gpiochip is
registered via airoha_pinctrl_add_gpiochip()), the chip->gpiodev pointer
will be NULL. Calling gpiochip_get_data() here will unconditionally
dereference that NULL pointer.

Would it be safer to retrieve the airoha_pinctrl struct using container_of()
on the embedded gpio_chip instead?

> +	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);

[Severity: High]
Will this pass an unregistered gpio_chip into the new helper when applying
pinctrl hogs for output or input direction during probe?

>  }
>  
>  static const struct pinmux_ops airoha_pmxops = {

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