diff mbox series

[v4,1/3] amd_pstate: Add ->fast_switch() callback

Message ID 20230517162817.8538-2-wyes.karny@amd.com
State New
Headers
Series cpufreq/amd-pstate: Fix null pointer dereference when frequency invariance is disabled |

Commit Message

Wyes Karny May 17, 2023, 4:28 p.m. UTC 
  From: "Gautham R. Shenoy" <gautham.shenoy@amd.com>

Schedutil normally calls the adjust_perf callback for drivers with
adjust_perf callback available and fast_switch_possible flag set.
However, when frequency invariance is disabled and schedutil tries to
invoke fast_switch. So, there is a chance of kernel crash if this
function pointer is not set. To protect against this scenario add
fast_switch callback to amd_pstate driver.

Fixes: 1d215f0319c2 ("cpufreq: amd-pstate: Add fast switch function for AMD P-State")

Signed-off-by: Wyes Karny <wyes.karny@amd.com>
Signed-off-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Rafael J. Wysocki May 24, 2023, 5:47 p.m. UTC | #1 
On Wed, May 17, 2023 at 6:29 PM Wyes Karny <wyes.karny@amd.com> wrote:
>
> From: "Gautham R. Shenoy" <gautham.shenoy@amd.com>
>
> Schedutil normally calls the adjust_perf callback for drivers with
> adjust_perf callback available and fast_switch_possible flag set.
> However, when frequency invariance is disabled and schedutil tries to
> invoke fast_switch. So, there is a chance of kernel crash if this
> function pointer is not set. To protect against this scenario add
> fast_switch callback to amd_pstate driver.
>
> Fixes: 1d215f0319c2 ("cpufreq: amd-pstate: Add fast switch function for AMD P-State")
>
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> Signed-off-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 5a3d4aa0f45a..45711fc0a856 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -444,9 +444,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy)
>         return 0;
>  }
>
> -static int amd_pstate_target(struct cpufreq_policy *policy,
> -                            unsigned int target_freq,
> -                            unsigned int relation)
> +static int amd_pstate_update_freq(struct cpufreq_policy *policy,
> +                                 unsigned int target_freq, bool fast_switch)
>  {
>         struct cpufreq_freqs freqs;
>         struct amd_cpudata *cpudata = policy->driver_data;
> @@ -465,14 +464,37 @@ static int amd_pstate_target(struct cpufreq_policy *policy,
>         des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
>                                      cpudata->max_freq);
>
> -       cpufreq_freq_transition_begin(policy, &freqs);
> +       WARN_ON(fast_switch && !policy->fast_switch_enabled);
> +       /*
> +        * If fast_switch is desired, then there aren't any registered
> +        * transition notifiers. See comment for
> +        * cpufreq_enable_fast_switch().
> +        */
> +       if (!fast_switch)
> +               cpufreq_freq_transition_begin(policy, &freqs);
> +
>         amd_pstate_update(cpudata, min_perf, des_perf,
> -                         max_perf, false, policy->governor->flags);
> -       cpufreq_freq_transition_end(policy, &freqs, false);
> +                       max_perf, fast_switch, policy->governor->flags);
> +
> +       if (!fast_switch)
> +               cpufreq_freq_transition_end(policy, &freqs, false);
>
>         return 0;
>  }
>
> +static int amd_pstate_target(struct cpufreq_policy *policy,
> +                            unsigned int target_freq,
> +                            unsigned int relation)
> +{
> +       return amd_pstate_update_freq(policy, target_freq, false);
> +}
> +
> +static unsigned int amd_pstate_fast_switch(struct cpufreq_policy *policy,
> +                                 unsigned int target_freq)
> +{
> +       return amd_pstate_update_freq(policy, target_freq, true);
> +}
> +
>  static void amd_pstate_adjust_perf(unsigned int cpu,
>                                    unsigned long _min_perf,
>                                    unsigned long target_perf,
> @@ -715,6 +737,7 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy)
>
>         freq_qos_remove_request(&cpudata->req[1]);
>         freq_qos_remove_request(&cpudata->req[0]);
> +       policy->fast_switch_possible = false;
>         kfree(cpudata);
>
>         return 0;
> @@ -1309,6 +1332,7 @@ static struct cpufreq_driver amd_pstate_driver = {
>         .flags          = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
>         .verify         = amd_pstate_verify,
>         .target         = amd_pstate_target,
> +       .fast_switch    = amd_pstate_fast_switch,
>         .init           = amd_pstate_cpu_init,
>         .exit           = amd_pstate_cpu_exit,
>         .suspend        = amd_pstate_cpu_suspend,
> --

Applied along with the [2/3], thanks!

Do you need them in 6.4 or would 6.5 be sufficient?  Also do you need
them to go into "stable"?
Wyes Karny May 29, 2023, 6:57 a.m. UTC | #2 
Hi Rafael,

On 24 May 19:47, Rafael J. Wysocki wrote:
> On Wed, May 17, 2023 at 6:29 PM Wyes Karny <wyes.karny@amd.com> wrote:
> >
> > From: "Gautham R. Shenoy" <gautham.shenoy@amd.com>
> >
> > Schedutil normally calls the adjust_perf callback for drivers with
> > adjust_perf callback available and fast_switch_possible flag set.
> > However, when frequency invariance is disabled and schedutil tries to
> > invoke fast_switch. So, there is a chance of kernel crash if this
> > function pointer is not set. To protect against this scenario add
> > fast_switch callback to amd_pstate driver.
> >
> > Fixes: 1d215f0319c2 ("cpufreq: amd-pstate: Add fast switch function for AMD P-State")
> >
> > Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> > Signed-off-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 5a3d4aa0f45a..45711fc0a856 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -444,9 +444,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy)
> >         return 0;
> >  }
> >
> > -static int amd_pstate_target(struct cpufreq_policy *policy,
> > -                            unsigned int target_freq,
> > -                            unsigned int relation)
> > +static int amd_pstate_update_freq(struct cpufreq_policy *policy,
> > +                                 unsigned int target_freq, bool fast_switch)
> >  {
> >         struct cpufreq_freqs freqs;
> >         struct amd_cpudata *cpudata = policy->driver_data;
> > @@ -465,14 +464,37 @@ static int amd_pstate_target(struct cpufreq_policy *policy,
> >         des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
> >                                      cpudata->max_freq);
> >
> > -       cpufreq_freq_transition_begin(policy, &freqs);
> > +       WARN_ON(fast_switch && !policy->fast_switch_enabled);
> > +       /*
> > +        * If fast_switch is desired, then there aren't any registered
> > +        * transition notifiers. See comment for
> > +        * cpufreq_enable_fast_switch().
> > +        */
> > +       if (!fast_switch)
> > +               cpufreq_freq_transition_begin(policy, &freqs);
> > +
> >         amd_pstate_update(cpudata, min_perf, des_perf,
> > -                         max_perf, false, policy->governor->flags);
> > -       cpufreq_freq_transition_end(policy, &freqs, false);
> > +                       max_perf, fast_switch, policy->governor->flags);
> > +
> > +       if (!fast_switch)
> > +               cpufreq_freq_transition_end(policy, &freqs, false);
> >
> >         return 0;
> >  }
> >
> > +static int amd_pstate_target(struct cpufreq_policy *policy,
> > +                            unsigned int target_freq,
> > +                            unsigned int relation)
> > +{
> > +       return amd_pstate_update_freq(policy, target_freq, false);
> > +}
> > +
> > +static unsigned int amd_pstate_fast_switch(struct cpufreq_policy *policy,
> > +                                 unsigned int target_freq)
> > +{
> > +       return amd_pstate_update_freq(policy, target_freq, true);
> > +}
> > +
> >  static void amd_pstate_adjust_perf(unsigned int cpu,
> >                                    unsigned long _min_perf,
> >                                    unsigned long target_perf,
> > @@ -715,6 +737,7 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy)
> >
> >         freq_qos_remove_request(&cpudata->req[1]);
> >         freq_qos_remove_request(&cpudata->req[0]);
> > +       policy->fast_switch_possible = false;
> >         kfree(cpudata);
> >
> >         return 0;
> > @@ -1309,6 +1332,7 @@ static struct cpufreq_driver amd_pstate_driver = {
> >         .flags          = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
> >         .verify         = amd_pstate_verify,
> >         .target         = amd_pstate_target,
> > +       .fast_switch    = amd_pstate_fast_switch,
> >         .init           = amd_pstate_cpu_init,
> >         .exit           = amd_pstate_cpu_exit,
> >         .suspend        = amd_pstate_cpu_suspend,
> > --
> 
> Applied along with the [2/3], thanks!
> 
> Do you need them in 6.4 or would 6.5 be sufficient?  Also do you need
> them to go into "stable"?

Sorry for late reply.
Thanks for picking this for 6.4 and stable.
I see the patch is not applying to the 6.3-stable tree.
I'll check that.

Thanks & Regards,
Wyes
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 5a3d4aa0f45a..45711fc0a856 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -444,9 +444,8 @@  static int amd_pstate_verify(struct cpufreq_policy_data *policy)
 	return 0;
 }
 
