linux-kernel - Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

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]

Message-ID: <Yvi8aBtytJ1pEDoe@gmail.com>
Date:   Sun, 14 Aug 2022 11:12:08 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Zhang Rui <rui.zhang@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-hwmon@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        corbet@....net, fenghua.yu@...el.com, jdelvare@...e.com,
        linux@...ck-us.net, len.brown@...el.com
Subject: Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value


* Zhang Rui <rui.zhang@...el.com> wrote:

> On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote:
> > 
> > * Zhang Rui <rui.zhang@...el.com> wrote:
> > 
> > > The coretemp driver supports up to a hard-coded limit of 128 cores.
> > > 
> > > Today, the driver can not support a core with an id above that
> > > limit.
> > > Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so
> > > they
> > > may be sparse and they may be large.
> > > 
> > > Update the driver to map arbitrary core_id numbers into appropriate
> > > array indexes so that 128 cores can be supported, no matter the
> > > encoding
> > > of core_ids's.
> > 
> > Please capitalize 'ID' consistently throughout the series.
> > 
> > > -       attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
> > > +       if (pkg_flag)
> > > +               attr_no = PKG_SYSFS_ATTR_NO;
> > > +       else {
> > > +               index = ida_alloc(&pdata->ida, GFP_KERNEL);
> > > +               if (index < 0)
> > > +                       return index;
> > > +               pdata->cpu_map[index] = topology_core_id(cpu);
> > > +               attr_no = index + BASE_SYSFS_ATTR_NO;
> > > +       }
> > 
> > Unbalanced curly braces.
> 
> Sure, will fix these two issues in next version.
> 
> > 
> > > -       int err, attr_no;
> > > +       int err, index, attr_no;
> > 
> > So it's 'index' here.
> > 
> > > @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct
> > > platform_data *pdata, int indx)
> > 
> > But 'indx' here.
> > 
> > > -       int indx, target;
> > > +       int i, indx = -1, target;
> > 
> > And 'indx' again. Did we run out of the letter 'e'? Either use
> > 'index' 
> > naming consistently, or 'idx' if it has to be abbreviated.
> 
> I'd prefer 'index', but here, this 'indx' is from previous code and
> this patch just initializes it to -1.

Then queue up a cleanup patch as patch #1 - but idiosyncratic noise like 
that makes review harder.

Thanks,

	Ingo