Re: [PATCH v2] pinctrl: sunxi: request the per-bank supply optionally

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pinctrl: sunxi: unbalanced bank regulator refcount in sunxi_pmx_request()
--

commit 387a0217944495d51d6191636af797bf5c7e5da5
Author: Yang Siyuan <hsyysy@163.com>

pinctrl: sunxi: request the per-bank supply optionally

This commit switches sunxi_pmx_request() from regulator_get() to
regulator_get_optional() so that banks without a per-bank supply in the
device tree do not trigger a dummy regulator warning. When
regulator_get_optional() returns -ENODEV, sunxi_pmx_request() returns 0
with s_reg->regulator left as NULL, and sunxi_pmx_free() skips cleanup when
s_reg->regulator is NULL.

> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 31cd142ce0f79..f6c48a287a0e4 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -925,11 +925,19 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
>  	}
>  
>  	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
> -	reg = regulator_get(pctl->dev, supply);
> -	if (IS_ERR(reg))
> +	reg = regulator_get_optional(pctl->dev, supply);
> +	if (IS_ERR(reg)) {
> +		/*
> +		 * The per-bank supply is optional: boards that do not
> +		 * describe it simply have no regulator to manage.
> +		 */
> +		if (PTR_ERR(reg) == -ENODEV)
> +			return 0;
> +
>  		return dev_err_probe(pctl->dev, PTR_ERR(reg),
>  				     "Couldn't get bank P%c regulator\n",
>  				     'A' + bank);
> +	}
>  
>  	ret = regulator_enable(reg);
>  	if (ret) {
> @@ -959,6 +967,10 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
>  					    PINS_PER_BANK;
>  	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
>  
> +	/* The per-bank supply is optional, so this bank may not have one. */
> +	if (!s_reg->regulator)
> +		return 0;

[Severity: High]
Can leaving s_reg->regulator as NULL on -ENODEV cause s_reg->refcount to
become unbalanced and disable the bank regulator while other pins in the
same bank are still active?

In sunxi_pmx_request(), returning 0 on -ENODEV without recording that the
bank has no supply or incrementing s_reg->refcount means every subsequent
pin request in the same bank will call regulator_get_optional() again.

Inside regulator_get_optional() -> _regulator_get() ->
regulator_dev_lookup(), when the pinctrl device tree node does not have a
vcc-pX-supply property, regulator_dt_lookup() returns NULL and
regulator_dev_lookup() falls back to a global name lookup:

drivers/regulator/core.c:regulator_dev_lookup() {
	/* first do a dt based lookup */
	r = regulator_dt_lookup(dev, supply);
	if (r)
		return r;
...
	r = regulator_lookup_by_name(supply);
	if (r)
		return r;

	return ERR_PTR(-ENODEV);
}

On several Allwinner boards (for example,
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi and
sun50i-a64-pine64.dts), vcc-pl-supply is omitted from r_pio to avoid a
circular dependency with the PMIC bus on PL0/PL1, while the PMIC regulator
node defines regulator-name = "vcc-pl":

	&r_pio {
		/*
		 * FIXME: We can't add that supply for now since it would
		 * create a circular dependency between pinctrl, the regulator
		 * and the RSB Bus.
		 *
		 * vcc-pl-supply = <&reg_aldo2>;
		 */
	};
	...
	&reg_aldo2 {
		regulator-always-on;
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
		regulator-name = "vcc-pl";
	};

When the PMIC bus driver probes before the PMIC and requests PL0/PL1,
regulator_get_optional(pctl->dev, "vcc-pl") returns -ENODEV, leaving
s_reg->regulator as NULL and s_reg->refcount at 0 for those active pins.

Once the PMIC registers the regulator named "vcc-pl", a later request for
another pin in bank PL sees s_reg->regulator == NULL, calls
regulator_get_optional(pctl->dev, "vcc-pl") again, matches the regulator via
regulator_lookup_by_name("vcc-pl"), and initializes s_reg->refcount to 1:

drivers/pinctrl/sunxi/pinctrl-sunxi.c:sunxi_pmx_request() {
	...
	s_reg->regulator = reg;
	refcount_set(&s_reg->refcount, 1);
}

If that later pin (or one of the earlier pins requested before the PMIC
probed) is subsequently freed in sunxi_pmx_free(), s_reg->regulator is now
non-NULL, so refcount_dec_and_test(&s_reg->refcount) drops the refcount to 0
and calls regulator_disable() and regulator_put() while other pins in the
bank remain requested.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026152027.1417178-1-hsyysy@163.com?part=1