Re: [PATCH v3 03/13] power: sequencing: Add pwrseq_power_is_on()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] pwrseq_power_is_on() returns -EINVAL for a NULL descriptor, which evaluates to true in a boolean context and breaks the optional resource pattern.
- [Low] The patch forgets to add the dummy implementation of pwrseq_power_is_on() for when CONFIG_POWER_SEQUENCING is disabled.
- [Low] Lockless read of desc->powered_on introduces a data race.
--

commit c1fd36a14061ff4c3c20d9f5699bb342ce2b052d
Author: Chen-Yu Tsai <wenst@chromium.org>

power: sequencing: Add pwrseq_power_is_on()

The power sequencing consumer API already does power on state tracking
internally. Expose the state to consumers through pwrseq_power_is_on()
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 02f42da915985..72b96d36920e5 100644
> --- 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);
>  
> +/**
> + * 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 and 0 for off; negative error number on failure.
> + */
> +int pwrseq_power_is_on(struct pwrseq_desc *desc)
> +{
> +	if (!desc)
> +		return -EINVAL;

[Severity: Medium]
Since pwrseq_power_on() gracefully handles an optional sequencer by
returning 0 when desc is NULL, should pwrseq_power_is_on() also handle a NULL
descriptor without returning an error?

Because -EINVAL is non-zero, returning it for a NULL descriptor will
evaluate to true in a boolean context. Consumers querying the state of an
optional sequencer using if (pwrseq_power_is_on(desc)) could falsely
evaluate this as powered on.

> +
> +	return desc->powered_on;

[Severity: Low]
Is it safe to read desc->powered_on locklessly here?

Because desc->powered_on is mutated dynamically by pwrseq_power_on() and
pwrseq_power_off() under pwrseq->state_lock, concurrent calls to
pwrseq_power_is_on() could result in a data race. Could this read use
READ_ONCE() (along with WRITE_ONCE() during updates) or be protected
by the state_lock?

> +}
> +EXPORT_SYMBOL_GPL(pwrseq_power_is_on);
> +
>  /**
>   * pwrseq_to_device() - Get the pwrseq device pointer from a descriptor.
>   * @desc: Descriptor referencing the power sequencer.

[ ... ]

> diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h
> index 3c907c9e1885d..5a5eaf85d5db9 100644
> --- a/include/linux/pwrseq/consumer.h
> +++ b/include/linux/pwrseq/consumer.h
> @@ -22,6 +22,7 @@ devm_pwrseq_get(struct device *dev, const char *target);
>  
>  int pwrseq_power_on(struct pwrseq_desc *desc);
>  int pwrseq_power_off(struct pwrseq_desc *desc);
> +int pwrseq_power_is_on(struct pwrseq_desc *desc);
>  
>  struct device *pwrseq_to_device(struct pwrseq_desc *desc);

[ ... ]

>  #else /* CONFIG_POWER_SEQUENCING */

[ ... ]

>  static inline int pwrseq_power_on(struct pwrseq_desc *desc)
>  {
>  	return -ENOSYS;
>  }
>  
>  static inline int pwrseq_power_off(struct pwrseq_desc *desc)
>  {
>  	return -ENOSYS;
>  }
>  
>  static inline struct device *pwrseq_to_device(struct pwrseq_desc *desc)
>  {
>  	return NULL;
>  }
>  
>  #endif /* CONFIG_POWER_SEQUENCING */

[Severity: Low]
Does this patch miss a dummy implementation of pwrseq_power_is_on() for when
CONFIG_POWER_SEQUENCING is disabled?

Any consumer driver using this function when the config is disabled will
cause a build failure due to an undeclared function.

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