Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231201190757.144741-1-daniel.lezcano@linaro.org>
In-Reply-To: <20231201190757.144741-1-daniel.lezcano@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 8 Dec 2023 21:42:25 +0100
Message-ID: <CAJZ5v0gNR3AC=fMeipiHNSoOiA0u==_irbQn1r-OJ6aUJ-XBrQ@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] pm_qos: Rename freq to interval constraint
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     rjw@rjwysocki.net, caleb.connolly@linaro.org, lina.iyer@linaro.org,
        linux-pm@vger.kernel.org, mani@kernel.org,
        linux-kernel@vger.kernel.org, lukasz.luba@arm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Dec 1, 2023 at 8:08 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The frequency pm_qos relies on a couple of values, the min and max
> frequencies. However more pm_qos will be added with the same logic of
> a couple of min and max. Instead of writing new set of constraints as
> well as type, etc... let's rename freq_* to a more generic name
> interval_*

I'm fine with a rename if it helps, but I would rather call it range_*
than interval_*.

>
> That way, new qos interval based can be added easily.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/power/qos.c          |  4 +--
>  drivers/powercap/dtpm_cpu.c       |  2 +-
>  drivers/thermal/cpufreq_cooling.c |  2 +-
>  include/linux/cpufreq.h           |  6 ++--
>  include/linux/pm_qos.h            | 55 ++++++++++++++++---------------
>  kernel/power/qos.c                | 53 +++++++++++++++--------------
>  6 files changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 8e93167f1783..561d2a0e106c 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -285,14 +285,14 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
>                 memset(req, 0, sizeof(*req));
>         }
>
> -       c = &qos->freq.min_freq;
> +       c = &qos->freq.min;
>         plist_for_each_entry_safe(req, tmp, &c->list, data.freq.pnode) {
>                 apply_constraint(req, PM_QOS_REMOVE_REQ,
>                                  PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE);
>                 memset(req, 0, sizeof(*req));
>         }
>
> -       c = &qos->freq.max_freq;
> +       c = &qos->freq.max;
>         plist_for_each_entry_safe(req, tmp, &c->list, data.freq.pnode) {
>                 apply_constraint(req, PM_QOS_REMOVE_REQ,
>                                  PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 2ff7717530bf..6587c94d0127 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -28,7 +28,7 @@
>
>  struct dtpm_cpu {
>         struct dtpm dtpm;
> -       struct freq_qos_request qos_req;
> +       struct interval_qos_request qos_req;
>         int cpu;
>  };
>
> diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> index e2cc7bd30862..72e9f0cde55c 100644
> --- a/drivers/thermal/cpufreq_cooling.c
> +++ b/drivers/thermal/cpufreq_cooling.c
> @@ -77,7 +77,7 @@ struct cpufreq_cooling_device {
>  #ifndef CONFIG_SMP
>         struct time_in_idle *idle_time;
>  #endif
> -       struct freq_qos_request qos_req;
> +       struct interval_qos_request qos_req;
>  };
>
>  #ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 71d186d6933a..7e2d66c37535 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -80,9 +80,9 @@ struct cpufreq_policy {
>         struct work_struct      update; /* if update_policy() needs to be
>                                          * called, but you're in IRQ context */
>
> -       struct freq_constraints constraints;
> -       struct freq_qos_request *min_freq_req;
> -       struct freq_qos_request *max_freq_req;
> +       struct interval_constraints     constraints;
> +       struct interval_qos_request     *min_freq_req;
> +       struct interval_qos_request     *max_freq_req;
>
>         struct cpufreq_frequency_table  *freq_table;
>         enum cpufreq_table_sorting freq_table_sorted;
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 4a69d4af3ff8..a662ac918e3e 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -77,25 +77,26 @@ struct pm_qos_flags {
>  #define FREQ_QOS_MIN_DEFAULT_VALUE     0
>  #define FREQ_QOS_MAX_DEFAULT_VALUE     S32_MAX
>
> -enum freq_qos_req_type {
> -       FREQ_QOS_MIN = 1,
> +enum interval_qos_req_type {
> +       INTERVAL_QOS_MIN = 1,
> +       INTERVAL_QOS_MAX,
> +       FREQ_QOS_MIN,
>         FREQ_QOS_MAX,
>  };
>
> -struct freq_constraints {
> -       struct pm_qos_constraints min_freq;
> -       struct blocking_notifier_head min_freq_notifiers;
> -       struct pm_qos_constraints max_freq;
> -       struct blocking_notifier_head max_freq_notifiers;
> +struct interval_constraints {
> +       struct pm_qos_constraints min;
> +       struct blocking_notifier_head min_notifiers;
> +       struct pm_qos_constraints max;
> +       struct blocking_notifier_head max_notifiers;
>  };

Also I would rename min_freq and max_freq to lower_bound and
upper_bound, respectively, because this is less likely to get confused
with other things with the same name.

So the above would be

+struct range_constraints {
+       struct pm_qos_constraints lower_bound;
+       struct blocking_notifier_head lower_bound_notifiers;
+       struct pm_qos_constraints upper_bound;
+       struct blocking_notifier_head upper_bound_notifiers;
};