In preparation to add a suspend notifier, wrap the logic to enable HFI and
program its memory buffer into helper functions. Both the CPU hotplug
callback and the suspend notifier will use it.
Cc: Chen Yu <yu.c.chen@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Zhao Liu <zhao1.liu@linux.intel.com>
Cc: linux-pm@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
drivers/thermal/intel/intel_hfi.c | 46 +++++++++++++++++--------------
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
index c69db6c90869..87ac7b196981 100644
--- a/drivers/thermal/intel/intel_hfi.c
+++ b/drivers/thermal/intel/intel_hfi.c
@@ -347,6 +347,25 @@ static void init_hfi_instance(struct hfi_instance *hfi_instance)
hfi_instance->data = hfi_instance->hdr + hfi_features.hdr_size;
}
+static void hfi_enable(void)
+{
+ u64 msr_val;
+
+ rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
+ msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
+ wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
+}
+
+static void hfi_set_hw_table(struct hfi_instance *hfi_instance)
+{
+ phys_addr_t hw_table_pa;
+ u64 msr_val;
+
+ hw_table_pa = virt_to_phys(hfi_instance->hw_table);
+ msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
+ wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
+}
+
/**
* intel_hfi_online() - Enable HFI on @cpu
* @cpu: CPU in which the HFI will be enabled
@@ -364,8 +383,6 @@ void intel_hfi_online(unsigned int cpu)
{
struct hfi_instance *hfi_instance;
struct hfi_cpu_info *info;
- phys_addr_t hw_table_pa;
- u64 msr_val;
u16 die_id;
/* Nothing to do if hfi_instances are missing. */
@@ -403,14 +420,16 @@ void intel_hfi_online(unsigned int cpu)
/*
* Hardware is programmed with the physical address of the first page
* frame of the table. Hence, the allocated memory must be page-aligned.
+ *
+ * Some processors do not forget the initial address of the HFI table
+ * even after having been reprogrammed. Keep using the same pages. Do
+ * not free them.
*/
hfi_instance->hw_table = alloc_pages_exact(hfi_features.nr_table_pages,
GFP_KERNEL | __GFP_ZERO);
if (!hfi_instance->hw_table)
goto unlock;
- hw_table_pa = virt_to_phys(hfi_instance->hw_table);
-
/*
* Allocate memory to keep a local copy of the table that
* hardware generates.
@@ -420,16 +439,6 @@ void intel_hfi_online(unsigned int cpu)
if (!hfi_instance->local_table)
goto free_hw_table;
- /*
- * Program the address of the feedback table of this die/package. On
- * some processors, hardware remembers the old address of the HFI table
- * even after having been reprogrammed and re-enabled. Thus, do not free
- * the pages allocated for the table or reprogram the hardware with a
- * new base address. Namely, program the hardware only once.
- */
- msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
- wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
-
init_hfi_instance(hfi_instance);
INIT_DELAYED_WORK(&hfi_instance->update_work, hfi_update_work_fn);
@@ -438,13 +447,8 @@ void intel_hfi_online(unsigned int cpu)
cpumask_set_cpu(cpu, hfi_instance->cpus);
- /*
- * Enable the hardware feedback interface and never disable it. See
- * comment on programming the address of the table.
- */
- rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
- msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
- wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
+ hfi_set_hw_table(hfi_instance);
+ hfi_enable();
unlock:
mutex_unlock(&hfi_instance_lock);
--
2.25.1On Wed, Dec 27, 2023 at 7:28 AM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> In preparation to add a suspend notifier, wrap the logic to enable HFI and
> program its memory buffer into helper functions. Both the CPU hotplug
> callback and the suspend notifier will use it.
No functional impact?
> Cc: Chen Yu <yu.c.chen@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Zhao Liu <zhao1.liu@linux.intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: stable@vger.kernel.org
Please don't CC stable@vger on patch submissions, although you may add
a Cc: stable tag without actually CCing it for my information, but in
that case please add a full tag including the earliest stable series
the patch is intended to apply to.
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> drivers/thermal/intel/intel_hfi.c | 46 +++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index c69db6c90869..87ac7b196981 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -347,6 +347,25 @@ static void init_hfi_instance(struct hfi_instance *hfi_instance)
> hfi_instance->data = hfi_instance->hdr + hfi_features.hdr_size;
> }
>
> +static void hfi_enable(void)
> +{
> + u64 msr_val;
> +
> + rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> + msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> + wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> +}
> +
> +static void hfi_set_hw_table(struct hfi_instance *hfi_instance)
> +{
> + phys_addr_t hw_table_pa;
> + u64 msr_val;
> +
> + hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> + msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> + wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> +}
> +
> /**
> * intel_hfi_online() - Enable HFI on @cpu
> * @cpu: CPU in which the HFI will be enabled
> @@ -364,8 +383,6 @@ void intel_hfi_online(unsigned int cpu)
> {
> struct hfi_instance *hfi_instance;
> struct hfi_cpu_info *info;
> - phys_addr_t hw_table_pa;
> - u64 msr_val;
> u16 die_id;
>
> /* Nothing to do if hfi_instances are missing. */
> @@ -403,14 +420,16 @@ void intel_hfi_online(unsigned int cpu)
> /*
> * Hardware is programmed with the physical address of the first page
> * frame of the table. Hence, the allocated memory must be page-aligned.
> + *
> + * Some processors do not forget the initial address of the HFI table
> + * even after having been reprogrammed. Keep using the same pages. Do
> + * not free them.
This comment change does not seem to belong to this patch. I guess it
needs to go to one of the subsequent patches?
> */
> hfi_instance->hw_table = alloc_pages_exact(hfi_features.nr_table_pages,
> GFP_KERNEL | __GFP_ZERO);
> if (!hfi_instance->hw_table)
> goto unlock;
>
> - hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> -
> /*
> * Allocate memory to keep a local copy of the table that
> * hardware generates.
> @@ -420,16 +439,6 @@ void intel_hfi_online(unsigned int cpu)
> if (!hfi_instance->local_table)
> goto free_hw_table;
>
> - /*
> - * Program the address of the feedback table of this die/package. On
> - * some processors, hardware remembers the old address of the HFI table
> - * even after having been reprogrammed and re-enabled. Thus, do not free
> - * the pages allocated for the table or reprogram the hardware with a
> - * new base address. Namely, program the hardware only once.
> - */
> - msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> - wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> -
> init_hfi_instance(hfi_instance);
>
> INIT_DELAYED_WORK(&hfi_instance->update_work, hfi_update_work_fn);
> @@ -438,13 +447,8 @@ void intel_hfi_online(unsigned int cpu)
>
> cpumask_set_cpu(cpu, hfi_instance->cpus);
>
> - /*
> - * Enable the hardware feedback interface and never disable it. See
> - * comment on programming the address of the table.
> - */
> - rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> - msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> - wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> + hfi_set_hw_table(hfi_instance);
> + hfi_enable();
>
> unlock:
> mutex_unlock(&hfi_instance_lock);
> --
On Fri, Dec 29, 2023 at 06:22:25PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 27, 2023 at 7:28 AM Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> >
> > In preparation to add a suspend notifier, wrap the logic to enable HFI and
> > program its memory buffer into helper functions. Both the CPU hotplug
> > callback and the suspend notifier will use it.
>
> No functional impact?
Thank you for your review!
Correct. There is no functional impact. I will update the commit message
in my next version.
>
> > Cc: Chen Yu <yu.c.chen@intel.com>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Zhao Liu <zhao1.liu@linux.intel.com>
> > Cc: linux-pm@vger.kernel.org
> > Cc: stable@vger.kernel.org
>
> Please don't CC stable@vger on patch submissions, although you may add
> a Cc: stable tag without actually CCing it for my information, but in
> that case please add a full tag including the earliest stable series
> the patch is intended to apply to.
I see. Sure Rafael, I can do this.
>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > drivers/thermal/intel/intel_hfi.c | 46 +++++++++++++++++--------------
> > 1 file changed, 25 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> > index c69db6c90869..87ac7b196981 100644
> > --- a/drivers/thermal/intel/intel_hfi.c
> > +++ b/drivers/thermal/intel/intel_hfi.c
> > @@ -347,6 +347,25 @@ static void init_hfi_instance(struct hfi_instance *hfi_instance)
> > hfi_instance->data = hfi_instance->hdr + hfi_features.hdr_size;
> > }
> >
> > +static void hfi_enable(void)
> > +{
> > + u64 msr_val;
> > +
> > + rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > + msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> > + wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > +}
> > +
> > +static void hfi_set_hw_table(struct hfi_instance *hfi_instance)
> > +{
> > + phys_addr_t hw_table_pa;
> > + u64 msr_val;
> > +
> > + hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> > + msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> > + wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> > +}
> > +
> > /**
> > * intel_hfi_online() - Enable HFI on @cpu
> > * @cpu: CPU in which the HFI will be enabled
> > @@ -364,8 +383,6 @@ void intel_hfi_online(unsigned int cpu)
> > {
> > struct hfi_instance *hfi_instance;
> > struct hfi_cpu_info *info;
> > - phys_addr_t hw_table_pa;
> > - u64 msr_val;
> > u16 die_id;
> >
> > /* Nothing to do if hfi_instances are missing. */
> > @@ -403,14 +420,16 @@ void intel_hfi_online(unsigned int cpu)
> > /*
> > * Hardware is programmed with the physical address of the first page
> > * frame of the table. Hence, the allocated memory must be page-aligned.
> > + *
> > + * Some processors do not forget the initial address of the HFI table
> > + * even after having been reprogrammed. Keep using the same pages. Do
> > + * not free them.
>
> This comment change does not seem to belong to this patch. I guess it
> needs to go to one of the subsequent patches?
My intention was is to relocate here the comment I deleted in the
subsequent hunk (after some rewording). Sure, I can put this comment in
patch 3/4, which deals with disabling HFI.
>
> > */
> > hfi_instance->hw_table = alloc_pages_exact(hfi_features.nr_table_pages,
> > GFP_KERNEL | __GFP_ZERO);
> > if (!hfi_instance->hw_table)
> > goto unlock;
> >
> > - hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> > -
> > /*
> > * Allocate memory to keep a local copy of the table that
> > * hardware generates.
> > @@ -420,16 +439,6 @@ void intel_hfi_online(unsigned int cpu)
> > if (!hfi_instance->local_table)
> > goto free_hw_table;
> >
> > - /*
> > - * Program the address of the feedback table of this die/package. On
> > - * some processors, hardware remembers the old address of the HFI table
> > - * even after having been reprogrammed and re-enabled. Thus, do not free
> > - * the pages allocated for the table or reprogram the hardware with a
> > - * new base address. Namely, program the hardware only once.
> > - */
> > - msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> > - wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> > -
> > init_hfi_instance(hfi_instance);
> >
> > INIT_DELAYED_WORK(&hfi_instance->update_work, hfi_update_work_fn);
> > @@ -438,13 +447,8 @@ void intel_hfi_online(unsigned int cpu)
> >
> > cpumask_set_cpu(cpu, hfi_instance->cpus);
> >
> > - /*
> > - * Enable the hardware feedback interface and never disable it. See
> > - * comment on programming the address of the table.
> > - */
> > - rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > - msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> > - wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > + hfi_set_hw_table(hfi_instance);
> > + hfi_enable();
> >
> > unlock:
> > mutex_unlock(&hfi_instance_lock);
> > --
© 2016 - 2025 Red Hat, Inc.