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:
> > Currently, coretemp driver only supports 128 cores per package.
> > This loses some core temperation information on systems that have
> > more
>
> s/temperation/temperature
>
> > than 128 cores per package.
> > [ 58.685033] coretemp coretemp.0: Adding Core 128 failed
> > [ 58.692009] coretemp coretemp.0: Adding Core 129 failed
> >
> > Fix the problem by using a per package list to maintain the per
> > core
> > temp_data instead of the fixed length pdata->core_data[] array.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> > drivers/hwmon/coretemp.c | 110 ++++++++++++++++++-----------------
> > ----
> > 1 file changed, 52 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > index cef43fedbd58..1bb1a6e4b07b 100644
> > --- a/drivers/hwmon/coretemp.c
> > +++ b/drivers/hwmon/coretemp.c
> > @@ -39,11 +39,7 @@ static int force_tjmax;
> > module_param_named(tjmax, force_tjmax, int, 0444);
> > MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
> >
> > -#define PKG_SYSFS_ATTR_NO 1 /* Sysfs attribute for
> > package temp */
> > -#define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no for
> > coretemp */
> > -#define NUM_REAL_CORES 128 /* Number of Real cores per
> > cpu */
> > #define CORETEMP_NAME_LENGTH 28 /* String Length of attrs
> > */
> > -#define MAX_CORE_DATA (NUM_REAL_CORES +
> > BASE_SYSFS_ATTR_NO)
> >
> > enum coretemp_attr_index {
> > ATTR_LABEL,
> > @@ -90,17 +86,17 @@ struct temp_data {
> > struct attribute *attrs[TOTAL_ATTRS + 1];
> > struct attribute_group attr_group;
> > struct mutex update_lock;
> > + struct list_head node;
> > };
> >
> > /* Platform Data per Physical CPU */
> > 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;
> > + struct mutex core_data_lock;
> > + struct list_head core_data_list;
> > };
> >
> > struct tjmax_pci {
> > @@ -491,6 +487,23 @@ static struct temp_data
> > *init_temp_data(unsigned int cpu, int pkg_flag)
> > return tdata;
> > }
> >
> > +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.
>
> > +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.
thanks,
rui
>
>
> > + if (!tdata)
> > + return -ENOMEM;
> > +
> > /*
> > * Find attr number for sysfs:
> > * We map the attr number to core id of the CPU
> > * The attr number is always core id + 2
> > * The Pkgtemp will always show up as temp1_*, if available
> > */
> > - 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) {
> > - err = -ERANGE;
> > - goto ida_free;
> > - }
> > -
> > - tdata = init_temp_data(cpu, pkg_flag);
> > - if (!tdata) {
> > - err = -ENOMEM;
> > - goto ida_free;
> > - }
> > + if (pkg_flag)
> > + attr_no = 1;
> > + else
> > + attr_no = tdata->cpu_core_id + 2;
> >
> > /* Test if we can access the status register */
> > err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax,
> > &edx);
> > @@ -547,20 +552,18 @@ static int create_core_data(struct
> > platform_device *pdev, unsigned int cpu,
> > if (get_ttarget(tdata, &pdev->dev) >= 0)
> > tdata->attr_size++;
> >
> > - pdata->core_data[attr_no] = tdata;
> > -
> > /* Create sysfs interfaces */
> > err = create_core_attrs(tdata, pdata->hwmon_dev, attr_no);
> > if (err)
> > goto exit_free;
> >
> > + mutex_lock(&pdata->core_data_lock);
> > + list_add(&tdata->node, &pdata->core_data_list);
> > + mutex_unlock(&pdata->core_data_lock);
> > +
> > return 0;
> > exit_free:
> > - pdata->core_data[attr_no] = NULL;
> > kfree(tdata);
> > -ida_free:
> > - if (!pkg_flag)
> > - ida_free(&pdata->ida, index);
> > return err;
> > }
> >
> > @@ -571,9 +574,9 @@ coretemp_add_core(struct platform_device *pdev,
> > unsigned int cpu, int pkg_flag)
> > dev_err(&pdev->dev, "Adding Core %u failed\n",
> > cpu);
> > }
> >
> > -static void coretemp_remove_core(struct platform_data *pdata, int
> > indx)
> > +static void coretemp_remove_core(struct platform_data *pdata, int
> > cpu)
> > {
> > - struct temp_data *tdata = pdata->core_data[indx];
> > + struct temp_data *tdata = get_tdata(pdata, cpu);
> >
> > /* if we errored on add then this is already gone */
> > if (!tdata)
> > @@ -582,11 +585,11 @@ static void coretemp_remove_core(struct
> > platform_data *pdata, int indx)
> > /* Remove the sysfs attributes */
> > sysfs_remove_group(&pdata->hwmon_dev->kobj, &tdata-
> > >attr_group);
> >
> > - kfree(pdata->core_data[indx]);
> > - pdata->core_data[indx] = NULL;
> > + mutex_lock(&pdata->core_data_lock);
> > + list_del(&tdata->node);
> > + mutex_unlock(&pdata->core_data_lock);
> >
> > - if (indx >= BASE_SYSFS_ATTR_NO)
> > - ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO);
> > + kfree(tdata);
> > }
> >
> > static int coretemp_device_add(int zoneid)
> > @@ -601,7 +604,8 @@ static int coretemp_device_add(int zoneid)
> > return -ENOMEM;
> >
> > pdata->pkg_id = zoneid;
> > - ida_init(&pdata->ida);
> > + mutex_init(&pdata->core_data_lock);
> > + INIT_LIST_HEAD(&pdata->core_data_list);
> >
> > pdev = platform_device_alloc(DRVNAME, zoneid);
> > if (!pdev) {
> > @@ -629,7 +633,6 @@ static void coretemp_device_remove(int zoneid)
> > struct platform_device *pdev = zone_devices[zoneid];
> > struct platform_data *pdata = platform_get_drvdata(pdev);
> >
> > - ida_destroy(&pdata->ida);
> > kfree(pdata);
> > platform_device_unregister(pdev);
> > }
> > @@ -699,7 +702,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 i, indx = -1, target;
> > + int target;
> >
> > /* No need to tear down any interfaces for suspend */
> > if (cpuhp_tasks_frozen)
> > @@ -710,19 +713,10 @@ static int coretemp_cpu_offline(unsigned int
> > cpu)
> > if (!pd->hwmon_dev)
> > return 0;
> >
> > - 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 populated, just
> > return */
> > - if (indx < 0)
> > + tdata = get_tdata(pd, cpu);
> > + if (!tdata)
> > return 0;
> >
> > - tdata = pd->core_data[indx];
> > -
> > cpumask_clear_cpu(cpu, &pd->cpumask);
> >
> > /*
> > @@ -732,20 +726,20 @@ static int coretemp_cpu_offline(unsigned int
> > cpu)
> > */
> > target = cpumask_any_and(&pd->cpumask,
> > topology_sibling_cpumask(cpu));
> > if (target >= nr_cpu_ids) {
> > - coretemp_remove_core(pd, indx);
> > - } else if (tdata && tdata->cpu == cpu) {
> > + coretemp_remove_core(pd, cpu);
> > + } else if (tdata->cpu == cpu) {
> > mutex_lock(&tdata->update_lock);
> > tdata->cpu = target;
> > mutex_unlock(&tdata->update_lock);
> > }
> >
> > + tdata = get_tdata(pd, -1);
> > /*
> > * If all cores in this pkg are offline, remove the
> > interface.
> > */
> > - tdata = pd->core_data[PKG_SYSFS_ATTR_NO];
> > if (cpumask_empty(&pd->cpumask)) {
> > if (tdata)
> > - coretemp_remove_core(pd,
> > PKG_SYSFS_ATTR_NO);
> > + coretemp_remove_core(pd, -1);
> > hwmon_device_unregister(pd->hwmon_dev);
> > pd->hwmon_dev = NULL;
> > return 0;
> > --
> > 2.34.1
> >
