Re: [PATCH 02/12] pinctrl: airoha: configure pin muxes on GPIO requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] pinctrl: airoha: pinmux ownership bypass and mux_setting
  corruption with function_is_gpio()
- [Medium] pinctrl: airoha: false -EBUSY conflict in
  airoha_pinmux_check_conflicts()
--

commit 9a9749c74aba6ad54ceaf9a1debe606913f398d5
Author: Zhengping Zhang <zhengping.zhang@airoha.com>

pinctrl: airoha: configure pin muxes on GPIO requests

This commit adds a gpio_request_enable callback to configure pin muxes when
a GPIO is requested and introduces optional conflict detection for shared
mux bits across pin groups and GPIO owners.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 8dd0881bba4ad..25562e51ebbdc 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -400,57 +400,236 @@ static int airoha_pinctrl_add_gpiochip(struct airoha_pinctrl *pinctrl,

[ ... ]

> +/* Shared mux bits can affect pads outside the requested pin group. */
> +static bool
> +airoha_pinmux_groups_conflict(const struct airoha_pinctrl_func_group *a,
> +			     const struct airoha_pinctrl_func_group *b)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < a->regmap_size; i++) {
> +		for (j = 0; j < b->regmap_size; j++) {
> +			u32 mask;
>  
> -		group = &func->groups[i];
> -		if (strcmp(group->name, grp->grp.name))
> +			if ((a->regmap[i].mux == AIROHA_FUNC_MUX) !=
> +			    (b->regmap[j].mux == AIROHA_FUNC_MUX) ||
> +			    a->regmap[i].offset != b->regmap[j].offset)
> +				continue;
> +
> +			mask = a->regmap[i].mask & b->regmap[j].mask;
> +			if (mask & (a->regmap[i].val ^ b->regmap[j].val))
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}

[ ... ]

> +static int
> +airoha_pinmux_check_conflicts(struct pinctrl_dev *pctldev,
> +			     const struct airoha_pinctrl_func_group *group)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < pctldev->desc->npins; i++) {
> +		const struct airoha_pinctrl_func_group *active;
> +		struct pinctrl_setting_mux setting;
> +		unsigned int number = pctldev->desc->pins[i].number;
> +		struct pin_desc *pin;
> +		bool claimed, gpio_owned;
> +
> +		pin = pin_desc_get(pctldev, number);
> +		if (!pin)
> +			continue;
> +
> +		scoped_guard(mutex, &pin->mux_lock) {
> +			claimed = pin->mux_usecount && pin->mux_setting;
> +			gpio_owned = !!pin->gpio_owner;
> +			if (claimed)
> +				setting = *pin->mux_setting;
> +		}
> +
> +		/* GPIO requests need no explicit DTS pinctrl state. */
> +		if (gpio_owned) {
> +			active = airoha_pinmux_get_gpio_group(pctldev, number);
> +			if (!active)
> +				return -EOPNOTSUPP;
> +			if (airoha_pinmux_groups_conflict(group, active))
> +				goto conflict;
> +		}
> +
> +		if (!claimed)
> +			continue;
> +		active = airoha_pinmux_get_func_group(pctldev, setting.func,
> +						    setting.group);
> +		if (!active)
> +			return -EINVAL;
> +		if (!airoha_pinmux_groups_conflict(group, active))
> +			continue;
> +
> +conflict:
> +		dev_err(pctldev->dev,
> +			"group %s conflicts with shared mux bits of group %s\n",
> +			group->name, active->name);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does airoha_pinmux_check_conflicts() reject valid two-group HSUART
configurations (groups = "hsuart", "hsuart_cts_rts") with -EBUSY on SoCs
that enable check_mux_conflicts?

On AN7581 and AN7583, hsuart_pins contains only the TX/RX pins and
hsuart_cts_rts_pins contains only the CTS/RTS pins, so enabling HSUART with
hardware flow control requires selecting both groups. However, in
uart_func_group, the "hsuart" entry includes GPIO_HSUART_CTS_RTS_MODE_MASK
in regmap[0].mask with val = GPIO_HSUART_MODE_MASK, whereas
"hsuart_cts_rts" sets the same mask with val = GPIO_HSUART_MODE_MASK |
GPIO_HSUART_CTS_RTS_MODE_MASK. When pinctrl_commit_state() enables the
second group after claiming the first, airoha_pinmux_groups_conflict() sees
differing values on GPIO_HSUART_CTS_RTS_MODE_MASK and fails the state
transition with -EBUSY.

[ ... ]

> +static int airoha_pinmux_set_mux(struct pinctrl_dev *pctldev,
> +			       unsigned int selector, unsigned int group)
> +{
> +	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct airoha_pinctrl_func_group *config;
> +	int i, err;
> +
> +	config = airoha_pinmux_get_func_group(pctldev, selector, group);
> +	if (!config)
> +		return -EINVAL;
> +
> +	guard(mutex)(&pinctrl->mux_lock);
> +	if (pinctrl->check_mux_conflicts) {
> +		struct group_desc *grp;
> +
> +		/* The core permits GPIO-category muxes alongside GPIO owners. */
> +		grp = pinctrl_generic_get_group(pctldev, group);
> +		if (!pinmux_generic_function_is_gpio(pctldev, selector)) {
> +			for (i = 0; i < grp->grp.npins; i++) {
> +				struct pin_desc *pin;
> +
> +				pin = pin_desc_get(pctldev, grp->grp.pins[i]);
> +				if (!pin)
> +					return -EINVAL;
> +				scoped_guard(mutex, &pin->mux_lock) {
> +					if (pin->gpio_owner)
> +						return -EBUSY;
> +				}
> +			}
> +		}
> +
> +		err = airoha_pinmux_check_conflicts(pctldev, config);
> +		if (err)
> +			return err;
>  	}
>  
> -	return -EINVAL;
> +	dev_dbg(pctldev->dev, "enable function %s group %s\n",
> +		pinmux_generic_get_function_name(pctldev, selector),
> +		config->name);
> +
> +	return airoha_pinmux_apply_group(pinctrl, config);
>  }
>  
>  static int airoha_pinmux_set_direction(struct pinctrl_dev *pctrl_dev,
> @@ -468,6 +647,8 @@ static int airoha_pinmux_set_direction(struct pinctrl_dev *pctrl_dev,
>  }
>  
>  static const struct pinmux_ops airoha_pmxops = {
> +	.gpio_request_enable = airoha_gpio_request_enable,
> +	.function_is_gpio = pinmux_generic_function_is_gpio,

[Severity: Medium]
Can setting .function_is_gpio allow a second consumer to bypass pinmux
ownership and corrupt pin->mux_setting when a pin is already claimed by an
explicit GPIO-category pinctrl state?

When a pin's active pin->mux_setting has PINFUNCTION_FLAG_GPIO (via
PINCTRL_GPIO_FUNC_DESC), a second consumer calling pinmux_enable_setting()
on a group containing that pin causes pin_request() to set gpio_ok = true,
skip the mux_owner check, and increment pin->mux_usecount to 2:

drivers/pinctrl/pinmux.c:pin_request() {
    ...
	if (gpio_range || ops->strict) {
		if (desc->mux_usecount && !gpio_ok) {
			dev_err(pctldev->dev,
				"pin %s already requested by %s; cannot claim for %s\n",
				desc->name, desc->mux_owner, owner);
			goto out;
		}
    ...
}

pinmux_enable_setting() then overwrites pin->mux_setting with the new
setting before calling airoha_pinmux_set_mux(). Because
airoha_pinmux_set_mux() only checks pin->gpio_owner, if the pin has no GPIO
descriptor owner, airoha_pinmux_check_conflicts() sees the already
overwritten pin->mux_setting and overwrites the first device's hardware mux
without detecting the conflict. Conversely, if airoha_pinmux_set_mux()
returns -EBUSY, pinmux_enable_setting()'s error path clears
pin->mux_setting to NULL while pin_free() only decrements pin->mux_usecount
to 1, hiding the first owner's active mux from subsequent conflict checks
and preventing pinmux_disable_setting() from releasing the pin.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110094339.749110-1-zhengping.zhang@airoha.com?part=2