Re: [RFC PATCH] pinctrl: aspeed: Handle GPIOZ6 pass-through strap on GPIO requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pinctrl: aspeed: clobbering shared SCU51C[9] strap bit breaks pass-through
- [High] pinctrl: aspeed: unconditionally rejecting GPIO requests based on SCU510[28]
- [Medium] pinctrl: aspeed: missing hardware mux rollback on error path
--

commit fc751bac2dfdee809f65ba4da118ede6fdbe6aaf
Author: Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>

pinctrl: aspeed: Handle GPIOZ6 pass-through strap on GPIO requests

This commit introduces an AST2600-specific GPIO request callback to handle
the GPIOZ6 pass-through strap in SCU51C[9] for GPIOP0-P5. It clears this
shared strap bit to allow host power control on boards like the B650D4U,
while also explicitly rejecting requests when SCU510[28] is set.

> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 85d6640ff4c4e..6e58feb48bf59 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c

[ ... ]

> @@ -2743,12 +2744,56 @@ static struct aspeed_pinctrl_data aspeed_g6_pinctrl_data = {
> +static int aspeed_g6_gpio_request_enable(struct pinctrl_dev *pctldev,
> +					 struct pinctrl_gpio_range *range,
> +					 unsigned int offset)
> +{
> +	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> +	struct regmap *scu = pdata->pinmux.maps[ASPEED_IP_SCU];
> +	unsigned int strap2, strap3;
> +	int ret;
> +
> +	if (offset < AB22 || offset > AB23)
> +		return aspeed_gpio_request_enable(pctldev, range, offset);
> +
> +	ret = regmap_read(scu, SCU510, &strap2);
> +	if (ret)
> +		return ret;
> +	if (strap2 & BIT(28))
> +		return -EOPNOTSUPP;

[Severity: High]
Does this unconditionally reject standard GPIO requests based on an unhandled
strap condition? 

Systems where the SCU510[28] strap is set will no longer be able to request
GPIOP0-P5 as standard GPIOs. Any standard GPIO consumer on such hardware will
fail, breaking backwards compatibility.

> +
> +	ret = aspeed_gpio_request_enable(pctldev, range, offset);
> +	if (ret)
> +		return ret;
> +
> +	/* In dedicated-strap mode, SCU51C[9] does not enable pass-through. */
> +	if (strap2 & BIT(30))
> +		return 0;
> +
> +	ret = regmap_update_bits(scu, SCU51C, BIT(9), 0);

[Severity: High]
Does this clear a shared hardware enable bit for all three pairs without
refcounting? 

The SCU51C[9] bit globally controls the pass-through feature for three
distinct pin pairs (GPIOP0/P1, GPIOP2/P3, and GPIOP4/P5). Modifying it
unconditionally for a single pin request might destructively clobber
concurrent pass-through usage for the other unconnected pins.

> +	if (ret)
> +		return ret;

[Severity: Medium]
If regmap_update_bits() fails here, does this return an error without
unwinding the active pinmux modification made earlier by
aspeed_gpio_request_enable()?

This could leave the pin's hardware multiplexer configured for GPIO, while
the pinctrl subsystem considers the request failed and the pin unreserved,
creating a state mismatch between hardware and software.

> +	ret = regmap_read(scu, SCU51C, &strap3);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Similarly, if this regmap_read() fails, does this also leak the hardware mux
state?

> +	if (strap3 & BIT(9))
> +		return -EPERM;

[Severity: Medium]
Could this error path also miss unwinding the hardware mux state from
aspeed_gpio_request_enable()?

> +
> +	return 0;
> +}
> +
>  static const struct pinmux_ops aspeed_g6_pinmux_ops = {
>  	.get_functions_count = aspeed_pinmux_get_fn_count,
>  	.get_function_name = aspeed_pinmux_get_fn_name,
>  	.get_function_groups = aspeed_pinmux_get_fn_groups,
>  	.set_mux = aspeed_pinmux_set_mux,
> -	.gpio_request_enable = aspeed_gpio_request_enable,
> +	.gpio_request_enable = aspeed_g6_gpio_request_enable,

[Severity: High]
Since this structure lacks a .gpio_disable_free callback, is there any
mechanism to ensure the shared strap modification is reversed upon release?

Without it, the SCU51C[9] bit is never restored when the GPIO is freed,
which could cause a permanent system-wide functional loss for pass-through
features until a reboot.

>  	.strict = true,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015035118.9495-1-prasanth.padarthi10@gmail.com?part=1