[PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Chen Yu]

On Dell Inc. XPS13 9333, the BIOS changes the value of
MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
the power source changes), the maximum frequency of the
CPU is not updated accordingly. This is due to the policy's
cpuinfo.max is not updated when _PPC notifier fires.

Fix this problem by updating the policy's cpuinfo.max
and broadcast the _PPC notifier to all online CPUs.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
 drivers/cpufreq/cpufreq.c        |  2 ++
 drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index a303fd0e108c..737dbf5aa7f7 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
 MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
 		 "limited by BIOS, this should help");
 
+static int broadcast_ppc;
+module_param(broadcast_ppc, int, 0644);
+MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
+
 #define PPC_REGISTERED   1
 #define PPC_IN_USE       2
 
@@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
 		else
 			acpi_processor_ppc_ost(pr->handle, 0);
 	}
-	if (ret >= 0)
-		cpufreq_update_policy(pr->id);
+	if (ret >= 0) {
+		if (broadcast_ppc) {
+			int cpu;
+
+			for_each_possible_cpu(cpu)
+				cpufreq_update_policy(cpu);
+		} else {
+			cpufreq_update_policy(pr->id);
+		}
+	}
 }
 
 int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e35a886e00bc..95e08816b512 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 
 	policy->min = new_policy->min;
 	policy->max = new_policy->max;
+	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
+	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
 	trace_cpu_frequency_limits(policy);
 
 	policy->cached_target_freq = UINT_MAX;
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index dd66decf2087..e1881313c396 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
 
 static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
 {
+	int max_freq;
 	struct cpudata *cpu = all_cpu_data[policy->cpu];
 
 	update_turbo_state();
+	max_freq = intel_pstate_get_max_freq(cpu);
+
+	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
+	    max_freq != policy->cpuinfo.max_freq) {
+		/*
+		 * System was not running under any constraints, but the
+		 * current max possible frequency is changed. So reset
+		 * policy limits.
+		 */
+		policy->cpuinfo.max_freq = policy->max = max_freq;
+	}
+
 	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
-				     intel_pstate_get_max_freq(cpu));
+				     max_freq);
 
 	if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
 	    policy->policy != CPUFREQ_POLICY_PERFORMANCE)
-- 
2.17.1

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Viresh Kumar]

On 09-02-19, 20:02, Chen Yu wrote:
> On Dell Inc. XPS13 9333, the BIOS changes the value of
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> the power source changes), the maximum frequency of the
> CPU is not updated accordingly. This is due to the policy's
> cpuinfo.max is not updated when _PPC notifier fires.
> 
> Fix this problem by updating the policy's cpuinfo.max
> and broadcast the _PPC notifier to all online CPUs.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
>  drivers/cpufreq/cpufreq.c        |  2 ++
>  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index a303fd0e108c..737dbf5aa7f7 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
>  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
>  		 "limited by BIOS, this should help");
>  
> +static int broadcast_ppc;
> +module_param(broadcast_ppc, int, 0644);
> +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> +
>  #define PPC_REGISTERED   1
>  #define PPC_IN_USE       2
>  
> @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
>  		else
>  			acpi_processor_ppc_ost(pr->handle, 0);
>  	}
> -	if (ret >= 0)
> -		cpufreq_update_policy(pr->id);
> +	if (ret >= 0) {
> +		if (broadcast_ppc) {
> +			int cpu;
> +
> +			for_each_possible_cpu(cpu)
> +				cpufreq_update_policy(cpu);
> +		} else {
> +			cpufreq_update_policy(pr->id);
> +		}
> +	}
>  }
>  
>  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..95e08816b512 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>  
>  	policy->min = new_policy->min;
>  	policy->max = new_policy->max;
> +	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> +	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
>  	trace_cpu_frequency_limits(policy);
>  
>  	policy->cached_target_freq = UINT_MAX;
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index dd66decf2087..e1881313c396 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
>  
>  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
>  {
> +	int max_freq;
>  	struct cpudata *cpu = all_cpu_data[policy->cpu];
>  
>  	update_turbo_state();
> +	max_freq = intel_pstate_get_max_freq(cpu);
> +
> +	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> +	    max_freq != policy->cpuinfo.max_freq) {
> +		/*
> +		 * System was not running under any constraints, but the
> +		 * current max possible frequency is changed. So reset
> +		 * policy limits.
> +		 */
> +		policy->cpuinfo.max_freq = policy->max = max_freq;
> +	}
> +
>  	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> -				     intel_pstate_get_max_freq(cpu));
> +				     max_freq);
>  
>  	if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
>  	    policy->policy != CPUFREQ_POLICY_PERFORMANCE)

By TURBO I believe this is about boost-frequencies and you should use
that infrastructure to make it work, isn't it ?

-- 
viresh

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Rafael J. Wysocki]

