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@xxxxxxxxxxxx>
> Suggested-by: Len Brown <lenb@xxxxxxxxxx>
> Cc: Len Brown <lenb@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Cc: Pavel Machek <pavel@xxxxxx>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
