Re: [PATCH v5 03/16] power: sequencing: Add pwrseq_power_is_on()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The !CONFIG_POWER_SEQUENCING stub for pwrseq_power_is_on() unconditionally returns -ENOSYS, breaking support for optional descriptors.
- [Medium] Unprotected lockless read of `desc->powered_on` in `pwrseq_power_is_on()` constitutes a data race.

Pre-existing issues:
- [High] The stubs for pwrseq_power_on() and pwrseq_power_off() unconditionally return -ENOSYS, breaking optional descriptors when CONFIG_POWER_SEQUENCING is disabled.
- [High] Lockless TOCTOU race on `desc->powered_on` in `pwrseq_power_on()` and `pwrseq_power_off()` allows concurrent calls to leak `enable_count` references.
--

--- Patch [3]: [PATCH v5 03/16] power: sequencing: Add pwrseq_power_is_on() ---
commit 32144de8b3a9da0a0745b35617cfc4d0359911df
Author: Chen-Yu Tsai <wenst@chromium.org>

power: sequencing: Add pwrseq_power_is_on()

This commit introduces pwrseq_power_is_on() to expose the internal power-on
state tracking to consumers, eliminating the need for local tracking. The API
handles optional descriptors by returning 1 if the descriptor is NULL.

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

[Severity: High]
This is a pre-existing issue, but could the lockless check of
desc->powered_on in pwrseq_power_on() and pwrseq_power_off() cause a race
condition that leaks enable_count references?

If two threads concurrently invoke pwrseq_power_on() with the same descriptor,
both could bypass the early check locklessly:

    if (!desc || desc->powered_on)
        return 0;

They then sequentially acquire pwrseq->state_lock, but neither re-validates
the flag inside the critical section. Both execute pwrseq_unit_enable(),
incrementing enable_count twice.

When pwrseq_power_off() is called later, the first invocation decrements the
count and clears desc->powered_on. The second invocation sees
desc->powered_on == false locklessly and returns early, leaving enable_count
permanently elevated.

> +/**
> + * pwrseq_power_is_on() - Queries the last requested state of the power sequencer.
> + * @desc: Descriptor referencing the power sequencer.
> + *
> + * This returns the last requested state of the power sequencer.
> + *
> + * Returns:
> + * On success, 1 for on or desc is NULL (optional) and 0 for off;
> + * negative error number on failure.
> + */
> +int pwrseq_power_is_on(struct pwrseq_desc *desc)
> +{
> +	if (!desc)
> +		return 1;
> +
> +	return desc->powered_on;

[Severity: Medium]
Does this lockless read of desc->powered_on need READ_ONCE() to prevent data
races?

Since desc->powered_on can be concurrently modified by pwrseq_power_on()
and pwrseq_power_off(), reading it without protection might lead to unsafe
compiler optimizations.

> +}
> +EXPORT_SYMBOL_GPL(pwrseq_power_is_on);

[ ... ]

> diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h
> index 3c907c9e1885d..3c6122bd02057 100644
> --- 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;
>  }

[Severity: High]
This is a pre-existing issue, but do the stubs for pwrseq_power_on() and
pwrseq_power_off() also break optional descriptors?

If a driver invokes pwrseq_power_on(NULL) on a kernel without power
sequencing enabled, returning -ENOSYS instead of 0 defeats the purpose of
an optional descriptor, as consumers checking for errors will abort their
operations.

> +static inline int pwrseq_power_is_on(struct pwrseq_desc *desc)
> +{
> +	return -ENOSYS;
> +}

[Severity: Medium]
When CONFIG_POWER_SEQUENCING is disabled, does this stub break the contract
for optional descriptors?

The commit message mentions returning 1 if the descriptor is NULL (optional),
but this stub unconditionally returns -ENOSYS. This will cause consumers
checking the power state of an optional descriptor to receive a negative
error code instead of the expected 1.

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