On Mon, Feb 11, 2019 at 10:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-02-19, 20:02, Chen Yu wrote:
> > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > the power source changes), the maximum frequency of the
> > CPU is not updated accordingly. This is due to the policy's
> > cpuinfo.max is not updated when _PPC notifier fires.
> >
> > Fix this problem by updating the policy's cpuinfo.max
> > and broadcast the _PPC notifier to all online CPUs.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> >  drivers/cpufreq/cpufreq.c        |  2 ++
> >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index a303fd0e108c..737dbf5aa7f7 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> >                "limited by BIOS, this should help");
> >
> > +static int broadcast_ppc;
> > +module_param(broadcast_ppc, int, 0644);
> > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > +
> >  #define PPC_REGISTERED   1
> >  #define PPC_IN_USE       2
> >
> > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> >               else
> >                       acpi_processor_ppc_ost(pr->handle, 0);
> >       }
> > -     if (ret >= 0)
> > -             cpufreq_update_policy(pr->id);
> > +     if (ret >= 0) {
> > +             if (broadcast_ppc) {
> > +                     int cpu;
> > +
> > +                     for_each_possible_cpu(cpu)
> > +                             cpufreq_update_policy(cpu);
> > +             } else {
> > +                     cpufreq_update_policy(pr->id);
> > +             }
> > +     }
> >  }
> >
> >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index e35a886e00bc..95e08816b512 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >
> >       policy->min = new_policy->min;
> >       policy->max = new_policy->max;
> > +     policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > +     policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> >       trace_cpu_frequency_limits(policy);
> >
> >       policy->cached_target_freq = UINT_MAX;
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index dd66decf2087..e1881313c396 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> >
> >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> >  {
> > +     int max_freq;
> >       struct cpudata *cpu = all_cpu_data[policy->cpu];
> >
> >       update_turbo_state();
> > +     max_freq = intel_pstate_get_max_freq(cpu);
> > +
> > +     if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > +         max_freq != policy->cpuinfo.max_freq) {
> > +             /*
> > +              * System was not running under any constraints, but the
> > +              * current max possible frequency is changed. So reset
> > +              * policy limits.
> > +              */
> > +             policy->cpuinfo.max_freq = policy->max = max_freq;
> > +     }
> > +
> >       cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> > -                                  intel_pstate_get_max_freq(cpu));
> > +                                  max_freq);
> >
> >       if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
> >           policy->policy != CPUFREQ_POLICY_PERFORMANCE)
>
> By TURBO I believe this is about boost-frequencies and you should use
> that infrastructure to make it work, isn't it ?

I guess you mean the the "boost" attribute in the core, but that's not
applicable to intel_pstate.

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Viresh Kumar]

On 11-02-19, 11:20, Rafael J. Wysocki wrote:
> On Mon, Feb 11, 2019 at 10:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 09-02-19, 20:02, Chen Yu wrote:
> > > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > > the power source changes), the maximum frequency of the
> > > CPU is not updated accordingly. This is due to the policy's
> > > cpuinfo.max is not updated when _PPC notifier fires.
> > >
> > > Fix this problem by updating the policy's cpuinfo.max
> > > and broadcast the _PPC notifier to all online CPUs.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > >  drivers/cpufreq/cpufreq.c        |  2 ++
> > >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> > >  3 files changed, 30 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > > index a303fd0e108c..737dbf5aa7f7 100644
> > > --- a/drivers/acpi/processor_perflib.c
> > > +++ b/drivers/acpi/processor_perflib.c
> > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > >                "limited by BIOS, this should help");
> > >
> > > +static int broadcast_ppc;
> > > +module_param(broadcast_ppc, int, 0644);
> > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > > +
> > >  #define PPC_REGISTERED   1
> > >  #define PPC_IN_USE       2
> > >
> > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > >               else
> > >                       acpi_processor_ppc_ost(pr->handle, 0);
> > >       }
> > > -     if (ret >= 0)
> > > -             cpufreq_update_policy(pr->id);
> > > +     if (ret >= 0) {
> > > +             if (broadcast_ppc) {
> > > +                     int cpu;
> > > +
> > > +                     for_each_possible_cpu(cpu)
> > > +                             cpufreq_update_policy(cpu);
> > > +             } else {
> > > +                     cpufreq_update_policy(pr->id);
> > > +             }
> > > +     }
> > >  }
> > >
> > >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e35a886e00bc..95e08816b512 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> > >
> > >       policy->min = new_policy->min;
> > >       policy->max = new_policy->max;
> > > +     policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > > +     policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > >       trace_cpu_frequency_limits(policy);
> > >
> > >       policy->cached_target_freq = UINT_MAX;
> > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > index dd66decf2087..e1881313c396 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> > >
> > >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > >  {
> > > +     int max_freq;
> > >       struct cpudata *cpu = all_cpu_data[policy->cpu];
> > >
> > >       update_turbo_state();
> > > +     max_freq = intel_pstate_get_max_freq(cpu);
> > > +
> > > +     if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > > +         max_freq != policy->cpuinfo.max_freq) {
> > > +             /*
> > > +              * System was not running under any constraints, but the
> > > +              * current max possible frequency is changed. So reset
> > > +              * policy limits.
> > > +              */
> > > +             policy->cpuinfo.max_freq = policy->max = max_freq;
> > > +     }
> > > +
> > >       cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> > > -                                  intel_pstate_get_max_freq(cpu));
> > > +                                  max_freq);
> > >
> > >       if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
> > >           policy->policy != CPUFREQ_POLICY_PERFORMANCE)
> >
> > By TURBO I believe this is about boost-frequencies and you should use
> > that infrastructure to make it work, isn't it ?
> 
> I guess you mean the the "boost" attribute in the core, but that's not
> applicable to intel_pstate.

Ahh, I missed the part that this is for the !has_target() platforms :(

-- 
viresh

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Viresh Kumar]

