[PATCH V2 00/11] hwmon: (coretemp) Fixes, improvements and support for large core count

[PATCH V2 00/11] hwmon: (coretemp) Fixes, improvements and support for large core count

[Zhang Rui]

Patch 1/11 is a bug fix that should be considered as stable material.
Patch 2/11 fixes a user visible sysfs attribute name change.
Patch 3/11 is a quick fix to allow coretemp driver to probe more than
           128 cores.
Patch 4/11 - 10/11 are a series of improvements aim to simplify the
           code logic and remove unnecessary macros, variables and
           structure fields, and make it easier for patch 11/11.
Patch 11/11 converts coretemp driver to use dynamic memory allocation
           for core temp_data, so that it is easy to remove the
           hardcoded core count limitation when _num_cores_per_package
           become available and reliable, which is WIP in
           https://lore.kernel.org/all/20240118123127.055361964@linutronix.de/

I can split the first three patches into a separate patch set if needed.

Patch seris V1 has been posted at
https://lore.kernel.org/all/20231127131651.476795-1-rui.zhang@intel.com/

thanks,
rui

----------------------------------------------------------------
Zhang Rui (11):
      hwmon: (coretemp) Fix out-of-bounds memory access in create_core_data()
      hwmon: (coretemp) Fix bogus core to attr mapping
      hwmon: (coretemp) Enlarge per package core count limit
      hwmon: (coretemp) Introduce enum for attr index
      hwmon: (coretemp) Remove unnecessary dependency of array index
      hwmon: (coretemp) Replace sensor_device_attribute with device_attribute
      hwmon: (coretemp) Remove redundant pdata->cpu_map[]
      hwmon: (coretemp) Abstract core_temp helpers
      hwmon: (coretemp) Split package temp_data and core temp_data
      hwmon: (coretemp) Remove redundant temp_data->is_pkg_data
      hwmon: (coretemp) Use dynamic allocated memory for core temp_data

 drivers/hwmon/coretemp.c | 219 ++++++++++++++++++++++++++---------------------
 1 file changed, 120 insertions(+), 99 deletions(-)

[PATCH V2 01/11] hwmon: (coretemp) Fix out-of-bounds memory access

[Zhang Rui]

Fix a bug that pdata->cpu_map[] is set before out-of-bounds check.
The problem might be triggered on systems with more than 128 cores per
package.

