Re: [PATCH] cpufreq: Allocate QoS freq_req objects with policy

[Rafael J. Wysocki]

On Tue, Mar 31, 2026 at 7:04 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> A recent change exposed a bug in the error path: if
> freq_qos_add_request(boost_freq_req) fails, min_freq_req may remain a
> valid pointer even though it was never successfully added. During policy
> teardown, this leads to an unconditional call to
> freq_qos_remove_request(), triggering a WARN.
>
> The current design allocates all three freq_req objects together, making
> the lifetime rules unclear and error handling fragile.
>
> Simplify this by allocating the QoS freq_req objects at policy
> allocation time. The policy itself is dynamically allocated, and two of
> the three requests are always needed anyway. This ensures consistent
> lifetime management and eliminates the inconsistent state in failure
> paths.
>
> Reported-by: Zhongqiu Han <zhongqiu.han@xxxxxxxxxxxxxxxx>
> Fixes: 6e39ba4e5a82 ("cpufreq: Add boost_freq_req QoS request")
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/cpufreq/cpufreq.c | 53 +++++++++++----------------------------
>  include/linux/cpufreq.h   |  6 ++---
>  2 files changed, 17 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c0aa970c7a67..f4a949f1e48f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -614,7 +614,7 @@ static int policy_set_boost(struct cpufreq_policy *policy, bool enable)
>                 return ret;
>         }
>
> -       ret = freq_qos_update_request(policy->boost_freq_req, policy->cpuinfo.max_freq);
> +       ret = freq_qos_update_request(&policy->boost_freq_req, policy->cpuinfo.max_freq);
>         if (ret < 0) {
>                 policy->boost_enabled = !policy->boost_enabled;
>                 cpufreq_driver->set_boost(policy, policy->boost_enabled);
> @@ -769,7 +769,7 @@ static ssize_t store_##file_name                                    \
>         if (ret)                                                        \
>                 return ret;                                             \
>                                                                         \
> -       ret = freq_qos_update_request(policy->object##_freq_req, val);\
> +       ret = freq_qos_update_request(&policy->object##_freq_req, val); \
>         return ret >= 0 ? count : ret;                                  \
>  }
>
> @@ -1374,7 +1374,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>         /* Cancel any pending policy->update work before freeing the policy. */
>         cancel_work_sync(&policy->update);
>
> -       if (policy->max_freq_req) {
> +       if (freq_qos_request_active(&policy->max_freq_req)) {
>                 /*
>                  * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
>                  * notification, since CPUFREQ_CREATE_POLICY notification was
> @@ -1382,12 +1382,13 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>                  */
>                 blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>                                              CPUFREQ_REMOVE_POLICY, policy);
> -               freq_qos_remove_request(policy->max_freq_req);
> +               freq_qos_remove_request(&policy->max_freq_req);
>         }
>
> -       freq_qos_remove_request(policy->min_freq_req);
> -       freq_qos_remove_request(policy->boost_freq_req);
> -       kfree(policy->min_freq_req);
> +       if (freq_qos_request_active(&policy->min_freq_req))
> +               freq_qos_remove_request(&policy->min_freq_req);
> +       if (freq_qos_request_active(&policy->boost_freq_req))
> +               freq_qos_remove_request(&policy->boost_freq_req);
>
>         cpufreq_policy_put_kobj(policy);
>         free_cpumask_var(policy->real_cpus);
> @@ -1452,57 +1453,31 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>         cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>
>         if (new_policy) {
> -               unsigned int count;
> -
>                 for_each_cpu(j, policy->related_cpus) {
>                         per_cpu(cpufreq_cpu_data, j) = policy;
>                         add_cpu_dev_symlink(policy, j, get_cpu_device(j));
>                 }
>
> -               count = policy->boost_supported ? 3 : 2;
> -               policy->min_freq_req = kzalloc(count * sizeof(*policy->min_freq_req),
> -                                              GFP_KERNEL);
> -               if (!policy->min_freq_req) {
> -                       ret = -ENOMEM;
> -                       goto out_destroy_policy;
> -               }
> -
>                 if (policy->boost_supported) {
> -                       policy->boost_freq_req = policy->min_freq_req + 2;
> -
>                         ret = freq_qos_add_request(&policy->constraints,
> -                                                  policy->boost_freq_req,
> +                                                  &policy->boost_freq_req,
>                                                    FREQ_QOS_MAX,
>                                                    policy->cpuinfo.max_freq);
> -                       if (ret < 0) {
> -                               policy->boost_freq_req = NULL;
> +                       if (ret < 0)
>                                 goto out_destroy_policy;
> -                       }
>                 }
>
>                 ret = freq_qos_add_request(&policy->constraints,
> -                                          policy->min_freq_req, FREQ_QOS_MIN,
> +                                          &policy->min_freq_req, FREQ_QOS_MIN,
>                                            FREQ_QOS_MIN_DEFAULT_VALUE);
> -               if (ret < 0) {
> -                       kfree(policy->min_freq_req);
> -                       policy->min_freq_req = NULL;
> +               if (ret < 0)
>                         goto out_destroy_policy;
> -               }
> -
> -               /*
> -                * This must be initialized right here to avoid calling
> -                * freq_qos_remove_request() on uninitialized request in case
> -                * of errors.
> -                */
> -               policy->max_freq_req = policy->min_freq_req + 1;
>
>                 ret = freq_qos_add_request(&policy->constraints,
> -                                          policy->max_freq_req, FREQ_QOS_MAX,
> +                                          &policy->max_freq_req, FREQ_QOS_MAX,
>                                            FREQ_QOS_MAX_DEFAULT_VALUE);
> -               if (ret < 0) {
> -                       policy->max_freq_req = NULL;
> +               if (ret < 0)
>                         goto out_destroy_policy;
> -               }
>
>                 blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>                                 CPUFREQ_CREATE_POLICY, policy);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index b6f6c7d06912..9b10eb486ece 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -79,9 +79,9 @@ struct cpufreq_policy {
>                                          * 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 freq_qos_request *boost_freq_req;
> +       struct freq_qos_request min_freq_req;
> +       struct freq_qos_request max_freq_req;
> +       struct freq_qos_request boost_freq_req;
>
>         struct cpufreq_frequency_table  *freq_table;
>         enum cpufreq_table_sorting freq_table_sorted;
> --

Applied as 7.1 material, thanks!