Re: [PATCH 1/4] thermal: intel: hfi: Refactor enabling code into helper functions

[Rafael J. Wysocki]

On Wed, Dec 27, 2023 at 7:28 AM Ricardo Neri
<ricardo.neri-calderon@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Len Brown <len.brown@xxxxxxxxx>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Cc: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> Cc: Zhao Liu <zhao1.liu@xxxxxxxxxxxxxxx>
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx

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@xxxxxxxxxxxxxxx>
> ---
>  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);
> --