Fixes: 7108b80a542b ("hwmon/coretemp: Handle large core ID value")
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hwmon/coretemp.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index ba82d1e79c13..e78c76919111 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -509,18 +509,14 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	if (pkg_flag) {
 		attr_no = PKG_SYSFS_ATTR_NO;
 	} else {
-		index = ida_alloc(&pdata->ida, GFP_KERNEL);
+		index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, 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;
-- 
2.34.1

Re: [PATCH V2 01/11] hwmon: (coretemp) Fix out-of-bounds memory access

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:34PM +0800, Zhang Rui wrote:
> Fix a bug that pdata->cpu_map[] is set before out-of-bounds check.
> The problem might be triggered on systems with more than 128 cores per
> package.
> 
> Fixes: 7108b80a542b ("hwmon/coretemp: Handle large core ID value")
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Cc: <stable@vger.kernel.org>

Applied.

Thanks,
Guenter

[PATCH V2 02/11] hwmon: (coretemp) Fix bogus core_id to attr name mapping

[Zhang Rui]

Before commit 7108b80a542b ("hwmon/coretemp: Handle large core ID
value"), there is a fixed mapping between
1. cpu_core_id
2. the index in pdata->core_data[] array
3. the sysfs attr name, aka "tempX_"
The later two always equal cpu_core_id + 2.

After the commit, pdata->core_data[] index is got from ida so that it
can handle sparse core ids and support more cores within a package.

However, the commit erroneously maps the sysfs attr name to
pdata->core_data[] index instead of cpu_core_id + 2.

As a result, the code is not aligned with the comments, and brings user
visible changes in hwmon sysfs on systems with sparse core id.

For example, before commit 7108b80a542b ("hwmon/coretemp: Handle large
core ID value"),
/sys/class/hwmon/hwmon2/temp2_label:Core 0
/sys/class/hwmon/hwmon2/temp3_label:Core 1
/sys/class/hwmon/hwmon2/temp4_label:Core 2
/sys/class/hwmon/hwmon2/temp5_label:Core 3
/sys/class/hwmon/hwmon2/temp6_label:Core 4
/sys/class/hwmon/hwmon3/temp10_label:Core 8
/sys/class/hwmon/hwmon3/temp11_label:Core 9
after commit,
/sys/class/hwmon/hwmon2/temp2_label:Core 0
/sys/class/hwmon/hwmon2/temp3_label:Core 1
/sys/class/hwmon/hwmon2/temp4_label:Core 2
/sys/class/hwmon/hwmon2/temp5_label:Core 3
/sys/class/hwmon/hwmon2/temp6_label:Core 4
/sys/class/hwmon/hwmon2/temp7_label:Core 8
/sys/class/hwmon/hwmon2/temp8_label:Core 9

Restore the previous behavior and rework the code, comments and variable
names to avoid future confusions.

Fixes: 7108b80a542b ("hwmon/coretemp: Handle large core ID value")
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index e78c76919111..95f4c0b00b2d 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -419,7 +419,7 @@ static ssize_t show_temp(struct device *dev,
 }
 
 static int create_core_attrs(struct temp_data *tdata, struct device *dev,
-			     int attr_no)
+			     int index)
 {
 	int i;
 	static ssize_t (*const rd_ptr[TOTAL_ATTRS]) (struct device *dev,
@@ -431,13 +431,20 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
 	};
 
 	for (i = 0; i < tdata->attr_size; i++) {
+		/*
+		 * 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
+		 */
+		int attr_no = tdata->is_pkg_data ? 1 : tdata->cpu_core_id + 2;
+
 		snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH,
 			 "temp%d_%s", attr_no, suffixes[i]);
 		sysfs_attr_init(&tdata->sd_attrs[i].dev_attr.attr);
 		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
 		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
 		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
-		tdata->sd_attrs[i].index = attr_no;
+		tdata->sd_attrs[i].index = index;
 		tdata->attrs[i] = &tdata->sd_attrs[i].dev_attr.attr;
 	}
 	tdata->attr_group.attrs = tdata->attrs;
@@ -495,26 +502,25 @@ 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, index;
 
 	if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
 		return 0;
 
 	/*
-	 * 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
+	 * Get the index of tdata in pdata->core_data[]
+	 * tdata for package: pdata->core_data[1]
+	 * tdata for core: pdata->core_data[2] .. pdata->core_data[NUM_REAL_CORES + 1]
 	 */
 	if (pkg_flag) {
-		attr_no = PKG_SYSFS_ATTR_NO;
+		index = PKG_SYSFS_ATTR_NO;
 	} else {
 		index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
 		if (index < 0)
 			return index;
 
 		pdata->cpu_map[index] = topology_core_id(cpu);
-		attr_no = index + BASE_SYSFS_ATTR_NO;
+		index += BASE_SYSFS_ATTR_NO;
 	}
 
 	tdata = init_temp_data(cpu, pkg_flag);
@@ -540,20 +546,20 @@ 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;
+	pdata->core_data[index] = tdata;
 
 	/* Create sysfs interfaces */
-	err = create_core_attrs(tdata, pdata->hwmon_dev, attr_no);
+	err = create_core_attrs(tdata, pdata->hwmon_dev, index);
 	if (err)
 		goto exit_free;
 
 	return 0;
 exit_free:
-	pdata->core_data[attr_no] = NULL;
+	pdata->core_data[index] = NULL;
 	kfree(tdata);
 ida_free:
 	if (!pkg_flag)
-		ida_free(&pdata->ida, index);
+		ida_free(&pdata->ida, index - BASE_SYSFS_ATTR_NO);
 	return err;
 }
 
-- 
2.34.1

Re: [PATCH V2 02/11] hwmon: (coretemp) Fix bogus core_id to attr name mapping

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:35PM +0800, Zhang Rui wrote:
> Before commit 7108b80a542b ("hwmon/coretemp: Handle large core ID
> value"), there is a fixed mapping between
> 1. cpu_core_id
> 2. the index in pdata->core_data[] array
> 3. the sysfs attr name, aka "tempX_"
> The later two always equal cpu_core_id + 2.
> 
> After the commit, pdata->core_data[] index is got from ida so that it
> can handle sparse core ids and support more cores within a package.
> 
> However, the commit erroneously maps the sysfs attr name to
> pdata->core_data[] index instead of cpu_core_id + 2.
> 
> As a result, the code is not aligned with the comments, and brings user
> visible changes in hwmon sysfs on systems with sparse core id.
> 
> For example, before commit 7108b80a542b ("hwmon/coretemp: Handle large
> core ID value"),
> /sys/class/hwmon/hwmon2/temp2_label:Core 0
> /sys/class/hwmon/hwmon2/temp3_label:Core 1
> /sys/class/hwmon/hwmon2/temp4_label:Core 2
> /sys/class/hwmon/hwmon2/temp5_label:Core 3
> /sys/class/hwmon/hwmon2/temp6_label:Core 4
> /sys/class/hwmon/hwmon3/temp10_label:Core 8
> /sys/class/hwmon/hwmon3/temp11_label:Core 9
> after commit,
> /sys/class/hwmon/hwmon2/temp2_label:Core 0
> /sys/class/hwmon/hwmon2/temp3_label:Core 1
> /sys/class/hwmon/hwmon2/temp4_label:Core 2
> /sys/class/hwmon/hwmon2/temp5_label:Core 3
> /sys/class/hwmon/hwmon2/temp6_label:Core 4
> /sys/class/hwmon/hwmon2/temp7_label:Core 8
> /sys/class/hwmon/hwmon2/temp8_label:Core 9
> 
> Restore the previous behavior and rework the code, comments and variable
> names to avoid future confusions.
> 
> Fixes: 7108b80a542b ("hwmon/coretemp: Handle large core ID value")
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 03/11] hwmon: (coretemp) Enlarge per package core count limit

[Zhang Rui]

Currently, coretemp driver supports only 128 cores per package.
This loses some core temperature information on systems that have more
than 128 cores per package.
 [   58.685033] coretemp coretemp.0: Adding Core 128 failed
 [   58.692009] coretemp coretemp.0: Adding Core 129 failed
 ...

Enlarge the limitation to 512 because there are platforms with more than
256 cores per package.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 95f4c0b00b2d..b8fc8d1ef20d 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -41,7 +41,7 @@ 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 NUM_REAL_CORES		512	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
 #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
 #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
-- 
2.34.1

Re: [PATCH V2 03/11] hwmon: (coretemp) Enlarge per package core count limit

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:36PM +0800, Zhang Rui wrote:
> Currently, coretemp driver supports only 128 cores per package.
> This loses some core temperature information on systems that have more
> than 128 cores per package.
>  [   58.685033] coretemp coretemp.0: Adding Core 128 failed
>  [   58.692009] coretemp coretemp.0: Adding Core 129 failed
>  ...
> 
> Enlarge the limitation to 512 because there are platforms with more than
> 256 cores per package.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 04/11] hwmon: (coretemp) Introduce enum for attr index

[Zhang Rui]

Introduce enum coretemp_attr_index to better describe the index of each
sensor attribute.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index b8fc8d1ef20d..32f99cf6308b 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
 #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
 #define NUM_REAL_CORES		512	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
-#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
-#define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
 #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
 
+enum coretemp_attr_index {
+	ATTR_LABEL,
+	ATTR_CRIT_ALARM,
+	ATTR_TEMP,
+	ATTR_TJMAX,
+	ATTR_TTARGET,
+	MAX_CORE_ATTRS = ATTR_TJMAX + 1,	/* Maximum no of basic attrs */
+	TOTAL_ATTRS = ATTR_TTARGET + 1		/* Maximum no of possible attrs */
+};
+
 #ifdef CONFIG_SMP
 #define for_each_sibling(i, cpu) \
 	for_each_cpu(i, topology_sibling_cpumask(cpu))
-- 
2.34.1

Re: [PATCH V2 04/11] hwmon: (coretemp) Introduce enum for attr index

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:37PM +0800, Zhang Rui wrote:
> Introduce enum coretemp_attr_index to better describe the index of each
> sensor attribute.
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 05/11] hwmon: (coretemp) Remove unnecessary dependency of array index