On 09-02-19, 20:02, Chen Yu wrote:
> On Dell Inc. XPS13 9333, the BIOS changes the value of
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> the power source changes), the maximum frequency of the
> CPU is not updated accordingly. This is due to the policy's
> cpuinfo.max is not updated when _PPC notifier fires.
> 
> Fix this problem by updating the policy's cpuinfo.max
> and broadcast the _PPC notifier to all online CPUs.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
>  drivers/cpufreq/cpufreq.c        |  2 ++
>  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index a303fd0e108c..737dbf5aa7f7 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
>  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
>  		 "limited by BIOS, this should help");
>  
> +static int broadcast_ppc;
> +module_param(broadcast_ppc, int, 0644);
> +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> +
>  #define PPC_REGISTERED   1
>  #define PPC_IN_USE       2
>  
> @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
>  		else
>  			acpi_processor_ppc_ost(pr->handle, 0);
>  	}
> -	if (ret >= 0)
> -		cpufreq_update_policy(pr->id);
> +	if (ret >= 0) {
> +		if (broadcast_ppc) {
> +			int cpu;
> +
> +			for_each_possible_cpu(cpu)
> +				cpufreq_update_policy(cpu);
> +		} else {
> +			cpufreq_update_policy(pr->id);
> +		}
> +	}
>  }
>  
>  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..95e08816b512 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>  
>  	policy->min = new_policy->min;
>  	policy->max = new_policy->max;
> +	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> +	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
>  	trace_cpu_frequency_limits(policy);
>  
>  	policy->cached_target_freq = UINT_MAX;
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index dd66decf2087..e1881313c396 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
>  
>  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
>  {
> +	int max_freq;
>  	struct cpudata *cpu = all_cpu_data[policy->cpu];
>  
>  	update_turbo_state();
> +	max_freq = intel_pstate_get_max_freq(cpu);
> +
> +	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&

Why do have this check for policy->max here ?

> +	    max_freq != policy->cpuinfo.max_freq) {
> +		/*
> +		 * System was not running under any constraints, but the
> +		 * current max possible frequency is changed. So reset
> +		 * policy limits.
> +		 */
> +		policy->cpuinfo.max_freq = policy->max = max_freq;
> +	}
> +
>  	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> -				     intel_pstate_get_max_freq(cpu));
> +				     max_freq);
>  
>  	if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
>  	    policy->policy != CPUFREQ_POLICY_PERFORMANCE)
> -- 
> 2.17.1

-- 
viresh

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Rafael J. Wysocki]

On Sat, Feb 9, 2019 at 12:54 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> On Dell Inc. XPS13 9333, the BIOS changes the value of
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> the power source changes), the maximum frequency of the
> CPU is not updated accordingly. This is due to the policy's
> cpuinfo.max is not updated when _PPC notifier fires.
>
> Fix this problem by updating the policy's cpuinfo.max
> and broadcast the _PPC notifier to all online CPUs.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
>  drivers/cpufreq/cpufreq.c        |  2 ++
>  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> index a303fd0e108c..737dbf5aa7f7 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
>  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
>                  "limited by BIOS, this should help");
>
> +static int broadcast_ppc;
> +module_param(broadcast_ppc, int, 0644);
> +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> +
>  #define PPC_REGISTERED   1
>  #define PPC_IN_USE       2
>
> @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
>                 else
>                         acpi_processor_ppc_ost(pr->handle, 0);
>         }
> -       if (ret >= 0)
> -               cpufreq_update_policy(pr->id);
> +       if (ret >= 0) {
> +               if (broadcast_ppc) {
> +                       int cpu;
> +
> +                       for_each_possible_cpu(cpu)
> +                               cpufreq_update_policy(cpu);
> +               } else {
> +                       cpufreq_update_policy(pr->id);
> +               }
> +       }
>  }
>
>  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..95e08816b512 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>
>         policy->min = new_policy->min;
>         policy->max = new_policy->max;
> +       policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> +       policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
>         trace_cpu_frequency_limits(policy);
>
>         policy->cached_target_freq = UINT_MAX;
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index dd66decf2087..e1881313c396 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
>
>  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
>  {
> +       int max_freq;
>         struct cpudata *cpu = all_cpu_data[policy->cpu];
>
>         update_turbo_state();

Well, update_turbo_state() should handle the case at hand already.

That's what it's for actually: It checks if
MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets
global.turbo_disabled is that's the case.

Why isn't that sufficient?

> +       max_freq = intel_pstate_get_max_freq(cpu);
> +
> +       if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> +           max_freq != policy->cpuinfo.max_freq) {
> +               /*
> +                * System was not running under any constraints, but the
> +                * current max possible frequency is changed. So reset
> +                * policy limits.
> +                */
> +               policy->cpuinfo.max_freq = policy->max = max_freq;
> +       }

Why does policy->cpuinfo.max_freq need to be updated?

> +
>         cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
> -                                    intel_pstate_get_max_freq(cpu));
> +                                    max_freq);
>
>         if (policy->policy != CPUFREQ_POLICY_POWERSAVE &&
>             policy->policy != CPUFREQ_POLICY_PERFORMANCE)
> --

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Yu Chen]

