[PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[PATCH][RFC v4] 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").

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 <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: 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 | 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,
+};
+
+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);
+
 static int acpi_read_throttling_status(struct acpi_processor *pr,
 					u64 *value)
 {
-- 
2.7.4

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Fri 2017-02-17 16:27:30, 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.

What if the machine _is_ hot? 

> +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.
> +	 */

Boot cpu is not cpu#0? How can that be?

Should we introduce generic framework to "fix" all the cpus? Actually,
should this be done right on cpu hotplug?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

Hi,
On Sat, Feb 18, 2017 at 10:02:07AM +0100, Pavel Machek wrote:
> On Fri 2017-02-17 16:27:30, 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.
> 
> What if the machine _is_ hot? 
> 
Later the linux has a chance to adjust the tstate if the system is too hot,
with the help of thermal framework.
But if the cpu is not inside any thermal zone, then there is no way for the
OS to adjust the tstate after resume, however in this case I think it is up
to the user space to adjust the tstate msr, for example, by using thermald
daemon to bind the cpu to the thermal zone.

> > +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.
> > +	 */
> 
> Boot cpu is not cpu#0? How can that be?
> 
In disable_nonboot_cpus, it first tries to disable cpus other than cpu#0,
but if cpu#0 is found to be offline, it will try to bypass it and use another
cpu as the boot/primary cpu:

if (!cpu_online(primary))
	primary = cpumask_first(cpu_online_mask);

> Should we introduce generic framework to "fix" all the cpus? Actually,
> should this be done right on cpu hotplug?
Do you mean, fix other MSR-inconsistent issues, not only the tstate MSR?
Currently the tstate re-adjusting is invoked in cpuhotplug notifier
after each nonboot cpus are brought up:
acpi_soft_cpu_online -> acpi_processor_reevaluate_tstate -> adjust_tstate_msr

Thanks,
Yu
> 
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Rafael J. Wysocki]

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 <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: 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 | 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.

> +
> +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.

> +
>  static int acpi_read_throttling_status(struct acpi_processor *pr,
>  					u64 *value)
>  {
> 

Thanks,
Rafael

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

Hi!

> > > 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.
> > 
> > What if the machine _is_ hot? 
> > 
> Later the linux has a chance to adjust the tstate if the system is too hot,
> with the help of thermal framework.

Will it adjust the tstate? Normally, we do such stuff when tresholds
are exceeded. If we are already above the threshold, we'll see no
reason to 

> But if the cpu is not inside any thermal zone, then there is no way for the
> OS to adjust the tstate after resume, however in this case I think it is up
> to the user space to adjust the tstate msr, for example, by using thermald
> daemon to bind the cpu to the thermal zone.

Umm. Userland should not access MSRs. And we certainly should not
depend on userland adjusting the MSRs!

> > Should we introduce generic framework to "fix" all the cpus? Actually,
> > should this be done right on cpu hotplug?
> Do you mean, fix other MSR-inconsistent issues, not only the tstate MSR?
> Currently the tstate re-adjusting is invoked in cpuhotplug notifier
> after each nonboot cpus are brought up:
> acpi_soft_cpu_online -> acpi_processor_reevaluate_tstate -> adjust_tstate_msr

Ok.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

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 <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: 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 | 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

Re: [PATCH][RFC v4] ACPI throttling: Disable the MSR T-state if enabled after resumed

[Chen Yu]

Hi,
On Tue, Mar 14, 2017 at 06:38:03PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > 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.
> > > 
> > > What if the machine _is_ hot? 
> > > 
> > Later the linux has a chance to adjust the tstate if the system is too hot,
> > with the help of thermal framework.
> 
> Will it adjust the tstate? Normally, we do such stuff when tresholds
> are exceeded. If we are already above the threshold, we'll see no
> reason to 
> 
According to current implementation of thermal_pm_notify, after resumed
back, the initial temperature will be reset to -274, thus the trend of
temperature will always be 'increasing', then the trip point(threshold)
will be compared with currently temperature, if it is bigger than the
trip point, the CPUs will be throttled. So I think the tstate has a chance
to be adjusted.
> > But if the cpu is not inside any thermal zone, then there is no way for the
> > OS to adjust the tstate after resume, however in this case I think it is up
> > to the user space to adjust the tstate msr, for example, by using thermald
> > daemon to bind the cpu to the thermal zone.
> 
> Umm. Userland should not access MSRs. And we certainly should not
> depend on userland adjusting the MSRs!
> 
I agree, so I think thermald is the only applicable solution for this case,
because the thermald is just trying to bind the CPUs to some thermal
zones via sysfs, and the user does not need to modify the MSRs because
the tstate MSRs will be taken care of by the thermal zones in kernelland.
> > > Should we introduce generic framework to "fix" all the cpus? Actually,
> > > should this be done right on cpu hotplug?
> > Do you mean, fix other MSR-inconsistent issues, not only the tstate MSR?
> > Currently the tstate re-adjusting is invoked in cpuhotplug notifier
> > after each nonboot cpus are brought up:
> > acpi_soft_cpu_online -> acpi_processor_reevaluate_tstate -> adjust_tstate_msr
> 
> Ok.
> 									Pavel
> --
Thanks,
Yu
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html