| Message ID | 20241011003640.1613812-2-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [V2,1/2] platform/x86/intel/pmc: Refactor platform resume functions to use cnl_resume() | expand |
On Thu, 2024-10-10 at 17:36 -0700, David E. Box wrote: > On some platforms, aggressive C1 auto-demotion may lead to failure to > enter > the deepest C-state during suspend-to-idle, causing high power > consumption. > To prevent this, disable C1 auto-demotion during suspend and re- > enable on > resume. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > > V2 - Remove #define DEBUG > - Move refactor of cnl_resume() to separate patch > - Use smp_call_function() to disable and restore C1_AUTO_DEMOTE > - Add comment that the MSR is per core, not per package. > - Add comment that the online cpu mask remains unchanged during > suspend due to frozen userspace. > > drivers/platform/x86/intel/pmc/cnp.c | 53 > ++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c > b/drivers/platform/x86/intel/pmc/cnp.c > index 513c02670c5a..f12d4f0f9e93 100644 > --- a/drivers/platform/x86/intel/pmc/cnp.c > +++ b/drivers/platform/x86/intel/pmc/cnp.c > @@ -8,6 +8,8 @@ > * > */ > > +#include <linux/smp.h> > +#include <linux/suspend.h> > #include "core.h" > > /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ > @@ -206,8 +208,52 @@ const struct pmc_reg_map cnp_reg_map = { > .etr3_offset = ETR3_OFFSET, > }; > > + > +/* > + * Disable C1 auto-demotion > + * > + * Aggressive C1 auto-demotion may lead to failure to enter the > deepest C-state > + * during suspend-to-idle, causing high power consumption. To > prevent this, we > + * disable C1 auto-demotion during suspend and re-enable on resume. > + * > + * Note that, although MSR_PKG_CST_CONFIG_CONTROL has 'package' in > its name, it > + * is actually a per-core MSR on client platforms, affecting only a > single CPU. > + * Therefore, it must be configured on all online CPUs. The online > cpu mask is > + * unchanged during the phase of suspend/resume as user space is > frozen. > + */ > + > +static DEFINE_PER_CPU(u64, pkg_cst_config); > + > +static void disable_c1_auto_demote(void *unused) > +{ > + int cpunum = smp_processor_id(); > + u64 val; > + > + rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > + per_cpu(pkg_cst_config, cpunum) = val; > + val &= ~NHM_C1_AUTO_DEMOTE; > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val); Do you want to leave pr_debug? > +} > + > +static void restore_c1_auto_demote(void *unused) > +{ > + int cpunum = smp_processor_id(); > + > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, > + per_cpu(pkg_cst_config, cpunum)); > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, > cpunum)); > +} > + > void cnl_suspend(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); Why do you need this? Thanks, Srinivas > + disable_c1_auto_demote(NULL); > + smp_call_function(disable_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > + > /* > * Due to a hardware limitation, the GBE LTR blocks PC10 > * when a cable is attached. To unblock PC10 during suspend, > @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) > > int cnl_resume(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); > + restore_c1_auto_demote(NULL); > + smp_call_function(restore_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > + > pmc_core_send_ltr_ignore(pmcdev, 3, 0); > > return pmc_core_resume_common(pmcdev);
On Thu, 2024-10-10 at 19:09 -0700, srinivas pandruvada wrote: > On Thu, 2024-10-10 at 17:36 -0700, David E. Box wrote: > > On some platforms, aggressive C1 auto-demotion may lead to failure to > > enter > > the deepest C-state during suspend-to-idle, causing high power > > consumption. > > To prevent this, disable C1 auto-demotion during suspend and re- > > enable on > > resume. > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > > > V2 - Remove #define DEBUG > > - Move refactor of cnl_resume() to separate patch > > - Use smp_call_function() to disable and restore C1_AUTO_DEMOTE > > - Add comment that the MSR is per core, not per package. > > - Add comment that the online cpu mask remains unchanged during > > suspend due to frozen userspace. > > > > drivers/platform/x86/intel/pmc/cnp.c | 53 > > ++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c > > b/drivers/platform/x86/intel/pmc/cnp.c > > index 513c02670c5a..f12d4f0f9e93 100644 > > --- a/drivers/platform/x86/intel/pmc/cnp.c > > +++ b/drivers/platform/x86/intel/pmc/cnp.c > > @@ -8,6 +8,8 @@ > > * > > */ > > > > +#include <linux/smp.h> > > +#include <linux/suspend.h> > > #include "core.h" > > > > /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ > > @@ -206,8 +208,52 @@ const struct pmc_reg_map cnp_reg_map = { > > .etr3_offset = ETR3_OFFSET, > > }; > > > > + > > +/* > > + * Disable C1 auto-demotion > > + * > > + * Aggressive C1 auto-demotion may lead to failure to enter the > > deepest C-state > > + * during suspend-to-idle, causing high power consumption. To > > prevent this, we > > + * disable C1 auto-demotion during suspend and re-enable on resume. > > + * > > + * Note that, although MSR_PKG_CST_CONFIG_CONTROL has 'package' in > > its name, it > > + * is actually a per-core MSR on client platforms, affecting only a > > single CPU. > > + * Therefore, it must be configured on all online CPUs. The online > > cpu mask is > > + * unchanged during the phase of suspend/resume as user space is > > frozen. > > + */ > > + > > +static DEFINE_PER_CPU(u64, pkg_cst_config); > > + > > +static void disable_c1_auto_demote(void *unused) > > +{ > > + int cpunum = smp_processor_id(); > > + u64 val; > > + > > + rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > > + per_cpu(pkg_cst_config, cpunum) = val; > > + val &= ~NHM_C1_AUTO_DEMOTE; > > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val); > Do you want to leave pr_debug? Thought it could be useful but it can be removed. > > > +} > > + > > +static void restore_c1_auto_demote(void *unused) > > +{ > > + int cpunum = smp_processor_id(); > > + > > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, > > + per_cpu(pkg_cst_config, cpunum)); > > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, > > cpunum)); > > +} > > + > > void cnl_suspend(struct pmc_dev *pmcdev) > > { > > + if (!pm_suspend_via_firmware()) { > > + preempt_disable(); > Why do you need this? To ensure that the cpu doesn't change between the next two calls. David > > > Thanks, > Srinivas > > > + disable_c1_auto_demote(NULL); > > + smp_call_function(disable_c1_auto_demote, NULL, 0); > > + preempt_enable(); > > + } > > + > > /* > > * Due to a hardware limitation, the GBE LTR blocks PC10 > > * when a cable is attached. To unblock PC10 during suspend, > > @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) > > > > int cnl_resume(struct pmc_dev *pmcdev) > > { > > + if (!pm_suspend_via_firmware()) { > > + preempt_disable(); > > + restore_c1_auto_demote(NULL); > > + smp_call_function(restore_c1_auto_demote, NULL, 0); > > + preempt_enable(); > > + } > > + > > pmc_core_send_ltr_ignore(pmcdev, 3, 0); > > > > return pmc_core_resume_common(pmcdev); >
On Fri, Oct 11, 2024 at 2:36 AM David E. Box <david.e.box@linux.intel.com> wrote: > > On some platforms, aggressive C1 auto-demotion may lead to failure to enter > the deepest C-state during suspend-to-idle, causing high power consumption. > To prevent this, disable C1 auto-demotion during suspend and re-enable on > resume. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > > V2 - Remove #define DEBUG > - Move refactor of cnl_resume() to separate patch > - Use smp_call_function() to disable and restore C1_AUTO_DEMOTE > - Add comment that the MSR is per core, not per package. > - Add comment that the online cpu mask remains unchanged during > suspend due to frozen userspace. > > drivers/platform/x86/intel/pmc/cnp.c | 53 ++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c > index 513c02670c5a..f12d4f0f9e93 100644 > --- a/drivers/platform/x86/intel/pmc/cnp.c > +++ b/drivers/platform/x86/intel/pmc/cnp.c > @@ -8,6 +8,8 @@ > * > */ > > +#include <linux/smp.h> > +#include <linux/suspend.h> > #include "core.h" > > /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ > @@ -206,8 +208,52 @@ const struct pmc_reg_map cnp_reg_map = { > .etr3_offset = ETR3_OFFSET, > }; > > + > +/* > + * Disable C1 auto-demotion > + * > + * Aggressive C1 auto-demotion may lead to failure to enter the deepest C-state > + * during suspend-to-idle, causing high power consumption. To prevent this, we > + * disable C1 auto-demotion during suspend and re-enable on resume. > + * > + * Note that, although MSR_PKG_CST_CONFIG_CONTROL has 'package' in its name, it > + * is actually a per-core MSR on client platforms, affecting only a single CPU. > + * Therefore, it must be configured on all online CPUs. The online cpu mask is > + * unchanged during the phase of suspend/resume as user space is frozen. > + */ > + > +static DEFINE_PER_CPU(u64, pkg_cst_config); > + > +static void disable_c1_auto_demote(void *unused) > +{ > + int cpunum = smp_processor_id(); > + u64 val; > + > + rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > + per_cpu(pkg_cst_config, cpunum) = val; > + val &= ~NHM_C1_AUTO_DEMOTE; > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val); > +} If you decide to retain the debug statements, I generally prefer to separate them from the rest of the code with empty lines, for clarity. > + > +static void restore_c1_auto_demote(void *unused) > +{ > + int cpunum = smp_processor_id(); > + > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, > + per_cpu(pkg_cst_config, cpunum)); > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, cpunum)); > +} > + I would add something like static void s2idle_cpu_quirk(smp_call_func_t func) { if (pm_suspend_via_firmware()) return; preempt_disable(); func(); smp_call_function(func, NULL, 0); preempt_enable(); } > void cnl_suspend(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); > + disable_c1_auto_demote(NULL); > + smp_call_function(disable_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > + And here I'd just do s2idle_cpu_quirk(disable_c1_auto_demote); and analogously below. > /* > * Due to a hardware limitation, the GBE LTR blocks PC10 > * when a cable is attached. To unblock PC10 during suspend, > @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) > > int cnl_resume(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); > + restore_c1_auto_demote(NULL); > + smp_call_function(restore_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > + > pmc_core_send_ltr_ignore(pmcdev, 3, 0); > > return pmc_core_resume_common(pmcdev); > --
On Thu, 2024-10-10 at 20:50 -0700, David E. Box wrote: > On Thu, 2024-10-10 at 19:09 -0700, srinivas pandruvada wrote: > > On Thu, 2024-10-10 at 17:36 -0700, David E. Box wrote: > > > On some platforms, aggressive C1 auto-demotion may lead to > > > failure to > > > enter > > > the deepest C-state during suspend-to-idle, causing high power > > > consumption. > > > To prevent this, disable C1 auto-demotion during suspend and re- > > > enable on > > > resume. > > > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > > --- > > > > > > V2 - Remove #define DEBUG > > > - Move refactor of cnl_resume() to separate patch > > > - Use smp_call_function() to disable and restore > > > C1_AUTO_DEMOTE > > > - Add comment that the MSR is per core, not per package. > > > - Add comment that the online cpu mask remains unchanged > > > during > > > suspend due to frozen userspace. > > > > > > drivers/platform/x86/intel/pmc/cnp.c | 53 > > > ++++++++++++++++++++++++++++ > > > 1 file changed, 53 insertions(+) > > > > > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c > > > b/drivers/platform/x86/intel/pmc/cnp.c > > > index 513c02670c5a..f12d4f0f9e93 100644 > > > --- a/drivers/platform/x86/intel/pmc/cnp.c > > > +++ b/drivers/platform/x86/intel/pmc/cnp.c > > > @@ -8,6 +8,8 @@ > > > * > > > */ > > > > > > +#include <linux/smp.h> > > > +#include <linux/suspend.h> > > > #include "core.h" > > > > > > /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ > > > @@ -206,8 +208,52 @@ const struct pmc_reg_map cnp_reg_map = { > > > .etr3_offset = ETR3_OFFSET, > > > }; > > > > > > + > > > +/* > > > + * Disable C1 auto-demotion > > > + * > > > + * Aggressive C1 auto-demotion may lead to failure to enter the > > > deepest C-state > > > + * during suspend-to-idle, causing high power consumption. To > > > prevent this, we > > > + * disable C1 auto-demotion during suspend and re-enable on > > > resume. > > > + * > > > + * Note that, although MSR_PKG_CST_CONFIG_CONTROL has 'package' > > > in > > > its name, it > > > + * is actually a per-core MSR on client platforms, affecting > > > only a > > > single CPU. > > > + * Therefore, it must be configured on all online CPUs. The > > > online > > > cpu mask is > > > + * unchanged during the phase of suspend/resume as user space is > > > frozen. > > > + */ > > > + > > > +static DEFINE_PER_CPU(u64, pkg_cst_config); > > > + > > > +static void disable_c1_auto_demote(void *unused) > > > +{ > > > + int cpunum = smp_processor_id(); > > > + u64 val; > > > + > > > + rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > > > + per_cpu(pkg_cst_config, cpunum) = val; > > > + val &= ~NHM_C1_AUTO_DEMOTE; > > > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); > > > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, > > > val); > > Do you want to leave pr_debug? > > Thought it could be useful but it can be removed. > > > > > > +} > > > + > > > +static void restore_c1_auto_demote(void *unused) > > > +{ > > > + int cpunum = smp_processor_id(); > > > + > > > + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, > > > + per_cpu(pkg_cst_config, cpunum)); > > > + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, > > > per_cpu(pkg_cst_config, > > > cpunum)); > > > +} > > > + > > > void cnl_suspend(struct pmc_dev *pmcdev) > > > { > > > + if (!pm_suspend_via_firmware()) { > > > + preempt_disable(); > > Why do you need this? > > To ensure that the cpu doesn't change between the next two calls. Correct. You need for smp_processor_id() Generally for this avoiding issue with smp_processor_id(), you can use get_cpu(), which gives current cpu as return value and also disable preemption. Something like this this_cpu = get_cpu(); disable_c1_auto_demote(&this_cpu); smp_call_function_many(cpu_online_mask, disable_c1_auto_demote, NULL, 0); put_cpu(); But fine, this makes your code more complex as you have to pass now a param and use for local_cpu and use smp_procesor_id() for remote call via smp_call.. Thanks, Srinivas > > David > > > > > > > Thanks, > > Srinivas > > > > > + disable_c1_auto_demote(NULL); > > > + smp_call_function(disable_c1_auto_demote, NULL, > > > 0); > > > + preempt_enable(); > > > + } > > > + > > > /* > > > * Due to a hardware limitation, the GBE LTR blocks PC10 > > > * when a cable is attached. To unblock PC10 during > > > suspend, > > > @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) > > > > > > int cnl_resume(struct pmc_dev *pmcdev) > > > { > > > + if (!pm_suspend_via_firmware()) { > > > + preempt_disable(); > > > + restore_c1_auto_demote(NULL); > > > + smp_call_function(restore_c1_auto_demote, NULL, > > > 0); > > > + preempt_enable(); > > > + } > > > + > > > pmc_core_send_ltr_ignore(pmcdev, 3, 0); > > > > > > return pmc_core_resume_common(pmcdev); > > > >
+Ricardo On Thu, 2024-10-10 at 17:36 -0700, David E. Box wrote: > On some platforms, aggressive C1 auto-demotion may lead to failure to > enter > the deepest C-state during suspend-to-idle, causing high power > consumption. > To prevent this, disable C1 auto-demotion during suspend and re- > enable on > resume. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > [...] > void cnl_suspend(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); > + disable_c1_auto_demote(NULL); > + smp_call_function(disable_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > As suggested by Ricardo using the following will work avoiding separate local CPU call. Preemption will be disabled, no need separate call. Also cpu_online_mask can't be changed during these callbacks. if (!pm_suspend_via_firmware()) on_each_cpu(disable_c1_auto_demote, NULL, true); I think you need wait=true. I have seen on some occasions you will miss msr write as we called mwait before before async calls finished. Thanks, Srinivas > + > /* > * Due to a hardware limitation, the GBE LTR blocks PC10 > * when a cable is attached. To unblock PC10 during suspend, > @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) > > int cnl_resume(struct pmc_dev *pmcdev) > { > + if (!pm_suspend_via_firmware()) { > + preempt_disable(); > + restore_c1_auto_demote(NULL); > + smp_call_function(restore_c1_auto_demote, NULL, 0); > + preempt_enable(); > + } > + > pmc_core_send_ltr_ignore(pmcdev, 3, 0); > > return pmc_core_resume_common(pmcdev);
diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c index 513c02670c5a..f12d4f0f9e93 100644 --- a/drivers/platform/x86/intel/pmc/cnp.c +++ b/drivers/platform/x86/intel/pmc/cnp.c @@ -8,6 +8,8 @@ * */ +#include <linux/smp.h> +#include <linux/suspend.h> #include "core.h" /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ @@ -206,8 +208,52 @@ const struct pmc_reg_map cnp_reg_map = { .etr3_offset = ETR3_OFFSET, }; + +/* + * Disable C1 auto-demotion + * + * Aggressive C1 auto-demotion may lead to failure to enter the deepest C-state + * during suspend-to-idle, causing high power consumption. To prevent this, we + * disable C1 auto-demotion during suspend and re-enable on resume. + * + * Note that, although MSR_PKG_CST_CONFIG_CONTROL has 'package' in its name, it + * is actually a per-core MSR on client platforms, affecting only a single CPU. + * Therefore, it must be configured on all online CPUs. The online cpu mask is + * unchanged during the phase of suspend/resume as user space is frozen. + */ + +static DEFINE_PER_CPU(u64, pkg_cst_config); + +static void disable_c1_auto_demote(void *unused) +{ + int cpunum = smp_processor_id(); + u64 val; + + rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); + per_cpu(pkg_cst_config, cpunum) = val; + val &= ~NHM_C1_AUTO_DEMOTE; + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val); + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val); +} + +static void restore_c1_auto_demote(void *unused) +{ + int cpunum = smp_processor_id(); + + pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, + per_cpu(pkg_cst_config, cpunum)); + wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, cpunum)); +} + void cnl_suspend(struct pmc_dev *pmcdev) { + if (!pm_suspend_via_firmware()) { + preempt_disable(); + disable_c1_auto_demote(NULL); + smp_call_function(disable_c1_auto_demote, NULL, 0); + preempt_enable(); + } + /* * Due to a hardware limitation, the GBE LTR blocks PC10 * when a cable is attached. To unblock PC10 during suspend, @@ -218,6 +264,13 @@ void cnl_suspend(struct pmc_dev *pmcdev) int cnl_resume(struct pmc_dev *pmcdev) { + if (!pm_suspend_via_firmware()) { + preempt_disable(); + restore_c1_auto_demote(NULL); + smp_call_function(restore_c1_auto_demote, NULL, 0); + preempt_enable(); + } + pmc_core_send_ltr_ignore(pmcdev, 3, 0); return pmc_core_resume_common(pmcdev);
On some platforms, aggressive C1 auto-demotion may lead to failure to enter the deepest C-state during suspend-to-idle, causing high power consumption. To prevent this, disable C1 auto-demotion during suspend and re-enable on resume. Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- V2 - Remove #define DEBUG - Move refactor of cnl_resume() to separate patch - Use smp_call_function() to disable and restore C1_AUTO_DEMOTE - Add comment that the MSR is per core, not per package. - Add comment that the online cpu mask remains unchanged during suspend due to frozen userspace. drivers/platform/x86/intel/pmc/cnp.c | 53 ++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)