Re: [PATCH] power: sequencing: pcie-m2: Drive the Key E SDIO_RESET# signal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pwrseq: pcie-m2: regulator enable leak in pwrseq_pcie_m2_vregs_enable()
- [Medium] pwrseq: pcie-m2: missing rollback on error in pwrseq_pcie_m2_vregs_disable()
--

commit e3e29ad909cb95485a7ebf63430d7d702095bb39
Author: William Bright <william.bright@imd-tec.com>

power: sequencing: pcie-m2: Drive the Key E SDIO_RESET# signal

This commit requests the optional sdio-reset-gpios line asserted during probe,
deasserts it after enabling the M.2 connector supplies, and asserts it again
before disabling the supplies.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index 56c5d51293c10..f8791e8080bdd 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -39,6 +39,7 @@ struct pwrseq_pcie_m2_ctx {
>  	struct notifier_block nb;
>  	struct gpio_desc *w_disable1_gpio;
>  	struct gpio_desc *w_disable2_gpio;
> +	struct gpio_desc *sdio_reset_gpio;
>  	struct device *dev;
>  	struct list_head pci_devices;
>  	struct mutex list_lock;
> @@ -47,13 +48,23 @@ struct pwrseq_pcie_m2_ctx {
>  static int pwrseq_pcie_m2_vregs_enable(struct pwrseq_device *pwrseq)
>  {
>  	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> +	if (ret)
> +		return ret;
>  
> -	return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> +	return gpiod_set_value_cansleep(ctx->sdio_reset_gpio, 0);
>  }

[Severity: Medium]
Does pwrseq_pcie_m2_vregs_enable() leak the regulator enable reference counts
in ctx->regs if gpiod_set_value_cansleep() fails (for example, when
sdio-reset-gpios is backed by an I2C GPIO expander)?

In drivers/power/sequencing/core.c, pwrseq_unit_enable() only disables
unit->deps when unit->enable() returns an error and does not increment
unit->enable_count or invoke unit->disable():

drivers/power/sequencing/core.c:pwrseq_unit_enable() {
	...
	if (unit->enable) {
		ret = unit->enable(pwrseq);
		if (ret) {
			dev_err(&pwrseq->dev,
				"Failed to enable target '%s': %d\n",
				unit->name, ret);
			pwrseq_unit_disable_deps(pwrseq, &unit->deps);
			return ret;
		}
	}

	unit->enable_count++;
	...
}

Should pwrseq_pcie_m2_vregs_enable() call regulator_bulk_disable(ctx->num_vregs,
ctx->regs) before returning when gpiod_set_value_cansleep() fails?

>  
>  static int pwrseq_pcie_m2_vregs_disable(struct pwrseq_device *pwrseq)
>  {
>  	struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +	int ret;
> +
> +	ret = gpiod_set_value_cansleep(ctx->sdio_reset_gpio, 1);
> +	if (ret)
> +		return ret;
>  
>  	return regulator_bulk_disable(ctx->num_vregs, ctx->regs);
>  }

[Severity: Medium]
Similarly, in pwrseq_pcie_m2_vregs_disable(), what happens if
regulator_bulk_disable(ctx->num_vregs, ctx->regs) fails after
ctx->sdio_reset_gpio has been asserted?

When unit->disable() fails in drivers/power/sequencing/core.c,
pwrseq_unit_disable() aborts without decrementing unit->enable_count and rolls
back the dependent unit by calling its unit->enable() callback, leaving the
regulators and power sequencer state enabled while ctx->sdio_reset_gpio remains
asserted.

Should ctx->sdio_reset_gpio be deasserted again if regulator_bulk_disable()
returns an error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-pwrseq-pcie-m2-sdio-reset-v1-1-02ecdafce518@imd-tec.com?part=1