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

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

New issues:
- [Low] The function `pwrseq_power_off()` was incorrectly renamed to `pwrseq_disble()` instead of `pwrseq_disable()`.

Pre-existing issues:
- [High] Unchecked failure in the rollback path of `pwrseq_enable()` leads to a leaked power sequence enable count and an inconsistent descriptor state.
- [High] TOCTOU race on `desc->powered_on` in `pwrseq_enable()` and `pwrseq_disble()` leads to leaked power sequencer votes or unbalanced enable counts.
--

--- Patch [1]: [PATCH v2] power: sequencing: rename pwrseq_power_on/off() to pwrseq_enable/disable() ---
commit adf6e02fc844795e2717b0bb913d426b91714a03
Author: Bartosz Golaszewski <[email protected]>
Subject: power: sequencing: rename pwrseq_power_on/off() to pwrseq_enable/disable()

This commit renames pwrseq_power_on() and pwrseq_power_off() to 
pwrseq_enable() and pwrseq_disable(). The new names better reflect the
semantics of voting for power states rather than direct power control.

Link: https://lore.kernel.org/all/CAMRc=Mefxn81d0VUwmQgwFtffkL1=Er_1VQZCbC1Sa-Qph4t1w@mail.gmail.com/
Link: https://patch.msgid.link/[email protected]

> diff --git a/Documentation/driver-api/pwrseq.rst b/Documentation/driver-api/pwrseq.rst
> index ad18b2326b689..0c315ab1138b0 100644
> --- a/Documentation/driver-api/pwrseq.rst
> +++ b/Documentation/driver-api/pwrseq.rst
> @@ -50,9 +50,9 @@ Consumer interface
>  The consumer API is aimed to be as simple as possible. The driver interested in
>  getting a descriptor from the power sequencer should call pwrseq_get() and
>  specify the name of the target it wants to reach in the sequence after calling
> -pwrseq_power_up(). The descriptor can be released by calling pwrseq_put() and
> +pwrseq_enable(). The descriptor can be released by calling pwrseq_put() and
>  the consumer can request the powering down of its target with
> -pwrseq_power_off(). Note that there is no guarantee that pwrseq_power_off()
> +pwrseq_disble(). Note that there is no guarantee that pwrseq_disble()

[Severity: Low]
Is this function name a typo? Should it be pwrseq_disable() instead of
pwrseq_disble()?

[ ... ]
> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> index 02f42da915985..34d5c0af6f37b 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_enable(struct pwrseq_desc *desc)
>  {

[Severity: High]
This is a pre-existing issue, but does this function leak the power sequencer
enable count if the rollback fails?

Looking at pwrseq_enable(), if target->post_enable() fails, it attempts to
roll back by calling pwrseq_unit_disable():

	if (target->post_enable) {
		ret = target->post_enable(pwrseq);
		if (ret) {
			scoped_guard(mutex, &pwrseq->state_lock) {
				pwrseq_unit_disable(pwrseq, unit);
				desc->powered_on = false;
			}
		}
	}

If pwrseq_unit_disable() returns an error (for example, due to a hardware
failure), the unit's enable_count won't be decremented, but desc->powered_on
is unconditionally set to false.

Later, when the consumer calls pwrseq_put() or pwrseq_disable(), it will skip
calling pwrseq_unit_disable() because desc->powered_on is false, permanently
leaving the hardware enabled. Should the code check the return value of
pwrseq_unit_disable() before clearing desc->powered_on?

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition when
checking desc->powered_on?

In pwrseq_enable(), the desc->powered_on flag is checked outside the state
lock, but modified inside it:

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

If multiple threads call pwrseq_enable() concurrently on the same descriptor,
both could see desc->powered_on as false. They would then serialize on the
state_lock and both increment the unit's enable_count. A subsequent call to
pwrseq_disable() would only decrement it once, leaking the second vote.

>  	struct pwrseq_device *pwrseq;
>  	struct pwrseq_target *target;
[ ... ]
> @@ -940,7 +940,7 @@ EXPORT_SYMBOL_GPL(pwrseq_power_on);
>   * Returns:
>   * 0 on success, negative error number on failure.
>   */
> -int pwrseq_power_off(struct pwrseq_desc *desc)
> +int pwrseq_disble(struct pwrseq_desc *desc)
>  {

[Severity: High]
This is a pre-existing issue, but does pwrseq_disble() have the same TOCTOU
race on desc->powered_on as pwrseq_enable()?

	if (!desc || !desc->powered_on)
		return 0;
    ...
	guard(mutex)(&pwrseq->state_lock);

	ret = pwrseq_unit_disable(pwrseq, unit);
	if (!ret)
		desc->powered_on = false;

Concurrent calls could result in both seeing desc->powered_on as true,
leading to unbalanced decrements of the enable_count.

>  	struct pwrseq_device *pwrseq;
>  	struct pwrseq_unit *unit;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1