Re: [PATCH v2 2/3] pmdomain: Add support for system-suspend-only states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] pmdomain: unfiltered system_state in
  genpd_reflect_residency()
--

commit 1997c1201917b38c7d4bd3614225f7a62b53a68b
Author: Maulik Shah <maulik.shah@oss.qualcomm.com>

pmdomain: Add support for system-suspend-only states

This commit adds a system_state boolean to struct genpd_power_state,
populated from the "system-state" DT property, so the genpd governor skips
system-suspend-only states during regular CPU and runtime idle while
keeping them selectable during system-wide suspend.

> diff --git a/drivers/pmdomain/governor.c b/drivers/pmdomain/governor.c
> index 38bba24ec26e3..7684f9eecf853 100644
> --- a/drivers/pmdomain/governor.c
> +++ b/drivers/pmdomain/governor.c

[ ... ]

> @@ -407,6 +410,9 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
>  	 */
>  	i = genpd->state_idx;
>  	do {
> +		if (genpd->states[i].system_state)
> +			continue;
> +
>  		if ((idle_duration_ns >= (genpd->states[i].residency_ns +
>  		    genpd->states[i].power_off_latency_ns)) &&
>  		    (global_constraint >= (genpd->states[i].power_on_latency_ns +

[Severity: Medium]
Should genpd_reflect_residency() also skip next_state when
next_state->system_state is set?

When cpu_power_down_ok() skips a system_state idle state and selects a
shallower state, it sets genpd->gd->reflect_residency = true.  On wakeup,
genpd_power_on() calls genpd_reflect_residency(), which checks the next
deeper state unconditionally:

drivers/pmdomain/core.c:genpd_reflect_residency() {
    ...
	} else if (state_idx < (genpd->state_count -1)) {
		next_state = &genpd->states[state_idx + 1];
		target_ns = next_state->power_off_latency_ns +
			next_state->residency_ns;

		if (sleep_ns >= target_ns)
			state->below++;
	}
    ...
}

If genpd->states[state_idx + 1] has system_state set, it is not eligible
for regular CPU idle, yet any CPU idle sleep reaching its target_ns will
falsely increment state->below in the debugfs idle_states statistics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-s2idle_state-v2-0-ad604182e7e7@oss.qualcomm.com?part=2