On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote:
>
> * Zhang Rui <rui.zhang@xxxxxxxxx> 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.
thanks,
rui
>
> Thanks,
>
> Ingo
