Re: [PATCH v3 03/13] power: sequencing: Add pwrseq_power_is_on()
From: sashiko-bot@kernel.org
Date: 2026-07-03 11:16:12
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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.
[ ... ]
quoted hunk ↗ jump to hunk
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