Hi Rafael,
On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote:
> On Sat, Feb 9, 2019 at 12:54 PM Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > the power source changes), the maximum frequency of the
> > CPU is not updated accordingly. This is due to the policy's
> > cpuinfo.max is not updated when _PPC notifier fires.
> >
> > Fix this problem by updating the policy's cpuinfo.max
> > and broadcast the _PPC notifier to all online CPUs.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> >  drivers/cpufreq/cpufreq.c        |  2 ++
> >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index a303fd0e108c..737dbf5aa7f7 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> >                  "limited by BIOS, this should help");
> >
> > +static int broadcast_ppc;
> > +module_param(broadcast_ppc, int, 0644);
> > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > +
> >  #define PPC_REGISTERED   1
> >  #define PPC_IN_USE       2
> >
> > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> >                 else
> >                         acpi_processor_ppc_ost(pr->handle, 0);
> >         }
> > -       if (ret >= 0)
> > -               cpufreq_update_policy(pr->id);
> > +       if (ret >= 0) {
> > +               if (broadcast_ppc) {
> > +                       int cpu;
> > +
> > +                       for_each_possible_cpu(cpu)
> > +                               cpufreq_update_policy(cpu);
> > +               } else {
> > +                       cpufreq_update_policy(pr->id);
> > +               }
> > +       }
> >  }
> >
> >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index e35a886e00bc..95e08816b512 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >
> >         policy->min = new_policy->min;
> >         policy->max = new_policy->max;
> > +       policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > +       policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> >         trace_cpu_frequency_limits(policy);
> >
> >         policy->cached_target_freq = UINT_MAX;
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index dd66decf2087..e1881313c396 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> >
> >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> >  {
> > +       int max_freq;
> >         struct cpudata *cpu = all_cpu_data[policy->cpu];
> >
> >         update_turbo_state();
> 
> Well, update_turbo_state() should handle the case at hand already.
> 
> That's what it's for actually: It checks if
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets
> global.turbo_disabled is that's the case.
> 
> Why isn't that sufficient?
> 
update_turbo_state() changes the flag of global.turbo_diabled but we
need to also leverage it to adjust the policy.max accordingly. This is why
we add intel_pstate_get_max_freq() to get the updated max freq in
intel_pstate_verify_policy().
> > +       max_freq = intel_pstate_get_max_freq(cpu);
> > +
> > +       if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > +           max_freq != policy->cpuinfo.max_freq) {
> > +               /*
> > +                * System was not running under any constraints, but the
> > +                * current max possible frequency is changed. So reset
> > +                * policy limits.
> > +                */
> > +               policy->cpuinfo.max_freq = policy->max = max_freq;
> > +       }
> 
> Why does policy->cpuinfo.max_freq need to be updated?
> 
This is my understanding:
There's a corner case that, if the system boots with battery,
the max cpu frequency will not scale up if we plug the AC later.
According to the log provided by Gabriele Mazzotta,  if the system
boot up with battery, then plug the AC after boot up, the max perf ratio
and policy->cpuinfo.max will remain 17 rather than increasing to
30(when AC plugged thus turbo enabled):

[   52.158810] CPU 0: _PPC is 6 - frequency  limited
[   52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000
[   52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17
[   52.158827] intel_pstate: cpu:0 global_min:8 global_max:30
[   52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8

This is caused by:
In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated
which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes,
we might need to also update the policy->cpuinfo.max to tell user that the hardware
status has changed.

So we give it a chance to adjust the policy.cpuinfo.max and policy.max in
cpufreq_driver->verify()  according to turbo status, this is what this patch mainly
aims to do.

Besides, since on this platform there's only one _PPC notification for one CPU, it is
necessary to broadcast the notification to all CPUs on this package. And this patch
broadcast it to all online CPUs to make the change simpler.

Best,
Yu

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Yu Chen]

Hi Viresh,
On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote:
> On 09-02-19, 20:02, Chen Yu wrote:
> > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > the power source changes), the maximum frequency of the
> > CPU is not updated accordingly. This is due to the policy's
> > cpuinfo.max is not updated when _PPC notifier fires.
> > 
> > Fix this problem by updating the policy's cpuinfo.max
> > and broadcast the _PPC notifier to all online CPUs.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> >  drivers/cpufreq/cpufreq.c        |  2 ++
> >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index a303fd0e108c..737dbf5aa7f7 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> >  		 "limited by BIOS, this should help");
> >  
> > +static int broadcast_ppc;
> > +module_param(broadcast_ppc, int, 0644);
> > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > +
> >  #define PPC_REGISTERED   1
> >  #define PPC_IN_USE       2
> >  
> > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> >  		else
> >  			acpi_processor_ppc_ost(pr->handle, 0);
> >  	}
> > -	if (ret >= 0)
> > -		cpufreq_update_policy(pr->id);
> > +	if (ret >= 0) {
> > +		if (broadcast_ppc) {
> > +			int cpu;
> > +
> > +			for_each_possible_cpu(cpu)
> > +				cpufreq_update_policy(cpu);
> > +		} else {
> > +			cpufreq_update_policy(pr->id);
> > +		}
> > +	}
> >  }
> >  
> >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index e35a886e00bc..95e08816b512 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >  
> >  	policy->min = new_policy->min;
> >  	policy->max = new_policy->max;
> > +	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > +	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> >  	trace_cpu_frequency_limits(policy);
> >  
> >  	policy->cached_target_freq = UINT_MAX;
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index dd66decf2087..e1881313c396 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> >  
> >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> >  {
> > +	int max_freq;
> >  	struct cpudata *cpu = all_cpu_data[policy->cpu];
> >  
> >  	update_turbo_state();
> > +	max_freq = intel_pstate_get_max_freq(cpu);
> > +
> > +	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> 
> Why do have this check for policy->max here ?
>
Thanks for looking at this change, I've replied to another email in detail of
the scenario that, this is due to corner case that if the system boots
with battery and plug the AC after boot up, the cpufreq max limit will not
increase even the turbo has been enabled after the AC plugged.

Best,
Yu

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Viresh Kumar]