[Zhang Rui]

When sensor_device_attribute pointer is available, use container_of() to
get the temp_data address.

This removes the unnecessary dependency of cached index in
pdata->core_data[].

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 32f99cf6308b..9a7bfc046c72 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
 
 	if (tdata->is_pkg_data)
 		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
@@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev,
 {
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
 
 	mutex_lock(&tdata->update_lock);
 	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
@@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
 	int ttarget;
 
 	mutex_lock(&tdata->update_lock);
@@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev,
 {
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -426,8 +422,7 @@ static ssize_t show_temp(struct device *dev,
 	return sprintf(buf, "%d\n", tdata->temp);
 }
 
-static int create_core_attrs(struct temp_data *tdata, struct device *dev,
-			     int index)
+static int create_core_attrs(struct temp_data *tdata, struct device *dev)
 {
 	int i;
 	static ssize_t (*const rd_ptr[TOTAL_ATTRS]) (struct device *dev,
@@ -452,7 +447,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
 		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
 		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
 		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
-		tdata->sd_attrs[i].index = index;
 		tdata->attrs[i] = &tdata->sd_attrs[i].dev_attr.attr;
 	}
 	tdata->attr_group.attrs = tdata->attrs;
@@ -557,7 +551,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	pdata->core_data[index] = tdata;
 
 	/* Create sysfs interfaces */
-	err = create_core_attrs(tdata, pdata->hwmon_dev, index);
+	err = create_core_attrs(tdata, pdata->hwmon_dev);
 	if (err)
 		goto exit_free;
 
-- 
2.34.1

Re: [PATCH V2 05/11] hwmon: (coretemp) Remove unnecessary dependency of array index

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:38PM +0800, Zhang Rui wrote:
> When sensor_device_attribute pointer is available, use container_of() to
> get the temp_data address.
> 
> This removes the unnecessary dependency of cached index in
> pdata->core_data[].
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 06/11] hwmon: (coretemp) Replace sensor_device_attribute with device_attribute

[Zhang Rui]

Replace sensor_device_attribute with device_attribute because
sensor_device_attribute->index is no longer used.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 9a7bfc046c72..cdd1e069d5c1 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -85,7 +85,7 @@ struct temp_data {
 	u32 status_reg;
 	int attr_size;
 	bool is_pkg_data;
-	struct sensor_device_attribute sd_attrs[TOTAL_ATTRS];
+	struct device_attribute sd_attrs[TOTAL_ATTRS];
 	char attr_name[TOTAL_ATTRS][CORETEMP_NAME_LENGTH];
 	struct attribute *attrs[TOTAL_ATTRS + 1];
 	struct attribute_group attr_group;
@@ -340,9 +340,8 @@ static struct platform_device **zone_devices;
 static ssize_t show_label(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
+	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_LABEL]);
 
 	if (tdata->is_pkg_data)
 		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
@@ -354,8 +353,8 @@ static ssize_t show_crit_alarm(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
 	u32 eax, edx;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
+	struct temp_data *tdata = container_of(devattr, struct temp_data,
+						sd_attrs[ATTR_CRIT_ALARM]);
 
 	mutex_lock(&tdata->update_lock);
 	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
@@ -367,8 +366,7 @@ static ssize_t show_crit_alarm(struct device *dev,
 static ssize_t show_tjmax(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
+	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TJMAX]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -381,8 +379,7 @@ static ssize_t show_tjmax(struct device *dev,
 static ssize_t show_ttarget(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
+	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TTARGET]);
 	int ttarget;
 
 	mutex_lock(&tdata->update_lock);
@@ -398,8 +395,7 @@ static ssize_t show_temp(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
 	u32 eax, edx;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
+	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_TEMP]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -443,11 +439,11 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev)
 
 		snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH,
 			 "temp%d_%s", attr_no, suffixes[i]);
-		sysfs_attr_init(&tdata->sd_attrs[i].dev_attr.attr);
-		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
-		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
-		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
-		tdata->attrs[i] = &tdata->sd_attrs[i].dev_attr.attr;
+		sysfs_attr_init(&tdata->sd_attrs[i].attr);
+		tdata->sd_attrs[i].attr.name = tdata->attr_name[i];
+		tdata->sd_attrs[i].attr.mode = 0444;
+		tdata->sd_attrs[i].show = rd_ptr[i];
+		tdata->attrs[i] = &tdata->sd_attrs[i].attr;
 	}
 	tdata->attr_group.attrs = tdata->attrs;
 	return sysfs_create_group(&dev->kobj, &tdata->attr_group);
-- 
2.34.1

Re: [PATCH V2 06/11] hwmon: (coretemp) Replace sensor_device_attribute with device_attribute

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:39PM +0800, Zhang Rui wrote:
> Replace sensor_device_attribute with device_attribute because
> sensor_device_attribute->index is no longer used.
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 07/11] hwmon: (coretemp) Remove redundant pdata->cpu_map[]

