| Message ID | 20220812164144.30829-4-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | x86/topology: Improve CPUID.1F handling | expand |
On Sat, Aug 13, 2022 at 12:41:40AM +0800, Zhang Rui 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. > > Acked-by: Len Brown <len.brown@intel.com> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/hwmon/coretemp.c | 55 +++++++++++++++++++++++++++++----------- > 1 file changed, 40 insertions(+), 15 deletions(-) > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > index ccf0af5b988a..3f0f7d7612ae 100644 > --- a/drivers/hwmon/coretemp.c > +++ b/drivers/hwmon/coretemp.c > @@ -46,9 +46,6 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); > #define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) > #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) > > -#define TO_CORE_ID(cpu) (cpu_data(cpu).cpu_core_id) > -#define TO_ATTR_NO(cpu) (TO_CORE_ID(cpu) + BASE_SYSFS_ATTR_NO) > - > #ifdef CONFIG_SMP > #define for_each_sibling(i, cpu) \ > for_each_cpu(i, topology_sibling_cpumask(cpu)) > @@ -91,6 +88,8 @@ struct temp_data { > struct platform_data { > struct device *hwmon_dev; > u16 pkg_id; > + u16 cpu_map[NUM_REAL_CORES]; > + struct ida ida; > struct cpumask cpumask; > struct temp_data *core_data[MAX_CORE_DATA]; > struct device_attribute name_attr; > @@ -441,7 +440,7 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag) > MSR_IA32_THERM_STATUS; > tdata->is_pkg_data = pkg_flag; > tdata->cpu = cpu; > - tdata->cpu_core_id = TO_CORE_ID(cpu); > + tdata->cpu_core_id = topology_core_id(cpu); > tdata->attr_size = MAX_CORE_ATTRS; > mutex_init(&tdata->update_lock); > return tdata; > @@ -454,7 +453,7 @@ 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, attr_no; > + int err, index, attr_no; > > /* > * Find attr number for sysfs: > @@ -462,14 +461,26 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu, > * The attr number is always core id + 2 > * The Pkgtemp will always show up as temp1_*, if available > */ > - 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; > + } > > - if (attr_no > MAX_CORE_DATA - 1) > - return -ERANGE; > + if (attr_no > MAX_CORE_DATA - 1) { > + err = -ERANGE; > + goto ida_free; > + } > > tdata = init_temp_data(cpu, pkg_flag); > - if (!tdata) > - return -ENOMEM; > + if (!tdata) { > + err = -ENOMEM; > + goto ida_free; > + } > > /* Test if we can access the status register */ > err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax, &edx); > @@ -505,6 +516,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu, > exit_free: > pdata->core_data[attr_no] = NULL; > kfree(tdata); > +ida_free: > + if (!pkg_flag) > + ida_free(&pdata->ida, index); > return err; > } > > @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx) > > kfree(pdata->core_data[indx]); > pdata->core_data[indx] = NULL; > + > + ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO); > } > > static int coretemp_probe(struct platform_device *pdev) > @@ -537,6 +553,7 @@ static int coretemp_probe(struct platform_device *pdev) > return -ENOMEM; > > pdata->pkg_id = pdev->id; > + ida_init(&pdata->ida); > platform_set_drvdata(pdev, pdata); > > pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME, > @@ -553,6 +570,7 @@ static int coretemp_remove(struct platform_device *pdev) > if (pdata->core_data[i]) > coretemp_remove_core(pdata, i); > > + ida_destroy(&pdata->ida); > return 0; > } > > @@ -647,7 +665,7 @@ static int coretemp_cpu_offline(unsigned int cpu) > struct platform_device *pdev = coretemp_get_pdev(cpu); > struct platform_data *pd; > struct temp_data *tdata; > - int indx, target; > + int i, indx = -1, target; > > /* > * Don't execute this on suspend as the device remove locks > @@ -660,12 +678,19 @@ static int coretemp_cpu_offline(unsigned int cpu) > if (!pdev) > return 0; > > - /* The core id is too big, just return */ > - indx = TO_ATTR_NO(cpu); > - if (indx > MAX_CORE_DATA - 1) > + pd = platform_get_drvdata(pdev); > + > + for (i = 0; i < NUM_REAL_CORES; i++) { > + if (pd->cpu_map[i] == topology_core_id(cpu)) { > + indx = i + BASE_SYSFS_ATTR_NO; > + break; > + } > + } > + > + /* Too many cores and this core is not pupolated, just return */ populated Other than that looks good. Acked-by: Guenter Roeck <linux@roeck-us.net> > + if (indx < 0) > return 0; > > - pd = platform_get_drvdata(pdev); > tdata = pd->core_data[indx]; > > cpumask_clear_cpu(cpu, &pd->cpumask); > -- > 2.34.1 >
* Zhang Rui <rui.zhang@intel.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. > - 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. Thanks, Ingo
On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote: > > * Zhang Rui <rui.zhang@intel.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. thanks, rui > > Thanks, > > Ingo
Hi, Guenter, On Fri, 2022-08-12 at 10:16 -0700, Guenter Roeck wrote: > > > - /* The core id is too big, just return */ > > - indx = TO_ATTR_NO(cpu); > > - if (indx > MAX_CORE_DATA - 1) > > + pd = platform_get_drvdata(pdev); > > + > > + for (i = 0; i < NUM_REAL_CORES; i++) { > > + if (pd->cpu_map[i] == topology_core_id(cpu)) { > > + indx = i + BASE_SYSFS_ATTR_NO; > > + break; > > + } > > + } > > + > > + /* Too many cores and this core is not pupolated, just > > return */ > > populated > > Other than that looks good. > > Acked-by: Guenter Roeck <linux@roeck-us.net> Thanks for reviewing, will fix in next version. thanks, rui
* Zhang Rui <rui.zhang@intel.com> wrote: > On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote: > > > > * Zhang Rui <rui.zhang@intel.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
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c index ccf0af5b988a..3f0f7d7612ae 100644 --- a/drivers/hwmon/coretemp.c +++ b/drivers/hwmon/coretemp.c @@ -46,9 +46,6 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); #define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) -#define TO_CORE_ID(cpu) (cpu_data(cpu).cpu_core_id) -#define TO_ATTR_NO(cpu) (TO_CORE_ID(cpu) + BASE_SYSFS_ATTR_NO) - #ifdef CONFIG_SMP #define for_each_sibling(i, cpu) \ for_each_cpu(i, topology_sibling_cpumask(cpu)) @@ -91,6 +88,8 @@ struct temp_data { struct platform_data { struct device *hwmon_dev; u16 pkg_id; + u16 cpu_map[NUM_REAL_CORES]; + struct ida ida; struct cpumask cpumask; struct temp_data *core_data[MAX_CORE_DATA]; struct device_attribute name_attr; @@ -441,7 +440,7 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag) MSR_IA32_THERM_STATUS; tdata->is_pkg_data = pkg_flag; tdata->cpu = cpu; - tdata->cpu_core_id = TO_CORE_ID(cpu); + tdata->cpu_core_id = topology_core_id(cpu); tdata->attr_size = MAX_CORE_ATTRS; mutex_init(&tdata->update_lock); return tdata; @@ -454,7 +453,7 @@ 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, attr_no; + int err, index, attr_no; /* * Find attr number for sysfs: @@ -462,14 +461,26 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu, * The attr number is always core id + 2 * The Pkgtemp will always show up as temp1_*, if available */ - 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; + } - if (attr_no > MAX_CORE_DATA - 1) - return -ERANGE; + if (attr_no > MAX_CORE_DATA - 1) { + err = -ERANGE; + goto ida_free; + } tdata = init_temp_data(cpu, pkg_flag); - if (!tdata) - return -ENOMEM; + if (!tdata) { + err = -ENOMEM; + goto ida_free; + } /* Test if we can access the status register */ err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax, &edx); @@ -505,6 +516,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu, exit_free: pdata->core_data[attr_no] = NULL; kfree(tdata); +ida_free: + if (!pkg_flag) + ida_free(&pdata->ida, index); return err; } @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx) kfree(pdata->core_data[indx]); pdata->core_data[indx] = NULL; + + ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO); } static int coretemp_probe(struct platform_device *pdev) @@ -537,6 +553,7 @@ static int coretemp_probe(struct platform_device *pdev) return -ENOMEM; pdata->pkg_id = pdev->id; + ida_init(&pdata->ida); platform_set_drvdata(pdev, pdata); pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME, @@ -553,6 +570,7 @@ static int coretemp_remove(struct platform_device *pdev) if (pdata->core_data[i]) coretemp_remove_core(pdata, i); + ida_destroy(&pdata->ida); return 0; } @@ -647,7 +665,7 @@ static int coretemp_cpu_offline(unsigned int cpu) struct platform_device *pdev = coretemp_get_pdev(cpu); struct platform_data *pd; struct temp_data *tdata; - int indx, target; + int i, indx = -1, target; /* * Don't execute this on suspend as the device remove locks @@ -660,12 +678,19 @@ static int coretemp_cpu_offline(unsigned int cpu) if (!pdev) return 0; - /* The core id is too big, just return */ - indx = TO_ATTR_NO(cpu); - if (indx > MAX_CORE_DATA - 1) + pd = platform_get_drvdata(pdev); + + for (i = 0; i < NUM_REAL_CORES; i++) { + if (pd->cpu_map[i] == topology_core_id(cpu)) { + indx = i + BASE_SYSFS_ATTR_NO; + break; + } + } + + /* Too many cores and this core is not pupolated, just return */ + if (indx < 0) return 0; - pd = platform_get_drvdata(pdev); tdata = pd->core_data[indx]; cpumask_clear_cpu(cpu, &pd->cpumask);