On 14-02-19, 00:55, Yu Chen wrote:
> Hi Viresh,
> On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote:
> > On 09-02-19, 20:02, Chen Yu wrote:
> > > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > > the power source changes), the maximum frequency of the
> > > CPU is not updated accordingly. This is due to the policy's
> > > cpuinfo.max is not updated when _PPC notifier fires.
> > > 
> > > Fix this problem by updating the policy's cpuinfo.max
> > > and broadcast the _PPC notifier to all online CPUs.
> > > 
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > >  drivers/cpufreq/cpufreq.c        |  2 ++
> > >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> > >  3 files changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > > index a303fd0e108c..737dbf5aa7f7 100644
> > > --- a/drivers/acpi/processor_perflib.c
> > > +++ b/drivers/acpi/processor_perflib.c
> > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > >  		 "limited by BIOS, this should help");
> > >  
> > > +static int broadcast_ppc;
> > > +module_param(broadcast_ppc, int, 0644);
> > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > > +
> > >  #define PPC_REGISTERED   1
> > >  #define PPC_IN_USE       2
> > >  
> > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > >  		else
> > >  			acpi_processor_ppc_ost(pr->handle, 0);
> > >  	}
> > > -	if (ret >= 0)
> > > -		cpufreq_update_policy(pr->id);
> > > +	if (ret >= 0) {
> > > +		if (broadcast_ppc) {
> > > +			int cpu;
> > > +
> > > +			for_each_possible_cpu(cpu)
> > > +				cpufreq_update_policy(cpu);
> > > +		} else {
> > > +			cpufreq_update_policy(pr->id);
> > > +		}
> > > +	}
> > >  }
> > >  
> > >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e35a886e00bc..95e08816b512 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> > >  
> > >  	policy->min = new_policy->min;
> > >  	policy->max = new_policy->max;
> > > +	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > > +	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > >  	trace_cpu_frequency_limits(policy);
> > >  
> > >  	policy->cached_target_freq = UINT_MAX;
> > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > index dd66decf2087..e1881313c396 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> > >  
> > >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > >  {
> > > +	int max_freq;
> > >  	struct cpudata *cpu = all_cpu_data[policy->cpu];
> > >  
> > >  	update_turbo_state();
> > > +	max_freq = intel_pstate_get_max_freq(cpu);
> > > +
> > > +	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > 
> > Why do have this check for policy->max here ?
> >
> Thanks for looking at this change, I've replied to another email in detail of
> the scenario that, this is due to corner case that if the system boots
> with battery and plug the AC after boot up, the cpufreq max limit will not
> increase even the turbo has been enabled after the AC plugged.

Yeah, but I asked a different question I believe, why is this
comparison necessary ?

policy->max == policy->cpuinfo.max_freq

-- 
viresh

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Rafael J. Wysocki]

 On Wed, Feb 13, 2019 at 5:44 PM Yu Chen <yu.c.chen@intel.com> wrote:
