Re: [PATCH v4 03/14] power: sequencing: Add pwrseq_power_is_on()

[sashiko-bot@kernel.org]

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

New issues:
- [High] pwrseq_power_is_on() returns negative error codes for optional descriptors and disabled builds, which evaluate to true in boolean contexts and break the optional descriptor pattern.

Pre-existing issues:
- [High] A TOCTOU race condition in the lockless check of desc->powered_on causes a reference count leak for unit->enable_count.
--

--- Patch [3]: [PATCH v4 03/14] power: sequencing: Add pwrseq_power_is_on() ---
commit f5ad172d0dbaf149ecc08081af49a9c15c411aa1
Author: Chen-Yu Tsai <wenst@chromium.org>
power: sequencing: Add pwrseq_power_is_on()

This patch introduces pwrseq_power_is_on() to expose the internal power on
state of the power sequencer to consumers. It also adds a fallback stub
for unconfigured builds.

> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> --- a/drivers/power/sequencing/core.c
> +++ b/drivers/power/sequencing/core.c
> @@ -968,6 +968,24 @@ int pwrseq_power_off(struct pwrseq_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(pwrseq_power_off);

[Severity: High]
This is a pre-existing issue, but can the lockless check of desc->powered_on
in pwrseq_power_on() and pwrseq_power_off() cause a reference count leak
for unit->enable_count?

If multiple threads concurrently call pwrseq_power_on() on the same
descriptor, both can bypass the lockless check:

pwrseq_power_on() {
        if (!desc || desc->powered_on)
                return 0;
        // threads proceed and increment unit->enable_count under lock
}

However, when pwrseq_power_off() is called, it will only decrement the count
once, because the first call sets desc->powered_on = false. Subsequent
power-off calls return early:

pwrseq_power_off() {
        if (!desc || !desc->powered_on)
                return 0; // early exit
        // ...
        desc->powered_on = false;
}

Could this leave the hardware powered on indefinitely and trigger a WARN
on device removal?

> +/**
> + * pwrseq_power_is_on() - Queries the last requested state of the power sequencer.
[ ... ]
> + */
> +int pwrseq_power_is_on(struct pwrseq_desc *desc)
> +{
> +	if (!desc)
> +		return -EINVAL;

[Severity: High]
Will this return value break the optional descriptor pattern?

Consumers using optional power sequencers might call this with a NULL
descriptor. Since -EINVAL evaluates to true in a C boolean context,
would this cause the caller to incorrectly assume the power is on?

Could we return 0 here instead, similar to how pwrseq_power_on() gracefully
returns 0 for NULL descriptors?

> +	return desc->powered_on;
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_power_is_on);
> +

[ ... ]

> diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h
> --- a/include/linux/pwrseq/consumer.h
> +++ b/include/linux/pwrseq/consumer.h

[ ... ]

> @@ -53,6 +54,11 @@ static inline int pwrseq_power_off(struct pwrseq_desc *desc)
>  	return -ENOSYS;
>  }
>  
> +static inline int pwrseq_power_is_on(struct pwrseq_desc *desc)
> +{
> +	return -ENOSYS;
> +}

[Severity: High]
Does this fallback stub cause similar issues for unconfigured builds?

Returning -ENOSYS will also evaluate to true in boolean checks.
Should this return 0 to ensure consistent behavior when POWER_SEQUENCING
is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709095726.704448-1-wenst@chromium.org?part=3