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: <ZHSx61V6fgMcXsvh@BLR-5CG13462PL.amd.com> <20230529142551.6726-1-wyes.karny@amd.com>
In-Reply-To: <20230529142551.6726-1-wyes.karny@amd.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 15 Jun 2023 18:16:13 +0200
Message-ID: <CAJZ5v0hviMc_vhnOx_mPLikAX-5X4YjZabP+F5GNH5ac8WNGcw@mail.gmail.com>
Subject: Re: [PATCH v4.1] cpufreq: Fail driver register if it has adjust_perf
 without fast_switch
To:     Wyes Karny <wyes.karny@amd.com>
Cc:     rafael@kernel.org, viresh.kumar@linaro.org, ray.huang@amd.com,
        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 Mon, May 29, 2023 at 4:26 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, fail driver registration if it has adjust_perf without
> fast_switch.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> ---
>  drivers/cpufreq/cpufreq.c | 3 ++-
>  include/linux/cpufreq.h   | 5 ++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..50bbc969ffe5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2828,7 +2828,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>              (driver_data->setpolicy && (driver_data->target_index ||
>                     driver_data->target)) ||
>              (!driver_data->get_intermediate != !driver_data->target_intermediate) ||
> -            (!driver_data->online != !driver_data->offline))
> +            (!driver_data->online != !driver_data->offline) ||
> +                (driver_data->adjust_perf && !driver_data->fast_switch))
>                 return -EINVAL;
>
>         pr_debug("trying to register driver %s\n", driver_data->name);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 26e2eb399484..172ff51c1b2a 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -340,7 +340,10 @@ 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. 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,
> --

Applied as 6.5 material, thanks!