Hi, > -----Original Message----- > From: rjwysocki@xxxxxxxxx [mailto:rjwysocki@xxxxxxxxx] On Behalf Of > Rafael J. Wysocki > Sent: Wednesday, November 23, 2016 7:03 AM > To: Chen, Yu C > Cc: ACPI Devel Maling List; Rui Wang; Linux Kernel Mailing List; Len Brown; > Rafael J. Wysocki; Pavel Machek; Matthew Garrett; Zhang, Rui; Linux PM > Subject: Re: [PATCH][RFC] ACPI throttling: Save/restore tstate for each CPUs > across suspend/resume > > On Mon, Nov 14, 2016 at 6:44 PM, Chen Yu <yu.c.chen@xxxxxxxxx> wrote: > > This is a trial version and any comments are appreciated. > > > > Previously a bug was reported that on certain Broadwell platforms, > > after resuming from S3, the CPU is running at an anomalously low > > speed, due to BIOS has enabled the throttling across S3. The solution > > to this is 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 more and more reports show that other platforms also > > experienced the same issue, because some BIOSes would like to adjust > > the tstate if he thinks the temperature is too high. > > To deal with this situation, the Linux uses a compensation strategy > > that, the thermal management leverages thermal_pm_notify() upon resume > > to check if the Processors inside the thermal zone should be throttled > > or not, thus tstate would be re-evaluated. Unfortunately on these > > bogus platforms, none of the Processors are inside any thermal zones > > due to BIOS's implementation. Thus tstate for Processors never has a > > chance to be brought back to normal. > > > > This patch tries to save/restore tstate on receiving the > > PM_SUSPEND_PREPARE and PM_POST_SUSPEND, to be more specific, the > > tstate is saved after thermal_pm_notify(PM_SUSPEND_PREPARE) > > is called, while it's restored before > > thermal_pm_notify(PM_POST_SUSPEND), > > in this way the thermal zone would adjust the tstate eventually and > > also help adjust the tstate for Processors which do not have thermal > > zone bound. Thus it does not imapct the old semantics. > > > > Another concern is that, each CPU should take care of the save/restore > > operation, thus this patch uses percpu workqueue to achieve this. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=90041 > > Reported-by: Matthew Garrett <mjg59@xxxxxxxxxxxxx> > > Reported-by: Kadir <kadir@xxxxxxxxxxxx> > > Cc: Len Brown <lenb@xxxxxxxxxx> > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx> > > Cc: Pavel Machek <pavel@xxxxxx> > > Cc: Matthew Garrett <mjg59@xxxxxxxxxxxxx> > > Cc: Rui Zhang <rui.zhang@xxxxxxxxx> > > Cc: linux-pm@xxxxxxxxxxxxxxx > > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx> > > --- > > drivers/acpi/processor_throttling.c | 70 > > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 70 insertions(+) > > > > diff --git a/drivers/acpi/processor_throttling.c > > b/drivers/acpi/processor_throttling.c > > index d51ca1c..8ddc7d6 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/suspend.h> > > #include <acpi/processor.h> > > #include <asm/io.h> > > #include <asm/uaccess.h> > > @@ -758,6 +759,75 @@ static int acpi_throttling_wrmsr(u64 value) > > } > > return ret; > > } > > + > > +#ifdef CONFIG_PM_SLEEP > > +static DEFINE_PER_CPU(u64, tstate_msr); > > Call it saved_tstate_msr maybe? OK. > > > + > > +static long tstate_pm_fn(void *data) > > +{ > > + u64 value; > > + bool save = *(bool *)data; > > + > > + if (save) { > > + acpi_throttling_rdmsr(&value); > > + this_cpu_write(tstate_msr, value); > > + } else { > > + value = this_cpu_read(tstate_msr); > > + if (value) > > + acpi_throttling_wrmsr(value); > > + } > > + return 0; > > +} > > I would split the above into two functions, one for saving and one for restoring -> > OK. > > + > > +static void tstate_check(unsigned long mode, bool suspend) { > > + int cpu; > > + bool save; > > + > > + if (suspend && mode == PM_SUSPEND_PREPARE) > > + save = true; > > + else if (!suspend && mode == PM_POST_SUSPEND) > > + save = false; > > + else > > + return; > > + > > + get_online_cpus(); > > + for_each_online_cpu(cpu) > > -> and decide here which one to invoke. OK. > > > + work_on_cpu(cpu, tstate_pm_fn, &save); > > Does work_on_cpu() wait for the work to complete? > Yes, it might increase the suspend/resume time, a 'queue_work_on' might be better? > > + put_online_cpus(); > > +} > > + > > +static int tstate_suspend(struct notifier_block *nb, > > + unsigned long mode, void *_unused) { > > + tstate_check(mode, true); > > + return 0; > > +} > > + > > +static int tstate_resume(struct notifier_block *nb, > > + unsigned long mode, void *_unused) { > > + tstate_check(mode, false); > > + return 0; > > +} > > + > > +static int __init tstate_pm_init(void) { > > + /* > > + * tstate_suspend should save tstate after > > + * thermal zone's update in thermal_pm_notify, > > + * vice versa tstate_resume restore tstate before > > + * thermal_pm_notify, thus the thermal framework > > + * has a chance to re-adjust tstate according to the > > + * temperature trend. > > + */ > > + pm_notifier(tstate_suspend, -1); > > + pm_notifier(tstate_resume, 1); > > I don't think this is going to do what you really want. > > Each of these notifiers is going to be invoked during both suspend and resume, Yes, > so I guess you only need one notifier? Here's my original thought: tstate_suspend needs to be invoked after thermal_pm_notify, which has a priority of '0', so the notifier of tstate_suspend should be lower than '0', thus '-1'. And the same for tstate_resume, it should be invoked before thermal_pm_notify, thus priority is '1' ? > > > + return 0; > > +} > > + > > +core_initcall(tstate_pm_init); > > +#endif > > #else > > static int acpi_throttling_rdmsr(u64 *value) { > > -- > > Thanks, > Rafael Thanks, Yu ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
