Re: [PATCH RFC] power: sequencing: rename pwrseq_power_on/off() to pwrseq_vote_on/off()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unhandled error from `pwrseq_vote_on()` in `slot_pwrctrl_power_on()` masks power-on failures.
- [High] Logic error in `pwrseq_vote_on()` executes `post_enable` and masks errors when `pwrseq_unit_enable()` fails.
--

--- Patch [1]: [PATCH RFC] power: sequencing: rename pwrseq_power_on/off() to pwrseq_vote_on/off() ---
commit e51e94e0f35e884f6a8dab94fc8221cd0e96b231
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

power: sequencing: rename pwrseq_power_on/off() to pwrseq_vote_on/off()

This commit renames the power sequencing API functions pwrseq_power_on and
pwrseq_power_off to pwrseq_vote_on and pwrseq_vote_off respectively.

Link: https://lore.kernel.org/all/CAMRc=Mefxn81d0VUwmQgwFtffkL1=Er_1VQZCbC1Sa-Qph4t1w@mail.gmail.com/

> diff --git a/drivers/pci/pwrctrl/generic.c b/drivers/pci/pwrctrl/generic.c
> index a7e599d841e62..8872f6501352d 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_vote_on(slot->pwrseq);

[Severity: High]
This isn't a bug introduced by this patch, but does this code ignore the
return value of pwrseq_vote_on()?

If the power sequence fails, the error is discarded and
slot_pwrctrl_power_on incorrectly returns 0. Could this lead to the PCI
subsystem attempting to probe unpowered hardware, causing fatal kernel
crashes when reading unpowered MMIO registers?

>  		return 0;
>  	}
>  

[ ... ]

> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> index 02f42da915985..c630b14f6f2c4 100644
> --- a/drivers/power/sequencing/core.c
> +++ b/drivers/power/sequencing/core.c

[ ... ]

> @@ -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_vote_on(struct pwrseq_desc *desc)
>  {
>  	struct pwrseq_device *pwrseq;
>  	struct pwrseq_target *target;

[Severity: High]
This is a pre-existing issue, but looking at the body of pwrseq_vote_on(),
is there a missing check for the return value of pwrseq_unit_enable()
before calling target->post_enable()?

	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, we bypass any failure check and proceed to
call target->post_enable(). If post_enable() succeeds, it overwrites the
original error with 0, meaning we return success even though the hardware
is unpowered.

If post_enable() fails, its error rollback unconditionally executes
pwrseq_unit_disable(), which triggers a WARN for unmatched power-off and
a potential negative enable count.

Should there be a check on ret before executing the post_enable logic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-pwrseq-vote-rename-v1-1-a2029aeeac65@oss.qualcomm.com?part=1