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

[Ricardo Neri]

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@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?

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@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.

I see. Sure Rafael, I can do this.

> 
> > 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?

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