Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230517162817.8538-1-wyes.karny@amd.com> <20230517162817.8538-4-wyes.karny@amd.com>
In-Reply-To: <20230517162817.8538-4-wyes.karny@amd.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 24 May 2023 19:44:53 +0200
Message-ID: <CAJZ5v0gte_QAt1MXXXhpGUTEAC3Eo8LCY6N+=uSFi0NRBG-Omw@mail.gmail.com>
Subject: Re: [PATCH v4 3/3] cpufreq: Return failure if fast_switch is not set
 and fast_switch_possible is set
To:     Wyes Karny <wyes.karny@amd.com>
Cc:     ray.huang@amd.com, rafael@kernel.org, viresh.kumar@linaro.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        gautham.shenoy@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Wed, May 17, 2023 at 6:30 PM Wyes Karny <wyes.karny@amd.com> wrote:
>
> If fast_switch_possible flag is set by the scaling driver, the governor
> is free to select fast_switch function even if adjust_perf is set.  Some
> scaling drivers which use adjust_perf don't set fast_switch thinking
> that the governor would never fall back to fast_switch. But the governor
> can fall back to fast_switch even in runtime if frequency invariance is
> disabled due to some reason. This could crash the kernel if the driver
> didn't set the fast_switch function pointer.
>
> Therefore, return failure in cpufreq_online function if fast_switch is
> not set and fast_switch_possible is set.
>
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> ---
>  drivers/cpufreq/cpufreq.c | 5 +++++
>  include/linux/cpufreq.h   | 4 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..7835ba4fa34c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1376,6 +1376,11 @@ static int cpufreq_online(unsigned int cpu)
>                         goto out_free_policy;
>                 }
>
> +               if (policy->fast_switch_possible && !cpufreq_driver->fast_switch) {
> +                       pr_err("fast_switch_possible is enabled but fast_switch callback is not set\n");
> +                       ret = -EINVAL;
> +                       goto out_destroy_policy;
> +               }

The driver registration can fail if the driver has ->adjust_perf
without ->fast_switch.  Then the check above would not be necessary
any more.

>                 /*
>                  * The initialization has succeeded and the policy is online.
>                  * If there is a problem with its frequency table, take it
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 26e2eb399484..8cdf77bb3bc1 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -340,7 +340,9 @@ struct cpufreq_driver {
>         /*
>          * ->fast_switch() replacement for drivers that use an internal
>          * representation of performance levels and can pass hints other than
> -        * the target performance level to the hardware.
> +        * the target performance level to the hardware. If driver is setting this,
> +        * then it needs to set fast_switch also. Because in certain scenario scale
> +        * invariance could be disabled and governor can switch back to fast_switch.

I would say something like "This can only be set if ->fast_switch is
set too, because in those cases (under specific conditions) scale
invariance can be disabled, which causes the schedutil governor to
fall back to the latter."

>          */
>         void            (*adjust_perf)(unsigned int cpu,
>                                        unsigned long min_perf,
> --
> 2.34.1
>