Re: [PATCH 3/3] hwmon: (coretemp) Fix core count limitation

[Ashok Raj <ashok.raj@intel.com>]

On Fri, Dec 01, 2023 at 09:47:30AM -0800, Zhang, Rui wrote:
> On Thu, 2023-11-30 at 18:08 -0800, Ashok Raj wrote:
> > On Mon, Nov 27, 2023 at 09:16:51PM +0800, Zhang Rui wrote:
> > >  
> > > +static struct temp_data *get_tdata(struct platform_data *pdata,
> > > int cpu)
> > > +{
> > > +       struct temp_data *tdata;
> > > +
> > > +       mutex_lock(&pdata->core_data_lock);
> > > +       list_for_each_entry(tdata, &pdata->core_data_list, node) {
> > > +               if (cpu >= 0 && !tdata->is_pkg_data && tdata-
> > > >cpu_core_id == topology_core_id(cpu))
> > > +                       goto found;
> > > +               if (cpu < 0 && tdata->is_pkg_data)
> > > +                       goto found;
> > > +       }
> > > +       tdata = NULL;
> > 
> > What used to be an array, is now a list? Is it possible to get the
> > number
> > of cores_per_package during initialization and allocate the per-core?
> > You
> > can still get them indexing from core_id and you can possibly lose
> > the
> > mutex and search?
> > 
> > I don't know this code well enough... Just a thought.
> 
> yeah, sadly cores_per_package is not available for now as I mentioned
> in the other email.

Couldn't we reuse the logic from Thomas's topology work that gives this
upfront based on 0x1f?

> 
> > 
> > > +found:
> > > +       mutex_unlock(&pdata->core_data_lock);
> > > +       return tdata;
> > > +}
> > > +
> > >  static int create_core_data(struct platform_device *pdev, unsigned
> > > int cpu,
> > >                             int pkg_flag)
> > >  {
> > > @@ -498,37 +511,29 @@ static int create_core_data(struct
> > > platform_device *pdev, unsigned int cpu,
> > >         struct platform_data *pdata = platform_get_drvdata(pdev);
> > >         struct cpuinfo_x86 *c = &cpu_data(cpu);
> > >         u32 eax, edx;
> > > -       int err, index, attr_no;
> > > +       int err, attr_no;
> > >  
> > >         if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
> > >                 return 0;
> > >  
> > > +       tdata = get_tdata(pdata, pkg_flag ? -1 : cpu);
> > > +       if (tdata)
> > > +               return -EEXIST;
> > > +
> > > +       tdata = init_temp_data(cpu, pkg_flag);
> > 
> > Is temp_data per_cpu or per_core?
> 
> it is per_core.
> 
> >  Wasn't sure if temp_data needs a CPU
> > number there along with cpu_core_id
> 
> CPU number is needed to access the core temperature MSRs.

What if that cpu is currently offline? maybe you can simply search
for_each_online_cpu() and match core_id from cpuinfo_x86?