Hi,
On Mon, Mar 13, 2017 at 11:41:03PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 17, 2017 04:27:30 PM Chen Yu wrote:
> > Previously a bug was reported that on certain Broadwell
> > platform, after resumed from S3, the CPU is running at
> > an anomalously low speed, due to the BIOS has enabled the
> > MSR throttling across S3. The solution to this was to introduce
> > a quirk framework to save/restore tstate MSR register around
> > suspend/resume, in Commit 7a9c2dd08ead ("x86/pm:
> > Introduce quirk framework to save/restore extra MSR
> > registers around suspend/resume").
> >
> > However there are still three problems left:
> > 1. More and more reports show that other platforms also
> > encountered the same issue, so the quirk list might
> > be endless.
> > 2. Each CPUs should take the save/restore operation into
> > consideration, rather than the boot CPU alone.
> > 3. Normally ACPI T-state re-evaluation is done on resume,
> > however there is no _TSS on the bogus platform, thus
> > above re-evaluation code does not run on that machine.
> >
> > Solution:
> > This patch is based on the fact that, we generally should not
> > expect the system to come back from resume with throttling
> > enabled, but leverage the OS components to deal with it,
> > such as thermal event. So we simply clear the MSR T-state
> > and print the warning if it is found to be enabled after
> > resumed back. Besides, we can remove the quirk in previous patch
> > later.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=90041
> > Reported-and-tested-by: Kadir <ka...@colakoglu.nl>
> > Suggested-by: Len Brown <l...@kernel.org>
> > Cc: Len Brown <l...@kernel.org>
> > Cc: "Rafael J. Wysocki" <raf...@kernel.org>
> > Cc: Pavel Machek <pa...@ucw.cz>
> > Cc: Zhang Rui <rui.zh...@intel.com>
> > Cc: Ingo Molnar <mi...@kernel.org>
> > Cc: linux...@vger.kernel.org
> > Signed-off-by: Chen Yu <yu.c.c...@intel.com>
> > ---
> > drivers/acpi/processor_throttling.c | 58
> > +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> >
> > diff --git a/drivers/acpi/processor_throttling.c
> > b/drivers/acpi/processor_throttling.c
> > index a12f96c..e121449 100644
> > --- a/drivers/acpi/processor_throttling.c
> > +++ b/drivers/acpi/processor_throttling.c
> > @@ -29,6 +29,7 @@
> > #include <linux/sched.h>
> > #include <linux/cpufreq.h>
> > #include <linux/acpi.h>
> > +#include <linux/syscore_ops.h>
> > #include <acpi/processor.h>
> > #include <asm/io.h>
> > #include <linux/uaccess.h>
> > @@ -64,6 +65,7 @@ struct acpi_processor_throttling_arg {
> > static int acpi_processor_get_throttling(struct acpi_processor *pr);
> > int acpi_processor_set_throttling(struct acpi_processor *pr,
> > int state, bool force);
> > +static void throttling_msr_reevaluate(int cpu);
> >
> > static int acpi_processor_update_tsd_coord(void)
> > {
> > @@ -386,6 +388,15 @@ void acpi_processor_reevaluate_tstate(struct
> > acpi_processor *pr,
> > pr->flags.throttling = 0;
> > return;
> > }
> > + /*
> > + * It was found after resumed from suspend to ram, some BIOSes would
> > + * adjust the MSR tstate, however on these platforms no _PSS is provided
> > + * thus we never have a chance to adjust the MSR T-state anymore.
> > + * Thus force clearing it if MSR T-state is enabled, because generally
> > + * we never expect to come back from resume with throttling enabled.
> > + * Later let other components to adjust T-state if necessary.
> > + */
> > + throttling_msr_reevaluate(pr->id);
> > /* the following is to recheck whether the T-state is valid for
> > * the online CPU
> > */
> > @@ -758,6 +769,24 @@ static int acpi_throttling_wrmsr(u64 value)
> > }
> > return ret;
> > }
> > +
> > +static long msr_reevaluate_fn(void *data)
> > +{
> > + u64 msr = 0;
> > +
> > + acpi_throttling_rdmsr(&msr);
> > + if (msr) {
> > + printk_once(KERN_ERR "PM: The BIOS might have modified the MSR
> > T-state, clear it for now.\n");
> > + acpi_throttling_wrmsr(0);
> > + }
> > + return 0;
> > +}
> > +
> > +static void throttling_msr_reevaluate(int cpu)
> > +{
> > + work_on_cpu(cpu, msr_reevaluate_fn, NULL);
> > +}
> > +
> > #else
> > static int acpi_throttling_rdmsr(u64 *value)
> > {
> > @@ -772,8 +801,37 @@ static int acpi_throttling_wrmsr(u64 value)
> > "HARDWARE addr space,NOT supported yet\n");
> > return -1;
> > }
> > +
> > +static long msr_reevaluate_fn(void *data)
> > +{
> > + return 0;
> > +}
> > +
> > +static void throttling_msr_reevaluate(int cpu)
> > +{
> > +}
> > #endif
> >
> > +void acpi_throttling_resume(void)
> > +{
> > + msr_reevaluate_fn(NULL);
> > +}
> > +
> > +static struct syscore_ops acpi_throttling_syscore_ops = {
> > + .resume = acpi_throttling_resume,
> > +};
>
> This should go under the #ifdef too.
>
OK, will change it.
> > +
> > +static int acpi_throttling_init_ops(void)
> > +{
> > + /*
> > + * Reevaluate on boot CPU. Since it is not always CPU0,
> > + * we can not invoke throttling_msr_reevaluate(0) directly.
> > + */
> > + register_syscore_ops(&acpi_throttling_syscore_ops);
> > + return 0;
> > +}
> > +device_initcall(acpi_throttling_init_ops);
>
> Isn't there a good place to call register_syscore_ops() for this aleady?
>
> I'd rather not add a new device_initcall() for that.
>
OK, will put it into acpi_processor_throttling_init.
> > +
> > static int acpi_read_throttling_status(struct acpi_processor *pr,
> > u64 *value)
> > {
> >
>
> Thanks,
> Rafael
>
Thanks,
Yu
