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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> > > > > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> > > > > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx> > > > > --- > > > > 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