-static int amd_pstate_target(struct cpufreq_policy *policy,
-			     unsigned int target_freq,
-			     unsigned int relation)
+static int amd_pstate_update_freq(struct cpufreq_policy *policy,
+				  unsigned int target_freq, bool fast_switch)
 {
 	struct cpufreq_freqs freqs;
 	struct amd_cpudata *cpudata = policy->driver_data;
@@ -465,14 +464,37 @@  static int amd_pstate_target(struct cpufreq_policy *policy,
 	des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
 				     cpudata->max_freq);
 
-	cpufreq_freq_transition_begin(policy, &freqs);
+	WARN_ON(fast_switch && !policy->fast_switch_enabled);
+	/*
+	 * If fast_switch is desired, then there aren't any registered
+	 * transition notifiers. See comment for
+	 * cpufreq_enable_fast_switch().
+	 */
+	if (!fast_switch)
+		cpufreq_freq_transition_begin(policy, &freqs);
+
 	amd_pstate_update(cpudata, min_perf, des_perf,
-			  max_perf, false, policy->governor->flags);
-	cpufreq_freq_transition_end(policy, &freqs, false);
+			max_perf, fast_switch, policy->governor->flags);
+
+	if (!fast_switch)
+		cpufreq_freq_transition_end(policy, &freqs, false);
 
 	return 0;
 }
 
