Re: [PATCH v8 03/16] power: sequencing: Add pwrseq_get_state()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Data race when reading desc->powered_on locklessly in pwrseq_get_state()

Pre-existing issues:
- [High] TOCTOU race in pwrseq_enable() and pwrseq_disable() causes permanent resource leak or WARN()
--

commit 5b89b7211c4aa2df354664ebde1b55a332d08712
Author: Chen-Yu Tsai <wenst@chromium.org>
Date:   Fri Sep 4 08:41:42 2026 +0000

    power: sequencing: Add pwrseq_get_state()
    
    The power sequencing consumer API already does power on state tracking
    internally. Expose the state to consumers through pwrseq_get_state()
    so that they don't have to reimplement it locally.

> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> index 721e888b658d1..dbb2bd2f8864e 100644
> --- a/drivers/power/sequencing/core.c
> +++ b/drivers/power/sequencing/core.c
> @@ -968,6 +968,25 @@ int pwrseq_disable(struct pwrseq_desc *desc)

[Severity: High]
This is a pre-existing issue, but could the lockless checks of desc->powered_on
lead to a resource leak or a WARN() due to a TOCTOU race?

If a consumer calls pwrseq_enable() from two threads concurrently, could they
both bypass the lockless check:

drivers/power/sequencing/core.c:pwrseq_enable() {
    ...
    if (!desc || desc->powered_on)
        return 0;
    ...
}

and then both acquire state_lock and double-increment the provider unit's
enable_count?

A subsequent single pwrseq_disable() would then clear desc->powered_on:

drivers/power/sequencing/core.c:pwrseq_disable() {
    ...
    if (!desc || !desc->powered_on)
        return 0;
    ...
}

causing future disables to return early locklessly. Would this leave the
unit's enable_count permanently elevated?

Additionally, if pwrseq_disable() races with the lockless post_enable() error
rollback path, could it trigger the WARN(1) in pwrseq_unit_disable() due to
underflowing enable_count?

>  }
>  EXPORT_SYMBOL_GPL(pwrseq_disable);
>  
> +/**
> + * pwrseq_get_state() - Queries the last requested state of the power sequencer.
[ ... ]
> +int pwrseq_get_state(struct pwrseq_desc *desc)
> +{
> +	if (!desc)
> +		return -EINVAL;
> +
> +	return desc->powered_on ? PWRSEQ_STATE_ON : PWRSEQ_STATE_OFF;

[Severity: Low]
Does this lockless read of a state variable that is concurrently modified by
writers without READ_ONCE() cause a data race?

Since pwrseq_get_state() can be called concurrently with pwrseq_enable() or
pwrseq_disable(), kernel concurrency rules generally require READ_ONCE() for
lockless reads of shared variables to prevent compiler optimization issues.

> +}
> +EXPORT_SYMBOL_GPL(pwrseq_get_state);

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