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;
Date:   Tue, 16 May 2023 06:40:01 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Wyes Karny <wyes.karny@amd.com>
Cc:     ray.huang@amd.com, rafael@kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, gautham.shenoy@amd.com
Subject: Re: [PATCH v3 2/2] cpufreq: Warn if fast_switch is not set
Message-ID: <20230516011001.epa4xlvbiimu6ai3@vireshk-i7>
References: <20230515113404.4259-1-wyes.karny@amd.com>
 <20230515113404.4259-3-wyes.karny@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230515113404.4259-3-wyes.karny@amd.com>
Precedence: bulk

On 15-05-23, 11:34, Wyes Karny 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. When
> the frequency invariance is disabled due to some reason governor
> fallbacks to fast_switch if fast_switch_possible is set. This could
> crash the kernel if the driver didn't set the fast_switch function
> pointer.
> 
> This issue becomes apparent when aperf/mperf overflow occurs with
> amd_pstate (passive) + schedutil.  When this happens, kernel disables
> frequency invariance calculation which causes schedutil to fallback to
> sugov_update_single_freq which currently relies on the fast_switch
> callback.
> 
> Normal flow:
>   sugov_update_single_perf
>     cpufreq_driver_adjust_perf
>       cpufreq_driver->adjust_perf
> 
> Error case flow:
>   sugov_update_single_perf
>     sugov_update_single_freq  <-- This is chosen because the freq invariant is disabled due to aperf/mperf overflow
>       cpufreq_driver_fast_switch
>          cpufreq_driver->fast_switch <-- Here NULL pointer dereference is happening, because fast_switch is not set

Not sure if all these details are required for this patch or not. It
is logically incorrect to set fast_switch_possible, while fast_switch
isn't set. That's a reason enough.

> Put up a warning message if the driver sets fast_switch_possible flag
> but not fast_switch.
> 
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> ---
>  drivers/cpufreq/cpufreq.c | 18 ++++++++++++++++++
>  include/linux/cpufreq.h   |  5 ++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..180be9235b08 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -501,6 +501,13 @@ void cpufreq_enable_fast_switch(struct cpufreq_policy *policy)
>  	if (!policy->fast_switch_possible)
>  		return;
>  
> +	/**

Doc style comments aren't required here I guess.

> +	 * It's not expected driver's fast_switch callback is not set
> +	 * even fast_switch_possible is true.
> +	 */
> +	if (!cpufreq_driver_has_fast_switch())
> +		pr_alert_once("fast_switch callback is not set\n");
> +
>  	mutex_lock(&cpufreq_fast_switch_lock);
>  	if (cpufreq_fast_switch_count >= 0) {
>  		cpufreq_fast_switch_count++;
> @@ -2143,6 +2150,17 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
>  
> +/**
> + * cpufreq_driver_has_fast_switch - Check "fast switch" callback.
> + *
> + * Return 'true' if the ->fast_switch callback is present for the
> + * current driver or 'false' otherwise.
> + */
> +bool cpufreq_driver_has_fast_switch(void)

Why create a routine for this, when no one else is going to use it ?

> +{
> +	return !!cpufreq_driver->fast_switch;
> +}

I think you should add the required check in cpufreq_online(), after
cpufreq_driver->init() is called, and return failure if fast_switch
isn't set and fast_switch_possible is.

-- 
viresh