Re: [PATCH v3] power: sequencing: rename pwrseq_power_on/off() to pwrseq_enable/disable()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing error check after `pwrseq_unit_enable()` in `pwrseq_enable()`.
- [High] The return value of `pwrseq_enable()` is ignored in `slot_pwrctrl_power_on()`.
- [High] Missing cleanup of previously enabled regulators in `slot_pwrctrl_power_on()` if clock enable fails.
- [High] `pwrseq_put()` frees the descriptor without decrementing `enable_count` if `pwrseq_disable()` fails.
--

--- Patch [1]: [PATCH v3] power: sequencing: rename pwrseq_power_on/off() to pwrseq_enable/disable() ---
commit 72b956d7bab750ba1857c47c880220b1aa1f9e10
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

power: sequencing: rename pwrseq_power_on/off() to pwrseq_enable/disable()

This commit renames the power sequencing functions pwrseq_power_on() and
pwrseq_power_off() to pwrseq_enable() and pwrseq_disable() to better reflect
their semantics. It updates consumers across various subsystems to use the new
names.

Link: https://lore.kernel.org/all/CAMRc=Mefxn81d0VUwmQgwFtffkL1=Er_1VQZCbC1Sa-Qph4t1w@mail.gmail.com/
Link: https://patch.msgid.link/20260731-pwrseq-vote-rename-v2-1-480357946e00@oss.qualcomm.com

> diff --git a/drivers/pci/pwrctrl/generic.c b/drivers/pci/pwrctrl/generic.c
> index a7e599d841e62..d56171e0ee244 100644
> --- a/drivers/pci/pwrctrl/generic.c
> +++ b/drivers/pci/pwrctrl/generic.c
> @@ -29,7 +29,7 @@ static int slot_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
>  	int ret;
>  
>  	if (slot->pwrseq) {
> -		pwrseq_power_on(slot->pwrseq);
> +		pwrseq_enable(slot->pwrseq);
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but the negative error code from pwrseq_enable()
is ignored here, and 0 is incorrectly returned on failure.

If pwrseq_enable() fails, the PCIe subsystem will proceed assuming the slot is
powered on. When it tries to probe or interact with the slot, it will access
unpowered hardware, potentially leading to bus faults or system hangs.

Should this code return the error code from pwrseq_enable() instead?

[Severity: High]
This is another pre-existing issue, but if slot->pwrseq is not present, this
function continues with regulator_bulk_enable() and clk_prepare_enable():

	ret = regulator_bulk_enable(slot->num_supplies, slot->supplies);
	if (ret < 0) {
		...
		return ret;
	}

	return clk_prepare_enable(slot->clk);

If clk_prepare_enable() fails, the function returns an error without disabling
the regulators that were just enabled, resulting in a resource leak.

Should clk_prepare_enable() failure be checked, and regulator_bulk_disable()
called if it fails?

[ ... ]

> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> index 02f42da915985..721e888b658d1 100644
> --- a/drivers/power/sequencing/core.c
> +++ b/drivers/power/sequencing/core.c
> @@ -708,7 +708,7 @@ void pwrseq_put(struct pwrseq_desc *desc)
>  	pwrseq = desc->pwrseq;
>  
>  	if (desc->powered_on)
> -		pwrseq_power_off(desc);
> +		pwrseq_disable(desc);
>  
>  	kfree(desc);
>  	module_put(pwrseq->owner);

[Severity: High]
This isn't a bug introduced by this patch, but pwrseq_put() frees the descriptor
without decrementing the enable_count if pwrseq_disable() fails.

If the underlying hardware disable fails, pwrseq_unit_disable() returns an error
and does not decrement the enable_count. However, pwrseq_put() ignores this
error and unconditionally calls kfree(desc), stranding the enable_count inside
the pwrseq_unit and causing a permanent logical leak.

Is it intentional that the descriptor is freed even if disabling fails?

[ ... ]

> @@ -874,7 +874,7 @@ static int pwrseq_unit_disable(struct pwrseq_device *pwrseq,
>  }
>  
>  /**
> - * pwrseq_power_on() - Issue a power-on request on behalf of the consumer
> + * pwrseq_enable() - Issue a power-on request on behalf of the consumer
>   *                     device.
>   * @desc: Descriptor referencing the power sequencer.
>   *
> @@ -887,7 +887,7 @@ static int pwrseq_unit_disable(struct pwrseq_device *pwrseq,
>   * Returns:
>   * 0 on success, negative error number on failure.
>   */
> -int pwrseq_power_on(struct pwrseq_desc *desc)
> +int pwrseq_enable(struct pwrseq_desc *desc)
>  {
>  	struct pwrseq_device *pwrseq;
>  	struct pwrseq_target *target;

[Severity: High]
This is a pre-existing issue, but there is a missing error check after
pwrseq_unit_enable() in pwrseq_enable().

Later in this function, we do:

	scoped_guard(mutex, &pwrseq->state_lock) {
		ret = pwrseq_unit_enable(pwrseq, unit);
		if (!ret)
			desc->powered_on = true;
	}

	if (target->post_enable) {
		ret = target->post_enable(pwrseq);
		...

If pwrseq_unit_enable() fails, the code ignores the error and proceeds to call
target->post_enable(). If post_enable() succeeds, it overwrites the error code
and returns 0 (success) to the caller, while desc->powered_on remains false
and the hardware is unpowered.

If post_enable() fails, it attempts to call pwrseq_unit_disable() on an
unpowered unit, causing an unbalanced enable count and a kernel WARN.

Should we return early if pwrseq_unit_enable() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-pwrseq-vote-rename-v3-1-44e60b8be053@oss.qualcomm.com?part=1