Re: [PATCH v5 4/4] pmdomain: add support system-wide resume latency constraints

[Ulf Hansson]

On Thu, Aug 27, 2026 at 12:23 AM Kevin Hilman (TI) <khilman@xxxxxxxxxxxx> wrote:
>
> In addition to checking for CPU latency constraints when checking if
> OK to power down a domain, also check for QoS latency constraints in
> all devices of a domain and use that in determining the final latency
> constraint to use for the domain.
>
> Since cpu_system_power_down_ok() is used for system-wide suspend, the
> per-device constratints are only relevant if the LATENCY_SYS QoS flag
> is set.

cpu_system_power_down_ok() is especially used for genpd's that have
the GENPD_FLAG_CPU_DOMAIN bit set (cpuidle-psci-domain and
cpuidle-riscv-sbi).

In other words, this has no effect on other types of PM domains that
are managed by genpd. Are you planning on adding that on top or this
is sufficient for your use cases?

>
> Reviewed-by: Abel Vesa <abel.vesa@xxxxxxxxxxxxxxxx>
> Reviewed-by: Kendall Willis <k-willis@xxxxxx>
> Signed-off-by: Kevin Hilman (TI) <khilman@xxxxxxxxxxxx>
> ---
>  drivers/pmdomain/governor.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/drivers/pmdomain/governor.c b/drivers/pmdomain/governor.c
> index 96737abbb496..1a85fd375db9 100644
> --- a/drivers/pmdomain/governor.c
> +++ b/drivers/pmdomain/governor.c
> @@ -13,6 +13,8 @@
>  #include <linux/cpumask.h>
>  #include <linux/ktime.h>
>
> +#include "core.h"
> +
>  static int dev_update_qos_constraint(struct device *dev, void *data)
>  {
>         s64 *constraint_ns_p = data;
> @@ -425,17 +427,71 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
>         return true;
>  }
>
> +/**
> + * check_device_qos_latency - Callback to check device QoS latency constraints
> + * @dev: Device to check
> + * @data: Pointer to s32 variable holding minimum latency found so far
> + *
> + * This callback checks if the device has a system-wide resume latency QoS
> + * constraint and updates the minimum latency if this device has a stricter
> + * constraint.
> + *
> + * This runs in atomic context: for a CPU domain the genpd lock is a raw
> + * spinlock and the s2idle path runs in the syscore suspend window with
> + * interrupts disabled.  The lockless dev_pm_qos_raw_*() accessors must
> + * therefore be used here; the locked dev_pm_qos_read_value() /
> + * dev_pm_qos_flags() would take dev->power.lock, which is a sleeping lock
> + * on PREEMPT_RT and must not be acquired in this context.  The values read
> + * are best-effort, which matches the sibling cpu_power_down_ok() governor.

This is a bit too much in my opinion, please leave out the parts
concerning the syscore/atomic/lockless parts.

If we want that information to be described (I guess it would make
sense), I suggest we add that along with cpu_system_power_down_ok()
instead as it better belongs there.

> + *
> + * The system-wide flag is checked first so that devices that have not opted
> + * in only incur a single lockless read.
> + *
> + * Returns: 0 to continue iteration.
> + */
> +static int check_device_qos_latency(struct device *dev, void *data)
> +{
> +       s32 *min_dev_latency = data;
> +       s32 dev_latency;
> +
> +       if (!(dev_pm_qos_raw_flags(dev) & PM_QOS_FLAG_LATENCY_SYS))
> +               return 0;
> +
> +       dev_latency = dev_pm_qos_raw_resume_latency(dev);
> +       if (dev_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) {
> +               dev_dbg(dev,
> +                       "has QoS system-wide resume latency=%d\n",
> +                       dev_latency);

Do we really need a dev_dbg() here? Leftover from debugging?

> +               if (dev_latency < *min_dev_latency)
> +                       *min_dev_latency = dev_latency;
> +       }
> +
> +       return 0;
> +}
> +
>  static bool cpu_system_power_down_ok(struct dev_pm_domain *pd)
>  {
>         s64 constraint_ns = cpu_wakeup_latency_qos_limit() * NSEC_PER_USEC;
>         struct generic_pm_domain *genpd = pd_to_genpd(pd);
>         int state_idx = genpd->state_count - 1;
> +       s32 min_dev_latency = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +       s64 min_dev_latency_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS;

We don't need to assign a default value for min_dev_latency_ns.

>
>         if (!(genpd->flags & GENPD_FLAG_CPU_DOMAIN)) {
>                 genpd->state_idx = state_idx;
>                 return true;
>         }
>
> +       genpd_for_each_child(genpd, check_device_qos_latency,
> +                            &min_dev_latency);
> +
> +       /* If device latency < CPU wakeup latency, use it instead */
> +       if (min_dev_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) {
> +               min_dev_latency_ns = min_dev_latency * NSEC_PER_USEC;
> +               if (min_dev_latency_ns < constraint_ns)
> +                       constraint_ns = min_dev_latency_ns;
> +       }
> +
>         /* Find the deepest state for the latency constraint. */
>         while (state_idx >= 0) {
>                 s64 latency_ns = genpd->states[state_idx].power_off_latency_ns +
>
> --
> 2.47.3
>

Kind regards
Uffe