| Message ID | 20211115184339.68847-2-philip.cox@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: intel_pstate: Clear HWP desired on suspend/shutdown and offline | expand |
On 15.11.21 19:43, Philip Cox wrote: > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/1950584 > > Commit a365ab6b9dfb ("cpufreq: intel_pstate: Implement the > ->adjust_perf() callback") caused intel_pstate to use nonzero HWP > desired values in certain usage scenarios, but it did not prevent > them from being leaked into the confugirations in which HWP desired > is expected to be 0. > > The failing scenarios are switching the driver from the passive > mode to the active mode and starting a new kernel via kexec() while > intel_pstate is running in the passive mode. > > To address this issue, ensure that HWP desired will be cleared on > offline and suspend/shutdown. > > Fixes: a365ab6b9dfb ("cpufreq: intel_pstate: Implement the ->adjust_perf() callback") > Reported-by: Julia Lawall <julia.lawall@inria.fr> > Tested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (backportedf from commit dbea75fe18f60e364de6d994fc938a24ba249d81) s/backportedf/backported/ This can be fixed when applying the patch. > Signed-off-by: Philip Cox <philip.cox@canonical.com> > --- > drivers/cpufreq/intel_pstate.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index d483383dcfb9..d2089adcebc3 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -910,9 +910,16 @@ static void intel_pstate_hwp_offline(struct cpudata *cpu) > */ > value &= ~GENMASK_ULL(31, 24); > value |= HWP_ENERGY_PERF_PREFERENCE(cpu->epp_cached); > - WRITE_ONCE(cpu->hwp_req_cached, value); > } > > + /* > + * Clear the desired perf field in the cached HWP request value to > + * prevent nonzero desired values from being leaked into the active > + * mode. > + */ > + value &= ~HWP_DESIRED_PERF(~0L); > + WRITE_ONCE(cpu->hwp_req_cached, value); > + > value &= ~GENMASK_ULL(31, 0); > min_perf = HWP_LOWEST_PERF(cpu->hwp_cap_cached); > > @@ -2766,6 +2773,27 @@ static int intel_cpufreq_cpu_exit(struct cpufreq_policy *policy) > return intel_pstate_cpu_exit(policy); > } > > +static int intel_cpufreq_suspend(struct cpufreq_policy *policy) > +{ > + intel_pstate_suspend(policy); > + > + if (hwp_active) { > + struct cpudata *cpu = all_cpu_data[policy->cpu]; > + u64 value = READ_ONCE(cpu->hwp_req_cached); > + > + /* > + * Clear the desired perf field in MSR_HWP_REQUEST in case > + * intel_cpufreq_adjust_perf() is in use and the last value > + * written by it may not be suitable. > + */ > + value &= ~HWP_DESIRED_PERF(~0L); > + wrmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value); > + WRITE_ONCE(cpu->hwp_req_cached, value); > + } > + > + return 0; > +} > + > static struct cpufreq_driver intel_cpufreq = { > .flags = CPUFREQ_CONST_LOOPS, > .verify = intel_cpufreq_verify_policy, > @@ -2775,7 +2803,7 @@ static struct cpufreq_driver intel_cpufreq = { > .exit = intel_cpufreq_cpu_exit, > .offline = intel_pstate_cpu_offline, > .online = intel_pstate_cpu_online, > - .suspend = intel_pstate_suspend, > + .suspend = intel_cpufreq_suspend, > .resume = intel_pstate_resume, > .update_limits = intel_pstate_update_limits, > .name = "intel_cpufreq", >
On 16.11.21 12:08, Kleber Souza wrote: > On 15.11.21 19:43, Philip Cox wrote: >> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1950584 >> >> Commit a365ab6b9dfb ("cpufreq: intel_pstate: Implement the >> ->adjust_perf() callback") caused intel_pstate to use nonzero HWP >> desired values in certain usage scenarios, but it did not prevent >> them from being leaked into the confugirations in which HWP desired >> is expected to be 0. >> >> The failing scenarios are switching the driver from the passive >> mode to the active mode and starting a new kernel via kexec() while >> intel_pstate is running in the passive mode. >> >> To address this issue, ensure that HWP desired will be cleared on >> offline and suspend/shutdown. >> >> Fixes: a365ab6b9dfb ("cpufreq: intel_pstate: Implement the ->adjust_perf() >> callback") >> Reported-by: Julia Lawall <julia.lawall@inria.fr> >> Tested-by: Julia Lawall <julia.lawall@inria.fr> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> (backportedf from commit dbea75fe18f60e364de6d994fc938a24ba249d81) > > s/backportedf/backported/ > Apart from that I would have some additional comments. First with the same patch going into Hirsute and Impish, I would have named the subjects there "PATCH 1/1". Also, I am not sure why there are 2 patches. If the same backport would apply to both that could be sent as one patch. For clarity it always helps to have a small section below the backported line which explains things. We usually do that in a form like [smb: context adjustments in hunk #X] or the like. And things like that could be in the cover email. That can copy the SRU justification from the bug report but does not have to be the same. More important is to have the template filled into the bug report. Adjusting the description tends to be best since that is the first thing one reads. This all can be clarified without submitting again, but it would be good to have a follow-up explaining things. -Stefan > This can be fixed when applying the patch. > >> Signed-off-by: Philip Cox <philip.cox@canonical.com> >> --- >> drivers/cpufreq/intel_pstate.c | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c >> index d483383dcfb9..d2089adcebc3 100644 >> --- a/drivers/cpufreq/intel_pstate.c >> +++ b/drivers/cpufreq/intel_pstate.c >> @@ -910,9 +910,16 @@ static void intel_pstate_hwp_offline(struct cpudata *cpu) >> */ >> value &= ~GENMASK_ULL(31, 24); >> value |= HWP_ENERGY_PERF_PREFERENCE(cpu->epp_cached); >> - WRITE_ONCE(cpu->hwp_req_cached, value); >> } >> + /* >> + * Clear the desired perf field in the cached HWP request value to >> + * prevent nonzero desired values from being leaked into the active >> + * mode. >> + */ >> + value &= ~HWP_DESIRED_PERF(~0L); >> + WRITE_ONCE(cpu->hwp_req_cached, value); >> + >> value &= ~GENMASK_ULL(31, 0); >> min_perf = HWP_LOWEST_PERF(cpu->hwp_cap_cached); >> @@ -2766,6 +2773,27 @@ static int intel_cpufreq_cpu_exit(struct cpufreq_policy >> *policy) >> return intel_pstate_cpu_exit(policy); >> } >> +static int intel_cpufreq_suspend(struct cpufreq_policy *policy) >> +{ >> + intel_pstate_suspend(policy); >> + >> + if (hwp_active) { >> + struct cpudata *cpu = all_cpu_data[policy->cpu]; >> + u64 value = READ_ONCE(cpu->hwp_req_cached); >> + >> + /* >> + * Clear the desired perf field in MSR_HWP_REQUEST in case >> + * intel_cpufreq_adjust_perf() is in use and the last value >> + * written by it may not be suitable. >> + */ >> + value &= ~HWP_DESIRED_PERF(~0L); >> + wrmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value); >> + WRITE_ONCE(cpu->hwp_req_cached, value); >> + } >> + >> + return 0; >> +} >> + >> static struct cpufreq_driver intel_cpufreq = { >> .flags = CPUFREQ_CONST_LOOPS, >> .verify = intel_cpufreq_verify_policy, >> @@ -2775,7 +2803,7 @@ static struct cpufreq_driver intel_cpufreq = { >> .exit = intel_cpufreq_cpu_exit, >> .offline = intel_pstate_cpu_offline, >> .online = intel_pstate_cpu_online, >> - .suspend = intel_pstate_suspend, >> + .suspend = intel_cpufreq_suspend, >> .resume = intel_pstate_resume, >> .update_limits = intel_pstate_update_limits, >> .name = "intel_cpufreq", >> > >
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index d483383dcfb9..d2089adcebc3 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -910,9 +910,16 @@ static void intel_pstate_hwp_offline(struct cpudata *cpu) */ value &= ~GENMASK_ULL(31, 24); value |= HWP_ENERGY_PERF_PREFERENCE(cpu->epp_cached); - WRITE_ONCE(cpu->hwp_req_cached, value); } + /* + * Clear the desired perf field in the cached HWP request value to + * prevent nonzero desired values from being leaked into the active + * mode. + */ + value &= ~HWP_DESIRED_PERF(~0L); + WRITE_ONCE(cpu->hwp_req_cached, value); + value &= ~GENMASK_ULL(31, 0); min_perf = HWP_LOWEST_PERF(cpu->hwp_cap_cached); @@ -2766,6 +2773,27 @@ static int intel_cpufreq_cpu_exit(struct cpufreq_policy *policy) return intel_pstate_cpu_exit(policy); } +static int intel_cpufreq_suspend(struct cpufreq_policy *policy) +{ + intel_pstate_suspend(policy); + + if (hwp_active) { + struct cpudata *cpu = all_cpu_data[policy->cpu]; + u64 value = READ_ONCE(cpu->hwp_req_cached); + + /* + * Clear the desired perf field in MSR_HWP_REQUEST in case + * intel_cpufreq_adjust_perf() is in use and the last value + * written by it may not be suitable. + */ + value &= ~HWP_DESIRED_PERF(~0L); + wrmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value); + WRITE_ONCE(cpu->hwp_req_cached, value); + } + + return 0; +} + static struct cpufreq_driver intel_cpufreq = { .flags = CPUFREQ_CONST_LOOPS, .verify = intel_cpufreq_verify_policy, @@ -2775,7 +2803,7 @@ static struct cpufreq_driver intel_cpufreq = { .exit = intel_cpufreq_cpu_exit, .offline = intel_pstate_cpu_offline, .online = intel_pstate_cpu_online, - .suspend = intel_pstate_suspend, + .suspend = intel_cpufreq_suspend, .resume = intel_pstate_resume, .update_limits = intel_pstate_update_limits, .name = "intel_cpufreq",