+static int amd_pstate_target(struct cpufreq_policy *policy,
+			     unsigned int target_freq,
+			     unsigned int relation)
+{
+	return amd_pstate_update_freq(policy, target_freq, false);
+}
+
+static unsigned int amd_pstate_fast_switch(struct cpufreq_policy *policy,
+				  unsigned int target_freq)
+{
+	return amd_pstate_update_freq(policy, target_freq, true);
+}
+
 static void amd_pstate_adjust_perf(unsigned int cpu,
 				   unsigned long _min_perf,
 				   unsigned long target_perf,
@@ -715,6 +737,7 @@  static int amd_pstate_cpu_exit(struct cpufreq_policy *policy)
 
 	freq_qos_remove_request(&cpudata->req[1]);
 	freq_qos_remove_request(&cpudata->req[0]);
+	policy->fast_switch_possible = false;
 	kfree(cpudata);
 
 	return 0;
@@ -1309,6 +1332,7 @@  static struct cpufreq_driver amd_pstate_driver = {
 	.flags		= CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
 	.verify		= amd_pstate_verify,
 	.target		= amd_pstate_target,
+	.fast_switch    = amd_pstate_fast_switch,
 	.init		= amd_pstate_cpu_init,
 	.exit		= amd_pstate_cpu_exit,
 	.suspend	= amd_pstate_cpu_suspend,