>
> Hi Rafael,
> On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote:
> > On Sat, Feb 9, 2019 at 12:54 PM Chen Yu <yu.c.chen@intel.com> wrote:
> > >
> > > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > > the power source changes), the maximum frequency of the
> > > CPU is not updated accordingly. This is due to the policy's
> > > cpuinfo.max is not updated when _PPC notifier fires.
> > >
> > > Fix this problem by updating the policy's cpuinfo.max
> > > and broadcast the _PPC notifier to all online CPUs.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > >  drivers/cpufreq/cpufreq.c        |  2 ++
> > >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> > >  3 files changed, 30 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > > index a303fd0e108c..737dbf5aa7f7 100644
> > > --- a/drivers/acpi/processor_perflib.c
> > > +++ b/drivers/acpi/processor_perflib.c
> > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > >                  "limited by BIOS, this should help");
> > >
> > > +static int broadcast_ppc;
> > > +module_param(broadcast_ppc, int, 0644);
> > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > > +
> > >  #define PPC_REGISTERED   1
> > >  #define PPC_IN_USE       2
> > >
> > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > >                 else
> > >                         acpi_processor_ppc_ost(pr->handle, 0);
> > >         }
> > > -       if (ret >= 0)
> > > -               cpufreq_update_policy(pr->id);
> > > +       if (ret >= 0) {
> > > +               if (broadcast_ppc) {
> > > +                       int cpu;
> > > +
> > > +                       for_each_possible_cpu(cpu)
> > > +                               cpufreq_update_policy(cpu);
> > > +               } else {
> > > +                       cpufreq_update_policy(pr->id);
> > > +               }
> > > +       }
> > >  }
> > >
> > >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e35a886e00bc..95e08816b512 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> > >
> > >         policy->min = new_policy->min;
> > >         policy->max = new_policy->max;
> > > +       policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > > +       policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > >         trace_cpu_frequency_limits(policy);
> > >
> > >         policy->cached_target_freq = UINT_MAX;
> > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > index dd66decf2087..e1881313c396 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> > >
> > >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > >  {
> > > +       int max_freq;
> > >         struct cpudata *cpu = all_cpu_data[policy->cpu];
> > >
> > >         update_turbo_state();
> >
> > Well, update_turbo_state() should handle the case at hand already.
> >
> > That's what it's for actually: It checks if
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets
> > global.turbo_disabled is that's the case.
> >
> > Why isn't that sufficient?
> >
> update_turbo_state() changes the flag of global.turbo_diabled but we
> need to also leverage it to adjust the policy.max accordingly. This is why
> we add intel_pstate_get_max_freq() to get the updated max freq in
> intel_pstate_verify_policy().

Yes, that's why intel_pstate_verify_policy() passes the return value
of intel_pstate_get_max_freq() as the second arg
cpufreq_verify_within_limits(), so really my question was about why
cpuinfo.max_freq needed to be updated (below).

> > > +       max_freq = intel_pstate_get_max_freq(cpu);
> > > +
> > > +       if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > > +           max_freq != policy->cpuinfo.max_freq) {
> > > +               /*
> > > +                * System was not running under any constraints, but the
> > > +                * current max possible frequency is changed. So reset
> > > +                * policy limits.
> > > +                */
> > > +               policy->cpuinfo.max_freq = policy->max = max_freq;
> > > +       }
> >
> > Why does policy->cpuinfo.max_freq need to be updated?
> >
> This is my understanding:
> There's a corner case that, if the system boots with battery,
> the max cpu frequency will not scale up if we plug the AC later.

I see.  The *initial* cpuinfo.max_freq may be too low.  This part is
missing from your patch changelog.

The driver is not expected to update cpuinfo.max_freq after init.
That may not actually break anything, even though it is racy in
principle, but if it is done, it needs to be done in the "passive"
mode too and that may be more problematic.

Anyway, this is more fundamental than you seem to be thinking.

