hwmon: (coretemp) Fixes, improvements and support for large core count

PATCH Applied Zhang Rui 2 participants 25 messages Feb 2, 2024 – Feb 4, 2024

Subsystems (2)

Hardware monitoring Coretemp hardware monitoring driver

lore

Share & tools

lore

Zhang Rui

author

2024-02-02 09:21 UTC #0 lore raw

v2 cover hwmon: (coretemp) Fixes, improvements and support for large core count

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/[email protected]/

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/[email protected]/

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(-)

Zhang Rui

author

2024-02-02 09:21 UTC #1 lore raw

PATCH v2 1/11 hwmon: (coretemp) Fix out-of-bounds memory access

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 <[email protected]>

Cc <[email protected]>

---
 drivers/hwmon/coretemp.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/hwmon/coretemp.c +2 -6

index ba82d1e79c13..e78c76919111 100644
@@ -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;

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-03 15:16 UTC #2 lore raw

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

Quoted 7 lines · 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 <[email protected]>
Cc <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #3 lore raw

PATCH v2 2/11 hwmon: (coretemp) Fix bogus core_id to attr name mapping

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

drivers/hwmon/coretemp.c +19 ~~-13~~

index e78c76919111..95f4c0b00b2d 100644
@@ -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;
 }

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-03 15:16 UTC #4 lore raw

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

Quoted 34 lines · 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

Quoted 5 lines

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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #5 lore raw

PATCH v2 3/11 hwmon: (coretemp) Enlarge per package core count limit

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hwmon/coretemp.c +1 -1

index 95f4c0b00b2d..b8fc8d1ef20d 100644
@@ -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)

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-03 15:17 UTC #6 lore raw

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

Quoted 11 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #7 lore raw

PATCH v2 4/11 hwmon: (coretemp) Introduce enum for attr index

In reply to Zhang Rui

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

No functional change.

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/hwmon/coretemp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/hwmon/coretemp.c +10 -2

index b8fc8d1ef20d..32f99cf6308b 100644
@@ -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))

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:45 UTC #8 lore raw

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

Quoted 6 lines · Zhang Rui

Introduce enum coretemp_attr_index to better describe the index of each sensor attribute.
No functional change.
Signed-off-by Zhang Rui <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #9 lore raw

PATCH v2 5/11 hwmon: (coretemp) Remove unnecessary dependency of array index

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/hwmon/coretemp.c +7 ~~-13~~

index 32f99cf6308b..9a7bfc046c72 100644
@@ -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;

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:46 UTC #10 lore raw

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

Quoted 9 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #11 lore raw

PATCH v2 6/11 hwmon: (coretemp) Replace sensor_device_attribute with device_attribute

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

drivers/hwmon/coretemp.c +12 ~~-16~~

index 9a7bfc046c72..cdd1e069d5c1 100644
@@ -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);

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:46 UTC #12 lore raw

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

Quoted 6 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #13 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

drivers/hwmon/coretemp.c +6 ~~-10~~

index cdd1e069d5c1..29ee8e0c0fe9 100644
@@ -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;

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:47 UTC #14 lore raw

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

Quoted 10 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Quoted 65 lines

---
 drivers/hwmon/coretemp.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

drivers/hwmon/coretemp.c +6 ~~-10~~

index cdd1e069d5c1..29ee8e0c0fe9 100644
@@ -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;

Zhang Rui

author

2024-02-02 09:22 UTC #15 lore raw

PATCH v2 8/11 hwmon: (coretemp) Abstract core_temp helpers

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 118 +++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 54 deletions(-)

drivers/hwmon/coretemp.c +64 ~~-54~~

index 29ee8e0c0fe9..a19799a302a2 100644
@@ -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;

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:48 UTC #16 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #17 lore raw

PATCH v2 9/11 hwmon: (coretemp) Split package temp_data and core temp_data

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

drivers/hwmon/coretemp.c +14 ~~-23~~

index a19799a302a2..1a3b5ae0baca 100644
@@ -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];

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:48 UTC #18 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #19 lore raw

PATCH v2 10/11 hwmon: (coretemp) Remove redundant temp_data->is_pkg_data

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

drivers/hwmon/coretemp.c +11 -8

index 1a3b5ae0baca..e548f2145449 100644
@@ -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;

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:49 UTC #20 lore raw

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

Quoted 9 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Zhang Rui

author

2024-02-02 09:22 UTC #21 lore raw

PATCH v2 11/11 hwmon: (coretemp) Use dynamic allocated memory for core temp_data

In reply to 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 <[email protected]>

---
 drivers/hwmon/coretemp.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/hwmon/coretemp.c +21 -3

index e548f2145449..27c98c7faf32 100644
@@ -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];

Signature · 1 line

2.34.1

Guenter Roeck

maintainer Applied

2024-02-04 14:50 UTC #22 lore raw

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

Quoted 11 lines · 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 <[email protected]>

Applied.

Thanks,
Guenter

Guenter Roeck

maintainer

2024-02-02 18:15 UTC #23 lore raw

In reply to Zhang Rui

Quoted 13 lines · 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/[email protected]/

Quoted 5 lines

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/[email protected]/

Change log ?

Guenter

Quoted 21 lines

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(-)

Zhang, Rui

author

2024-02-03 05:40 UTC #24 lore raw

Quoted 19 lines · Guenter Roeck

Quoted 18 lines · 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/[email protected]/

Quoted 8 lines

Quoted 6 lines
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/[email protected]/
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

Quoted 31 lines

Guenter
Quoted 26 lines
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(-)