[Zhang Rui]

pdata->cpu_map[] saves the mapping between cpu core id and the index in
pdata->core_data[]. This is used to find the temp_data structure using
cpu_core_id, by traversing the pdata->cpu_map[] array. But the same goal
can be achieved by traversing the pdata->core_temp[] array directly.

Remove redundant pdata->cpu_map[].

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index cdd1e069d5c1..29ee8e0c0fe9 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -96,7 +96,6 @@ 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];
@@ -517,7 +516,6 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 		if (index < 0)
 			return index;
 
-		pdata->cpu_map[index] = topology_core_id(cpu);
 		index += BASE_SYSFS_ATTR_NO;
 	}
 
@@ -696,7 +694,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 i, target;
 
 	/* No need to tear down any interfaces for suspend */
 	if (cpuhp_tasks_frozen)
@@ -707,18 +705,16 @@ 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;
+	for (i = BASE_SYSFS_ATTR_NO; i < MAX_CORE_DATA; i++) {
+		if (pd->core_data[i] && pd->core_data[i]->cpu_core_id == topology_core_id(cpu))
 			break;
-		}
 	}
 
 	/* Too many cores and this core is not populated, just return */
-	if (indx < 0)
+	if (i == MAX_CORE_DATA)
 		return 0;
 
-	tdata = pd->core_data[indx];
+	tdata = pd->core_data[i];
 
 	cpumask_clear_cpu(cpu, &pd->cpumask);
 
@@ -729,7 +725,7 @@ 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);
+		coretemp_remove_core(pd, i);
 	} else if (tdata && tdata->cpu == cpu) {
 		mutex_lock(&tdata->update_lock);
 		tdata->cpu = target;
-- 
2.34.1

Re: [PATCH V2 07/11] hwmon: (coretemp) Remove redundant pdata->cpu_map[]

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:40PM +0800, Zhang Rui wrote:
> pdata->cpu_map[] saves the mapping between cpu core id and the index in
> pdata->core_data[]. This is used to find the temp_data structure using
> cpu_core_id, by traversing the pdata->cpu_map[] array. But the same goal
> can be achieved by traversing the pdata->core_temp[] array directly.
> 
> Remove redundant pdata->cpu_map[].
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/coretemp.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index cdd1e069d5c1..29ee8e0c0fe9 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -96,7 +96,6 @@ 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];
> @@ -517,7 +516,6 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  		if (index < 0)
>  			return index;
>  
> -		pdata->cpu_map[index] = topology_core_id(cpu);
>  		index += BASE_SYSFS_ATTR_NO;
>  	}
>  
> @@ -696,7 +694,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 i, target;
>  
>  	/* No need to tear down any interfaces for suspend */
>  	if (cpuhp_tasks_frozen)
> @@ -707,18 +705,16 @@ 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;
> +	for (i = BASE_SYSFS_ATTR_NO; i < MAX_CORE_DATA; i++) {
> +		if (pd->core_data[i] && pd->core_data[i]->cpu_core_id == topology_core_id(cpu))
>  			break;
> -		}
>  	}
>  
>  	/* Too many cores and this core is not populated, just return */
> -	if (indx < 0)
> +	if (i == MAX_CORE_DATA)
>  		return 0;
>  
> -	tdata = pd->core_data[indx];
> +	tdata = pd->core_data[i];
>  
>  	cpumask_clear_cpu(cpu, &pd->cpumask);
>  
> @@ -729,7 +725,7 @@ 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);
> +		coretemp_remove_core(pd, i);
>  	} else if (tdata && tdata->cpu == cpu) {
>  		mutex_lock(&tdata->update_lock);
>  		tdata->cpu = target;

[PATCH V2 08/11] hwmon: (coretemp) Abstract core_temp helpers

[Zhang Rui]

coretemp driver has an obscure and fragile logic for handling package
and core temperature data.

Place the logic in newly introduced helpers for further optimizations.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 118 +++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 54 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 29ee8e0c0fe9..a19799a302a2 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -81,6 +81,7 @@ struct temp_data {
 	int tjmax;
 	unsigned long last_updated;
 	unsigned int cpu;
+	unsigned int index;
 	u32 cpu_core_id;
 	u32 status_reg;
 	int attr_size;
@@ -474,14 +475,36 @@ static struct platform_device *coretemp_get_pdev(unsigned int cpu)
 	return NULL;
 }
 
-static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag)
+static struct temp_data *
+init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 {
 	struct temp_data *tdata;
+	int index;
 
 	tdata = kzalloc(sizeof(struct temp_data), GFP_KERNEL);
 	if (!tdata)
 		return NULL;
 
+	/*
+	 * Get the index of tdata in pdata->core_data[]
+	 * tdata for package: pdata->core_data[1]
+	 * tdata for core: pdata->core_data[2] .. pdata->core_data[NUM_REAL_CORES + 1]
+	 */
+	if (pkg_flag) {
+		index = PKG_SYSFS_ATTR_NO;
+	} else {
+		index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
+		if (index < 0) {
+			kfree(tdata);
+			return NULL;
+		}
+		index += BASE_SYSFS_ATTR_NO;
+	}
+	/* Index in pdata->core_data[] */
+	tdata->index = index;
+
+	pdata->core_data[index] = tdata;
+
 	tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
 							MSR_IA32_THERM_STATUS;
 	tdata->is_pkg_data = pkg_flag;
@@ -492,6 +515,30 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag)
 	return tdata;
 }
 
