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

[Kevin Hilman <khilman@baylibre.com>]

Ulf Hansson <ulf.hansson@oss.qualcomm.com> writes:

> On Thu, Aug 27, 2026 at 12:23 AM Kevin Hilman (TI) <khilman@baylibre.com> 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?

This is sufficient for my use cases.

>> Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
>> Reviewed-by: Kendall Willis <k-willis@ti.com>
>> Signed-off-by: Kevin Hilman (TI) <khilman@baylibre.com>
>> ---
>>  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.

OK, sounds good.  I'll move it.

>> + *
>> + * 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?

Not needed, debug leftover.

>> +               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.

OK.

>>
>>         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
>>

Kevin