> According to the log provided by Gabriele Mazzotta,  if the system
> boot up with battery, then plug the AC after boot up, the max perf ratio
> and policy->cpuinfo.max will remain 17 rather than increasing to
> 30(when AC plugged thus turbo enabled):
>
> [   52.158810] CPU 0: _PPC is 6 - frequency  limited
> [   52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000
> [   52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17
> [   52.158827] intel_pstate: cpu:0 global_min:8 global_max:30
> [   52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8
>
> This is caused by:
> In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated
> which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes,
> we might need to also update the policy->cpuinfo.max to tell user that the hardware
> status has changed.
>
> So we give it a chance to adjust the policy.cpuinfo.max and policy.max in
> cpufreq_driver->verify()  according to turbo status, this is what this patch mainly
> aims to do.
>
> Besides, since on this platform there's only one _PPC notification for one CPU, it is
> necessary to broadcast the notification to all CPUs on this package. And this patch
> broadcast it to all online CPUs to make the change simpler.

You're trying to make two substantial changes in one go, broadcasting
_PPC and updating cpuinfo.max_freq.  Don't do that, they need to be
separate changes.

Moreover, we may want to address the initial cpuinfo.max_freq issue in
a different way.

Thanks,
Rafael

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Yu Chen]

On Thu, Feb 14, 2019 at 12:36:48PM +0530, Viresh Kumar wrote:
> On 14-02-19, 00:55, Yu Chen wrote:
> > Hi Viresh,
> > On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote:
> > > On 09-02-19, 20:02, Chen Yu wrote:
> > > > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > > > the power source changes), the maximum frequency of the
> > > > CPU is not updated accordingly. This is due to the policy's
> > > > cpuinfo.max is not updated when _PPC notifier fires.
> > > > 
> > > > Fix this problem by updating the policy's cpuinfo.max
> > > > and broadcast the _PPC notifier to all online CPUs.
> > > > 
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > > > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > ---
> > > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > > >  drivers/cpufreq/cpufreq.c        |  2 ++
> > > >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> > > >  3 files changed, 30 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > > > index a303fd0e108c..737dbf5aa7f7 100644
> > > > --- a/drivers/acpi/processor_perflib.c
> > > > +++ b/drivers/acpi/processor_perflib.c
> > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > > >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > > >  		 "limited by BIOS, this should help");
> > > >  
> > > > +static int broadcast_ppc;
> > > > +module_param(broadcast_ppc, int, 0644);
> > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > > > +
> > > >  #define PPC_REGISTERED   1
> > > >  #define PPC_IN_USE       2
> > > >  
> > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > > >  		else
> > > >  			acpi_processor_ppc_ost(pr->handle, 0);
> > > >  	}
> > > > -	if (ret >= 0)
> > > > -		cpufreq_update_policy(pr->id);
> > > > +	if (ret >= 0) {
> > > > +		if (broadcast_ppc) {
> > > > +			int cpu;
> > > > +
> > > > +			for_each_possible_cpu(cpu)
> > > > +				cpufreq_update_policy(cpu);
> > > > +		} else {
> > > > +			cpufreq_update_policy(pr->id);
> > > > +		}
> > > > +	}
> > > >  }
> > > >  
> > > >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index e35a886e00bc..95e08816b512 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> > > >  
> > > >  	policy->min = new_policy->min;
> > > >  	policy->max = new_policy->max;
> > > > +	policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > > > +	policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > > >  	trace_cpu_frequency_limits(policy);
> > > >  
> > > >  	policy->cached_target_freq = UINT_MAX;
> > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > > index dd66decf2087..e1881313c396 100644
> > > > --- a/drivers/cpufreq/intel_pstate.c
> > > > +++ b/drivers/cpufreq/intel_pstate.c
> > > > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> > > >  
> > > >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > > >  {
> > > > +	int max_freq;
> > > >  	struct cpudata *cpu = all_cpu_data[policy->cpu];
> > > >  
> > > >  	update_turbo_state();
> > > > +	max_freq = intel_pstate_get_max_freq(cpu);
> > > > +
> > > > +	if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > > 
> > > Why do have this check for policy->max here ?
> > >
> > Thanks for looking at this change, I've replied to another email in detail of
> > the scenario that, this is due to corner case that if the system boots
> > with battery and plug the AC after boot up, the cpufreq max limit will not
> > increase even the turbo has been enabled after the AC plugged.
> 
> Yeah, but I asked a different question I believe, why is this
> comparison necessary ?
>
Sorry for late response, I was caught in another issue.

The reason for why checking if policy->max equals to policy->cpuinfo.max_freq
is to bypass unnecessary assignment(it turns out to be a hacky
logic here):

Say, if the system boots up with AC attached, after bootup the cpuinfo.max
is always the highest, and there is no need to reassign the cpuinfo.max.

However if the system boots up with AC deattached, then the cpuinfo.max
will not scale up if the AC is attached after bootup.

Thus in former case, the cpuinfo.max does not equals to policy.max, and
we don't need to update cpuinfo.max. While in latter case, the cpuinfo.max
might equal to policy.max, and in this situation we need to update the
cpuinfo.max.


Case 1: Boot up AC attached, cpuinfo.max does not equal to policy.max

# Unplug

[   25.775643] CPU 0: _PPC is 6 - frequency  limited
[   25.775660] intel_pstate: set_policy cpuinfo.max 3000000 policy->max 1700000
[   25.775666] intel_pstate: cpu:0 max_state 17 min_policy_perf:8 max_policy_perf:17
[   25.775670] intel_pstate: cpu:0 global_min:8 global_max:30
[   25.775674] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8

"REDUCED FREQUENCY ABOVE AFTER UNPLUG"


# Re-plug

[   36.979264] CPU 0: _PPC is 6 - frequency  limited
[   36.979276] intel_pstate: policy->max > max non turbo frequency
[   36.979280] intel_pstate: set_policy cpuinfo.max 3000000 policy->max 3000000
[   36.979283] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:30
[   36.979286] intel_pstate: cpu:0 global_min:8 global_max:30
[   36.979289] intel_pstate: cpu:0 max_perf_ratio:30 min_perf_ratio:8


Case 2: boot up without AC attached, cpuinfo.max equals to policy.max:


# Plug

[   52.158810] CPU 0: _PPC is 6 - frequency  limited
[   52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000
[   52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17
[   52.158827] intel_pstate: cpu:0 global_min:8 global_max:30
[   52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8


As we see, the check is a little hacky, because it does not do with the logic
on how to check if the system's cpuinfo.max is abnormal. In v2 version, I've
removed this check to make the code easier to be understood.


Best,
Yu

> policy->max == policy->cpuinfo.max_freq
> 
> -- 
> viresh

Re: [PATCH][RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs

[Yu Chen]

On Thu, Feb 14, 2019 at 11:21:13AM +0100, Rafael J. Wysocki wrote:
>  On Wed, Feb 13, 2019 at 5:44 PM Yu Chen <yu.c.chen@intel.com> wrote:
> >
> > Hi Rafael,
> > On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote:
> > > On Sat, Feb 9, 2019 at 12:54 PM Chen Yu <yu.c.chen@intel.com> wrote:
> > > >
> > > > On Dell Inc. XPS13 9333, the BIOS changes the value of
> > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when
> > > > the power source changes), the maximum frequency of the
> > > > CPU is not updated accordingly. This is due to the policy's
> > > > cpuinfo.max is not updated when _PPC notifier fires.
> > > >
> > > > Fix this problem by updating the policy's cpuinfo.max
> > > > and broadcast the _PPC notifier to all online CPUs.
> > > >
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759
> > > > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > ---
> > > >  drivers/acpi/processor_perflib.c | 16 ++++++++++++++--
> > > >  drivers/cpufreq/cpufreq.c        |  2 ++
> > > >  drivers/cpufreq/intel_pstate.c   | 15 ++++++++++++++-
> > > >  3 files changed, 30 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > > > index a303fd0e108c..737dbf5aa7f7 100644
> > > > --- a/drivers/acpi/processor_perflib.c
> > > > +++ b/drivers/acpi/processor_perflib.c
> > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644);
> > > >  MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \
> > > >                  "limited by BIOS, this should help");
> > > >
> > > > +static int broadcast_ppc;
> > > > +module_param(broadcast_ppc, int, 0644);
> > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs");
> > > > +
> > > >  #define PPC_REGISTERED   1
> > > >  #define PPC_IN_USE       2
> > > >
> > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag)
> > > >                 else
> > > >                         acpi_processor_ppc_ost(pr->handle, 0);
> > > >         }
> > > > -       if (ret >= 0)
> > > > -               cpufreq_update_policy(pr->id);
> > > > +       if (ret >= 0) {
> > > > +               if (broadcast_ppc) {
> > > > +                       int cpu;
> > > > +
> > > > +                       for_each_possible_cpu(cpu)
> > > > +                               cpufreq_update_policy(cpu);
> > > > +               } else {
> > > > +                       cpufreq_update_policy(pr->id);
> > > > +               }
> > > > +       }
> > > >  }
> > > >
> > > >  int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index e35a886e00bc..95e08816b512 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> > > >
> > > >         policy->min = new_policy->min;
> > > >         policy->max = new_policy->max;
> > > > +       policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq;
> > > > +       policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq;
> > > >         trace_cpu_frequency_limits(policy);
> > > >
> > > >         policy->cached_target_freq = UINT_MAX;
> > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > > index dd66decf2087..e1881313c396 100644
> > > > --- a/drivers/cpufreq/intel_pstate.c
> > > > +++ b/drivers/cpufreq/intel_pstate.c
> > > > @@ -2081,11 +2081,24 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
> > > >
> > > >  static int intel_pstate_verify_policy(struct cpufreq_policy *policy)
> > > >  {
> > > > +       int max_freq;
> > > >         struct cpudata *cpu = all_cpu_data[policy->cpu];
> > > >
> > > >         update_turbo_state();
> > >
> > > Well, update_turbo_state() should handle the case at hand already.
> > >
> > > That's what it's for actually: It checks if
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets
> > > global.turbo_disabled is that's the case.
> > >
> > > Why isn't that sufficient?
> > >
> > update_turbo_state() changes the flag of global.turbo_diabled but we
> > need to also leverage it to adjust the policy.max accordingly. This is why
> > we add intel_pstate_get_max_freq() to get the updated max freq in
> > intel_pstate_verify_policy().
> 
> Yes, that's why intel_pstate_verify_policy() passes the return value
> of intel_pstate_get_max_freq() as the second arg
> cpufreq_verify_within_limits(), so really my question was about why
> cpuinfo.max_freq needed to be updated (below).
>
Ok.
> > > > +       max_freq = intel_pstate_get_max_freq(cpu);
> > > > +
> > > > +       if (acpi_ppc && policy->max == policy->cpuinfo.max_freq &&
> > > > +           max_freq != policy->cpuinfo.max_freq) {
> > > > +               /*
> > > > +                * System was not running under any constraints, but the
> > > > +                * current max possible frequency is changed. So reset
> > > > +                * policy limits.
> > > > +                */
> > > > +               policy->cpuinfo.max_freq = policy->max = max_freq;
> > > > +       }
> > >
> > > Why does policy->cpuinfo.max_freq need to be updated?
> > >
> > This is my understanding:
> > There's a corner case that, if the system boots with battery,
> > the max cpu frequency will not scale up if we plug the AC later.
> 
> I see.  The *initial* cpuinfo.max_freq may be too low.  This part is
> missing from your patch changelog.
> 
> The driver is not expected to update cpuinfo.max_freq after init.
> That may not actually break anything, even though it is racy in
> principle, but if it is done, it needs to be done in the "passive"
> mode too and that may be more problematic.
> 
Do you mean updating it for "passive" mode might not be suitable?
> Anyway, this is more fundamental than you seem to be thinking.
> 
> > According to the log provided by Gabriele Mazzotta,  if the system
> > boot up with battery, then plug the AC after boot up, the max perf ratio
> > and policy->cpuinfo.max will remain 17 rather than increasing to
> > 30(when AC plugged thus turbo enabled):
> >
> > [   52.158810] CPU 0: _PPC is 6 - frequency  limited
> > [   52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000
> > [   52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17
> > [   52.158827] intel_pstate: cpu:0 global_min:8 global_max:30
> > [   52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8
> >
> > This is caused by:
> > In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated
> > which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes,
> > we might need to also update the policy->cpuinfo.max to tell user that the hardware
> > status has changed.
> >
> > So we give it a chance to adjust the policy.cpuinfo.max and policy.max in
> > cpufreq_driver->verify()  according to turbo status, this is what this patch mainly
> > aims to do.
> >
> > Besides, since on this platform there's only one _PPC notification for one CPU, it is
> > necessary to broadcast the notification to all CPUs on this package. And this patch
> > broadcast it to all online CPUs to make the change simpler.
> 
> You're trying to make two substantial changes in one go, broadcasting
> _PPC and updating cpuinfo.max_freq.  Don't do that, they need to be
> separate changes.
Ok, I'll send version 2 out which is composed of two modifications to
address different problems.
> 
> Moreover, we may want to address the initial cpuinfo.max_freq issue in
> a different way.
Adjust the cpuinfo.max_freq in .verify() seems to be a proper place since
this callback is invoked at a very early stage for both active and passive
mode, and I did not see race condition for them. (Could not quite catch
what we talked last time on this, if you have proposed another suggestion
on how to update the cpuinfo.max_freq) :)


Thanks,
Ryan(Yu)

> 
> Thanks,
> Rafael