[PATCH 0/2] Disable the MSR T-state if it is enabled after resumed

[PATCH 0/2] Disable the MSR T-state if it is enabled after resumed

[Chen Yu]

Disable the MSR T-state if it is enabled after resumed back, because
generally we don't expect the throttling to be enabled after resumed
and should rely on other OS components to adjust the throttling state.

Chen Yu (2):
  ACPI throttling: Disable the MSR T-state if enabled after resumed
  x86/pm: Delete the quirk to save/restore extra MSR around
    suspend/resume

 arch/x86/power/cpu.c                | 38 +++++--------------------------------
 drivers/acpi/processor_throttling.c | 33 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH 1/2][RFC v3] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

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").

There are three problems here:
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 nonboot 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.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=90041
Reported-by: Kadir <kadir@colakoglu.nl>
Suggested-by: Len Brown <lenb@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/acpi/processor_throttling.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
index a12f96c..3fb759d 100644
--- a/drivers/acpi/processor_throttling.c
+++ b/drivers/acpi/processor_throttling.c
@@ -64,6 +64,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 +387,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 +768,23 @@ 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,6 +799,12 @@ static int acpi_throttling_wrmsr(u64 value)
 		"HARDWARE addr space,NOT supported yet\n");
 	return -1;
 }
+
+static void throttling_msr_reevaluate(int cpu)
+{
+	return -1;
+}
+
 #endif
 
 static int acpi_read_throttling_status(struct acpi_processor *pr,
-- 
2.7.4

[PATCH 2/2][RFC v3] x86/pm: Delete the quirk to save/restore extra MSR around suspend/resume

[Chen Yu]

As we have already added the logic to disable the MSR T-state
if it is enabled after resumed, the quirk here seems not valuable
anymore, thus delete it and also keep the save/restore MSR framework
for future use.

Cc: Len Brown <lenb@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 arch/x86/power/cpu.c | 38 +++++---------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 66ade16..db75b91 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -378,7 +378,7 @@ static int __init bsp_pm_check_init(void)
 
 core_initcall(bsp_pm_check_init);
 
-static int msr_init_context(const u32 *msr_id, const int total_num)
+static int  __maybe_unused msr_init_context(const u32 *msr_id, const int total_num)
 {
 	int i = 0;
 	struct saved_msr *msr_array;
@@ -405,40 +405,12 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
 	return 0;
 }
 
-/*
- * The following section is a quirk framework for problematic BIOSen:
- * Sometimes MSRs are modified by the BIOSen after suspended to
- * RAM, this might cause unexpected behavior after wakeup.
- * Thus we save/restore these specified MSRs across suspend/resume
- * in order to work around it.
- *
- * For any further problematic BIOSen/platforms,
- * please add your own function similar to msr_initialize_bdw.
- */
-static int msr_initialize_bdw(const struct dmi_system_id *d)
-{
-	/* Add any extra MSR ids into this array. */
-	u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
-
-	pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
-	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
-}
-
-static struct dmi_system_id msr_save_dmi_table[] = {
-	{
-	 .callback = msr_initialize_bdw,
-	 .ident = "BROADWELL BDX_EP",
-	 .matches = {
-		DMI_MATCH(DMI_PRODUCT_NAME, "GRANTLEY"),
-		DMI_MATCH(DMI_PRODUCT_VERSION, "E63448-400"),
-		},
-	},
-	{}
-};
-
 static int pm_check_save_msr(void)
 {
-	dmi_check_system(msr_save_dmi_table);
+	/* Save/restore MSRs in the future, for example:
+	 * u32 msr_id[] = { MSR_IA32_THERM_CONTROL };
+	 * msr_init_context(msr_id, ARRAY_SIZE(msr_id));
+	 */
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 1/2][RFC v3] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Yu Chen]

On Wed, Feb 08, 2017 at 12:23:16AM +0800, 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").
> 
> There are three problems here:
> 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 nonboot 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.
>
[cut] 
> +
> +static void throttling_msr_reevaluate(int cpu)
> +{
> +	return -1;
Oops, I made a mistake, should not return any value here.
But please help check if this draft is in the right direction? thanks very much.

Yu

Re: [PATCH 1/2][RFC v3] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

On Wed, Feb 08, 2017 at 12:23:16AM +0800, 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").
> 
> There are three problems here:
> 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 nonboot 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.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=90041
> Reported-by: Kadir <kadir@colakoglu.nl>
> Suggested-by: Len Brown <lenb@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
Please ignore this patch for now, need to do more test on this.

Thanks,
Yu