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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2023 14:58:18 -0800
From:   Ashok Raj <ashok.raj@...el.com>
To:     "Zhang, Rui" <rui.zhang@...el.com>
CC:     "ashok_raj@...ux.intel.com" <ashok_raj@...ux.intel.com>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH 3/3] hwmon: (coretemp) Fix core count limitation

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?