+static void destroy_temp_data(struct platform_data *pdata, struct temp_data *tdata)
+{
+	pdata->core_data[tdata->index] = NULL;
+	if (!tdata->is_pkg_data)
+		ida_free(&pdata->ida, tdata->index - BASE_SYSFS_ATTR_NO);
+	kfree(tdata);
+}
+
+static struct temp_data *get_temp_data(struct platform_data *pdata, int cpu)
+{
+	int i;
+
+	/* cpu < 0 means get pkg temp_data */
+	if (cpu < 0)
+		return pdata->core_data[PKG_SYSFS_ATTR_NO];
+
+	for (i = BASE_SYSFS_ATTR_NO; i < MAX_CORE_DATA; i++) {
+		if (pdata->core_data[i] &&
+		    pdata->core_data[i]->cpu_core_id == topology_core_id(cpu))
+			return pdata->core_data[i];
+	}
+	return NULL;
+}
+
 static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 			    int pkg_flag)
 {
@@ -499,36 +546,19 @@ 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;
+	int err;
 
 	if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
 		return 0;
 
-	/*
-	 * Get the index of tdata in pdata->core_data[]
-	 * tdata for package: pdata->core_data[1]
-	 * tdata for core: pdata->core_data[2] .. pdata->core_data[NUM_REAL_CORES + 1]
-	 */
-	if (pkg_flag) {
-		index = PKG_SYSFS_ATTR_NO;
-	} else {
-		index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
-		if (index < 0)
-			return index;
-
-		index += BASE_SYSFS_ATTR_NO;
-	}
-
-	tdata = init_temp_data(cpu, pkg_flag);
-	if (!tdata) {
-		err = -ENOMEM;
-		goto ida_free;
-	}
+	tdata = init_temp_data(pdata, cpu, pkg_flag);
+	if (!tdata)
+		return -ENOMEM;
 
 	/* Test if we can access the status register */
 	err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax, &edx);
 	if (err)
-		goto exit_free;
+		goto err;
 
 	/* Make sure tdata->tjmax is a valid indicator for dynamic/static tjmax */
 	get_tjmax(tdata, &pdev->dev);
@@ -542,20 +572,15 @@ 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[index] = tdata;
-
 	/* Create sysfs interfaces */
 	err = create_core_attrs(tdata, pdata->hwmon_dev);
 	if (err)
-		goto exit_free;
+		goto err;
 
 	return 0;
-exit_free:
-	pdata->core_data[index] = NULL;
-	kfree(tdata);
-ida_free:
-	if (!pkg_flag)
-		ida_free(&pdata->ida, index - BASE_SYSFS_ATTR_NO);
+
+err:
+	destroy_temp_data(pdata, tdata);
 	return err;
 }
 
@@ -566,10 +591,8 @@ 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, struct temp_data *tdata)
 {
-	struct temp_data *tdata = pdata->core_data[indx];
-
 	/* if we errored on add then this is already gone */
 	if (!tdata)
 		return;
@@ -577,11 +600,7 @@ 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;
-
-	if (indx >= BASE_SYSFS_ATTR_NO)
-		ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO);
+	destroy_temp_data(pdata, tdata);
 }
 
 static int coretemp_device_add(int zoneid)
@@ -694,7 +713,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, target;
+	int target;
 
 	/* No need to tear down any interfaces for suspend */
 	if (cpuhp_tasks_frozen)
@@ -705,16 +724,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	if (!pd->hwmon_dev)
 		return 0;
 
-	for (i = BASE_SYSFS_ATTR_NO; i < MAX_CORE_DATA; i++) {
-		if (pd->core_data[i] && pd->core_data[i]->cpu_core_id == topology_core_id(cpu))
-			break;
-	}
-
-	/* Too many cores and this core is not populated, just return */
-	if (i == MAX_CORE_DATA)
-		return 0;
-
-	tdata = pd->core_data[i];
+	tdata = get_temp_data(pd, cpu);
 
 	cpumask_clear_cpu(cpu, &pd->cpumask);
 
@@ -725,7 +735,7 @@ 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, i);
+		coretemp_remove_core(pd, tdata);
 	} else if (tdata && tdata->cpu == cpu) {
 		mutex_lock(&tdata->update_lock);
 		tdata->cpu = target;
@@ -735,10 +745,10 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	/*
 	 * If all cores in this pkg are offline, remove the interface.
 	 */
-	tdata = pd->core_data[PKG_SYSFS_ATTR_NO];
+	tdata = get_temp_data(pd, -1);
 	if (cpumask_empty(&pd->cpumask)) {
 		if (tdata)
-			coretemp_remove_core(pd, PKG_SYSFS_ATTR_NO);
+			coretemp_remove_core(pd, tdata);
 		hwmon_device_unregister(pd->hwmon_dev);
 		pd->hwmon_dev = NULL;
 		return 0;
-- 
2.34.1

Re: [PATCH V2 08/11] hwmon: (coretemp) Abstract core_temp helpers

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:41PM +0800, Zhang Rui wrote:
> coretemp driver has an obscure and fragile logic for handling package
> and core temperature data.
> 
> Place the logic in newly introduced helpers for further optimizations.
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 09/11] hwmon: (coretemp) Split package temp_data and core temp_data

[Zhang Rui]

Saving package temp_data and core temp_data in one array with different
offsets is fragile.

Split them and clean up crabbed maths and macros. This also fixes a
problem that pdata->core_data[0] was never used.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index a19799a302a2..1a3b5ae0baca 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -39,11 +39,8 @@ 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		512	/* 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,
@@ -99,7 +96,8 @@ struct platform_data {
 	u16			pkg_id;
 	struct ida		ida;
 	struct cpumask		cpumask;
-	struct temp_data	*core_data[MAX_CORE_DATA];
+	struct temp_data	*pkg_data;
+	struct temp_data	*core_data[NUM_REAL_CORES];
 	struct device_attribute name_attr;
 };
 
@@ -479,31 +477,21 @@ static struct temp_data *
 init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 {
 	struct temp_data *tdata;
-	int index;
 
 	tdata = kzalloc(sizeof(struct temp_data), GFP_KERNEL);
 	if (!tdata)
 		return NULL;
 
-	/*
-	 * Get the index of tdata in pdata->core_data[]
-	 * tdata for package: pdata->core_data[1]
-	 * tdata for core: pdata->core_data[2] .. pdata->core_data[NUM_REAL_CORES + 1]
-	 */
 	if (pkg_flag) {
-		index = PKG_SYSFS_ATTR_NO;
+		pdata->pkg_data = tdata;
 	} else {
-		index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
-		if (index < 0) {
+		tdata->index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
+		if (tdata->index < 0) {
 			kfree(tdata);
 			return NULL;
 		}
-		index += BASE_SYSFS_ATTR_NO;
+		pdata->core_data[tdata->index] = tdata;
 	}
-	/* Index in pdata->core_data[] */
-	tdata->index = index;
-
-	pdata->core_data[index] = tdata;
 
 	tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
 							MSR_IA32_THERM_STATUS;
@@ -517,9 +505,12 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 
 static void destroy_temp_data(struct platform_data *pdata, struct temp_data *tdata)
 {
-	pdata->core_data[tdata->index] = NULL;
-	if (!tdata->is_pkg_data)
-		ida_free(&pdata->ida, tdata->index - BASE_SYSFS_ATTR_NO);
+	if (tdata->is_pkg_data) {
+		pdata->pkg_data = NULL;
+	} else {
+		pdata->core_data[tdata->index] = NULL;
+		ida_free(&pdata->ida, tdata->index);
+	}
 	kfree(tdata);
 }
 
@@ -529,9 +520,9 @@ static struct temp_data *get_temp_data(struct platform_data *pdata, int cpu)
 
 	/* cpu < 0 means get pkg temp_data */
 	if (cpu < 0)
-		return pdata->core_data[PKG_SYSFS_ATTR_NO];
+		return pdata->pkg_data;
 
-	for (i = BASE_SYSFS_ATTR_NO; i < MAX_CORE_DATA; i++) {
+	for (i = 0; i < NUM_REAL_CORES; i++) {
 		if (pdata->core_data[i] &&
 		    pdata->core_data[i]->cpu_core_id == topology_core_id(cpu))
 			return pdata->core_data[i];
-- 
2.34.1

Re: [PATCH V2 09/11] hwmon: (coretemp) Split package temp_data and core temp_data

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:42PM +0800, Zhang Rui wrote:
> Saving package temp_data and core temp_data in one array with different
> offsets is fragile.
> 
> Split them and clean up crabbed maths and macros. This also fixes a
> problem that pdata->core_data[0] was never used.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 10/11] hwmon: (coretemp) Remove redundant temp_data->is_pkg_data

[Zhang Rui]

temp_data->index saves the index in pdata->core_data[]. It is not used
by package temp_data.

Use temp_data->index as the indicator of package temp_data and remove
redundant temp_data->is_pkg_data.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 1a3b5ae0baca..e548f2145449 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -70,19 +70,16 @@ enum coretemp_attr_index {
  * @status_reg: One of IA32_THERM_STATUS or IA32_PACKAGE_THERM_STATUS,
  *		from where the temperature values should be read.
  * @attr_size:  Total number of pre-core attrs displayed in the sysfs.
- * @is_pkg_data: If this is 1, the temp_data holds pkgtemp data.
- *		Otherwise, temp_data holds coretemp data.
  */
 struct temp_data {
 	int temp;
 	int tjmax;
 	unsigned long last_updated;
 	unsigned int cpu;
-	unsigned int index;
+	int index;
 	u32 cpu_core_id;
 	u32 status_reg;
 	int attr_size;
-	bool is_pkg_data;
 	struct device_attribute sd_attrs[TOTAL_ATTRS];
 	char attr_name[TOTAL_ATTRS][CORETEMP_NAME_LENGTH];
 	struct attribute *attrs[TOTAL_ATTRS + 1];
@@ -149,6 +146,11 @@ static const struct tjmax_model tjmax_model_table[] = {
 				 */
 };
 
+static bool is_pkg_temp_data(struct temp_data *tdata)
+{
+	return tdata->index < 0;
+}
+
 static int adjust_tjmax(struct cpuinfo_x86 *c, u32 id, struct device *dev)
 {
 	/* The 100C is default for both mobile and non mobile CPUs */
@@ -341,7 +343,7 @@ static ssize_t show_label(struct device *dev,
 	struct platform_data *pdata = dev_get_drvdata(dev);
 	struct temp_data *tdata = container_of(devattr, struct temp_data, sd_attrs[ATTR_LABEL]);
 
-	if (tdata->is_pkg_data)
+	if (is_pkg_temp_data(tdata))
 		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
 
 	return sprintf(buf, "Core %u\n", tdata->cpu_core_id);
@@ -433,7 +435,7 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev)
 		 * The attr number is always core id + 2
 		 * The Pkgtemp will always show up as temp1_*, if available
 		 */
-		int attr_no = tdata->is_pkg_data ? 1 : tdata->cpu_core_id + 2;
+		int attr_no = is_pkg_temp_data(tdata) ? 1 : tdata->cpu_core_id + 2;
 
 		snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH,
 			 "temp%d_%s", attr_no, suffixes[i]);
@@ -484,6 +486,8 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 
 	if (pkg_flag) {
 		pdata->pkg_data = tdata;
+		/* Use tdata->index as indicator of package temp data */
+		tdata->index = -1;
 	} else {
 		tdata->index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
 		if (tdata->index < 0) {
@@ -495,7 +499,6 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 
 	tdata->status_reg = pkg_flag ? MSR_IA32_PACKAGE_THERM_STATUS :
 							MSR_IA32_THERM_STATUS;
-	tdata->is_pkg_data = pkg_flag;
 	tdata->cpu = cpu;
 	tdata->cpu_core_id = topology_core_id(cpu);
 	tdata->attr_size = MAX_CORE_ATTRS;
@@ -505,7 +508,7 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 
 static void destroy_temp_data(struct platform_data *pdata, struct temp_data *tdata)
 {
-	if (tdata->is_pkg_data) {
+	if (is_pkg_temp_data(tdata)) {
 		pdata->pkg_data = NULL;
 	} else {
 		pdata->core_data[tdata->index] = NULL;
-- 
2.34.1

Re: [PATCH V2 10/11] hwmon: (coretemp) Remove redundant temp_data->is_pkg_data

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:43PM +0800, Zhang Rui wrote:
> temp_data->index saves the index in pdata->core_data[]. It is not used
> by package temp_data.
> 
> Use temp_data->index as the indicator of package temp_data and remove
> redundant temp_data->is_pkg_data.
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

[PATCH V2 11/11] hwmon: (coretemp) Use dynamic allocated memory for core temp_data

[Zhang Rui]

The total memory needed for saving per core temperature data depends on
the number of cores in a package. Using static allocated memory wastes
memories on systems with low per package core count.

Improve the code to use dynamic allocated memory so that it can be
improved further when per package core count information becomes
available.

No functional change intended.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index e548f2145449..27c98c7faf32 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -91,10 +91,11 @@ struct temp_data {
 struct platform_data {
 	struct device		*hwmon_dev;
 	u16			pkg_id;
+	int			nr_cores;
 	struct ida		ida;
 	struct cpumask		cpumask;
 	struct temp_data	*pkg_data;
-	struct temp_data	*core_data[NUM_REAL_CORES];
+	struct temp_data	**core_data;
 	struct device_attribute name_attr;
 };
 
@@ -480,6 +481,20 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 {
 	struct temp_data *tdata;
 
+	if (!pdata->core_data) {
+		/*
+		 * TODO:
+		 * The information of actual possible cores in a package is broken for now.
+		 * Will replace hardcoded NUM_REAL_CORES with actual per package core count
+		 * when this information becomes available.
+		 */
+		pdata->nr_cores = NUM_REAL_CORES;
+		pdata->core_data = kcalloc(pdata->nr_cores, sizeof(struct temp_data *),
+						GFP_KERNEL);
+		if (!pdata->core_data)
+			return NULL;
+	}
+
 	tdata = kzalloc(sizeof(struct temp_data), GFP_KERNEL);
 	if (!tdata)
 		return NULL;
@@ -489,7 +504,7 @@ init_temp_data(struct platform_data *pdata, unsigned int cpu, int pkg_flag)
 		/* Use tdata->index as indicator of package temp data */
 		tdata->index = -1;
 	} else {
-		tdata->index = ida_alloc_max(&pdata->ida, NUM_REAL_CORES - 1, GFP_KERNEL);
+		tdata->index = ida_alloc_max(&pdata->ida, pdata->nr_cores - 1, GFP_KERNEL);
 		if (tdata->index < 0) {
 			kfree(tdata);
 			return NULL;
@@ -510,6 +525,9 @@ static void destroy_temp_data(struct platform_data *pdata, struct temp_data *tda
 {
 	if (is_pkg_temp_data(tdata)) {
 		pdata->pkg_data = NULL;
+		kfree(pdata->core_data);
+		pdata->core_data = NULL;
+		pdata->nr_cores = 0;
 	} else {
 		pdata->core_data[tdata->index] = NULL;
 		ida_free(&pdata->ida, tdata->index);
@@ -525,7 +543,7 @@ static struct temp_data *get_temp_data(struct platform_data *pdata, int cpu)
 	if (cpu < 0)
 		return pdata->pkg_data;
 
-	for (i = 0; i < NUM_REAL_CORES; i++) {
+	for (i = 0; i < pdata->nr_cores; i++) {
 		if (pdata->core_data[i] &&
 		    pdata->core_data[i]->cpu_core_id == topology_core_id(cpu))
 			return pdata->core_data[i];
-- 
2.34.1

Re: [PATCH V2 11/11] hwmon: (coretemp) Use dynamic allocated memory for core temp_data

[Guenter Roeck]

On Fri, Feb 02, 2024 at 05:21:44PM +0800, Zhang Rui wrote:
> The total memory needed for saving per core temperature data depends on
> the number of cores in a package. Using static allocated memory wastes
> memories on systems with low per package core count.
> 
> Improve the code to use dynamic allocated memory so that it can be
> improved further when per package core count information becomes
> available.
> 
> No functional change intended.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Applied.

Thanks,
Guenter

Re: [PATCH V2 00/11] hwmon: (coretemp) Fixes, improvements and support for large core count

[Guenter Roeck]

On 2/2/24 01:21, Zhang Rui wrote:
> Patch 1/11 is a bug fix that should be considered as stable material.
> Patch 2/11 fixes a user visible sysfs attribute name change.
> Patch 3/11 is a quick fix to allow coretemp driver to probe more than
>             128 cores.
> Patch 4/11 - 10/11 are a series of improvements aim to simplify the
>             code logic and remove unnecessary macros, variables and
>             structure fields, and make it easier for patch 11/11.
> Patch 11/11 converts coretemp driver to use dynamic memory allocation
>             for core temp_data, so that it is easy to remove the
>             hardcoded core count limitation when _num_cores_per_package
>             become available and reliable, which is WIP in
>             https://lore.kernel.org/all/20240118123127.055361964@linutronix.de/
> 
> I can split the first three patches into a separate patch set if needed.
> 
> Patch seris V1 has been posted at
> https://lore.kernel.org/all/20231127131651.476795-1-rui.zhang@intel.com/
> 

Change log ?

Guenter

> thanks,
> rui
> 
> ----------------------------------------------------------------
> Zhang Rui (11):
>        hwmon: (coretemp) Fix out-of-bounds memory access in create_core_data()
>        hwmon: (coretemp) Fix bogus core to attr mapping
>        hwmon: (coretemp) Enlarge per package core count limit
>        hwmon: (coretemp) Introduce enum for attr index
>        hwmon: (coretemp) Remove unnecessary dependency of array index
>        hwmon: (coretemp) Replace sensor_device_attribute with device_attribute
>        hwmon: (coretemp) Remove redundant pdata->cpu_map[]
>        hwmon: (coretemp) Abstract core_temp helpers
>        hwmon: (coretemp) Split package temp_data and core temp_data
>        hwmon: (coretemp) Remove redundant temp_data->is_pkg_data
>        hwmon: (coretemp) Use dynamic allocated memory for core temp_data
> 
>   drivers/hwmon/coretemp.c | 219 ++++++++++++++++++++++++++---------------------
>   1 file changed, 120 insertions(+), 99 deletions(-)
> 
> 

Re: [PATCH V2 00/11] hwmon: (coretemp) Fixes, improvements and support for large core count

[Zhang, Rui]

On Fri, 2024-02-02 at 10:15 -0800, Guenter Roeck wrote:
> On 2/2/24 01:21, Zhang Rui wrote:
> > Patch 1/11 is a bug fix that should be considered as stable
> > material.
> > Patch 2/11 fixes a user visible sysfs attribute name change.
> > Patch 3/11 is a quick fix to allow coretemp driver to probe more
> > than
> >             128 cores.
> > Patch 4/11 - 10/11 are a series of improvements aim to simplify the
> >             code logic and remove unnecessary macros, variables and
> >             structure fields, and make it easier for patch 11/11.
> > Patch 11/11 converts coretemp driver to use dynamic memory
> > allocation
> >             for core temp_data, so that it is easy to remove the
> >             hardcoded core count limitation when
> > _num_cores_per_package
> >             become available and reliable, which is WIP in
> >            
> > https://lore.kernel.org/all/20240118123127.055361964@linutronix.de/
> > 
> > I can split the first three patches into a separate patch set if
> > needed.
> > 
> > Patch seris V1 has been posted at
> > https://lore.kernel.org/all/20231127131651.476795-1-rui.zhang@intel.com/
> > 
> 
> Change log ?

Changes since V1:
 - Add two new fixes for issues found during code rewrite.
 - Reorder enum coretemp_attr_index to better represent the 
   relationship between each element and keep their value in ascending
   order. Suggested by Ashok.
 - Replace sensor_device_attribute with sensor_device_attribute.
   Suggested by Ashok.
 - Use a dynamic allocated array to save the per core temperature info
   instead of a list. Suggested by Guenter.
 - Simplify the logic for handling pkg temp_data and core temp_data
   and remove unused marcos.

thanks,
rui

> 
> Guenter
> 
> > thanks,
> > rui
> > 
> > ----------------------------------------------------------------
> > Zhang Rui (11):
> >        hwmon: (coretemp) Fix out-of-bounds memory access in
> > create_core_data()
> >        hwmon: (coretemp) Fix bogus core to attr mapping
> >        hwmon: (coretemp) Enlarge per package core count limit
> >        hwmon: (coretemp) Introduce enum for attr index
> >        hwmon: (coretemp) Remove unnecessary dependency of array
> > index
> >        hwmon: (coretemp) Replace sensor_device_attribute with
> > device_attribute
> >        hwmon: (coretemp) Remove redundant pdata->cpu_map[]
> >        hwmon: (coretemp) Abstract core_temp helpers
> >        hwmon: (coretemp) Split package temp_data and core temp_data
> >        hwmon: (coretemp) Remove redundant temp_data->is_pkg_data
> >        hwmon: (coretemp) Use dynamic allocated memory for core
> > temp_data
> > 
> >   drivers/hwmon/coretemp.c | 219 ++++++++++++++++++++++++++--------
> > -------------
> >   1 file changed, 120 insertions(+), 99 deletions(-)
> > 
> > 
> 
>