thermal: Validate new state in cur_state_store()

PATCH In review Viresh Kumar 2 participants 4 messages Oct 17, 2022 – Oct 25, 2022 Thermal

Share & tools

Viresh Kumar

author

2022-10-17 10:03 UTC #0 lore raw

cover thermal: Validate new state in cur_state_store()

In cur_state_store(), the new state of the cooling device is received from user-space and is not validated by the thermal core but the same is left for the individual drivers to take care of. Apart from duplicating the code it leaves possibility for introducing bugs where a driver may not do it right.

Lets make the thermal core check the new state itself and store the max value in the cooling device structure.

Once this is merged, I will update the thermal drivers to stop checking for the same.

Based of v6.1-rc1.

Viresh Kumar (2):
thermal: Validate new state in cur_state_store()
thermal: sysfs: Reuse cdev->max_state

 drivers/thermal/gov_fair_share.c |  6 +-----
 drivers/thermal/thermal_core.c   | 15 ++++++-------
 drivers/thermal/thermal_sysfs.c  | 37 +++++++++++++-------------------
 include/linux/thermal.h          |  1 +
 4 files changed, 24 insertions(+), 35 deletions(-)

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-10-17 10:03 UTC #1 lore raw

PATCH 1/2 thermal: Validate new state in cur_state_store()

Lets make the thermal core check the new state itself and store the max value in the cooling device structure.

Link https://lore.kernel.org/all/Y0ltRJRjO7AkawvE@kili/ open here

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/thermal/gov_fair_share.c |  6 +-----
 drivers/thermal/thermal_core.c   | 15 +++++++--------
 drivers/thermal/thermal_sysfs.c  | 11 +++++------
 include/linux/thermal.h          |  1 +
 4 files changed, 14 insertions(+), 19 deletions(-)

drivers/thermal/gov_fair_share.c +1 -5

index a4ee4661e9cc..1cfeac16e7ac 100644
@@ -49,11 +49,7 @@ static int get_trip_level(struct thermal_zone_device *tz)
 static long get_target_state(struct thermal_zone_device *tz,
 		struct thermal_cooling_device *cdev, int percentage, int level)
 {
-	unsigned long max_state;
-
-	cdev->ops->get_max_state(cdev, &max_state);
-
-	return (long)(percentage * level * max_state) / (100 * tz->num_trips);
+	return (long)(percentage * level * cdev->max_state) / (100 * tz->num_trips);
 }
 
 /**

drivers/thermal/thermal_core.c +7 -8

index 117eeaf7dd24..08de59369e94 100644
@@ -603,8 +603,7 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	struct thermal_instance *pos;
 	struct thermal_zone_device *pos1;
 	struct thermal_cooling_device *pos2;
-	unsigned long max_state;
-	int result, ret;
+	int result;
 
 	if (trip >= tz->num_trips || trip < 0)
 		return -EINVAL;

@@ -621,15 +620,11 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
 	if (tz != pos1 || cdev != pos2)
 		return -EINVAL;
 
-	ret = cdev->ops->get_max_state(cdev, &max_state);
-	if (ret)
-		return ret;
-
 	/* lower default 0, upper default max_state */
 	lower = lower == THERMAL_NO_LIMIT ? 0 : lower;
-	upper = upper == THERMAL_NO_LIMIT ? max_state : upper;
+	upper = upper == THERMAL_NO_LIMIT ? cdev->max_state : upper;
 
-	if (lower > upper || upper > max_state)
+	if (lower > upper || upper > cdev->max_state)
 		return -EINVAL;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);

@@ -900,6 +895,10 @@ __thermal_cooling_device_register(struct device_node *np,
 	cdev->updated = false;
 	cdev->device.class = &thermal_class;
 	cdev->devdata = devdata;
+
+	if (cdev->ops->get_max_state(cdev, &cdev->max_state))
+		goto out_kfree_type;
+
 	thermal_cooling_device_setup_sysfs(cdev);
 	ret = device_register(&cdev->device);
 	if (ret)

drivers/thermal/thermal_sysfs.c +5 -6

index ec495c7dff03..bd7596125461 100644
@@ -589,13 +589,8 @@ static ssize_t max_state_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
-	unsigned long state;
-	int ret;
 
-	ret = cdev->ops->get_max_state(cdev, &state);
-	if (ret)
-		return ret;
-	return sprintf(buf, "%ld\n", state);
+	return sprintf(buf, "%ld\n", cdev->max_state);
 }
 
 static ssize_t cur_state_show(struct device *dev, struct device_attribute *attr,

@@ -625,6 +620,10 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
 	if ((long)state < 0)
 		return -EINVAL;
 
+	/* Requested state should be less than max_state + 1 */
+	if (state > cdev->max_state)
+		return -EINVAL;
+
 	mutex_lock(&cdev->lock);
 
 	result = cdev->ops->set_cur_state(cdev, state);

include/linux/thermal.h +1 -0

index 9ecc128944a1..5e093602e8fc 100644
@@ -100,6 +100,7 @@ struct thermal_cooling_device_ops {
 struct thermal_cooling_device {
 	int id;
 	char *type;
+	unsigned long max_state;
 	struct device device;
 	struct device_node *np;
 	void *devdata;

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-10-17 10:03 UTC #2 lore raw

PATCH 2/2 thermal: sysfs: Reuse cdev->max_state

In reply to Viresh Kumar

Now that the cooling device structure stores the max_state value, reuse it and drop max_states from struct cooling_dev_stats.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/thermal/thermal_sysfs.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/thermal/thermal_sysfs.c +10 ~~-16~~

index bd7596125461..febf9e76c440 100644
@@ -661,7 +661,6 @@ struct cooling_dev_stats {
 	spinlock_t lock;
 	unsigned int total_trans;
 	unsigned long state;
-	unsigned long max_states;
 	ktime_t last_time;
 	ktime_t *time_in_state;
 	unsigned int *trans_table;

@@ -691,7 +690,7 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 		goto unlock;
 
 	update_time_in_state(stats);
-	stats->trans_table[stats->state * stats->max_states + new_state]++;
+	stats->trans_table[stats->state * (cdev->max_state + 1) + new_state]++;
 	stats->state = new_state;
 	stats->total_trans++;

@@ -725,7 +724,7 @@ time_in_state_ms_show(struct device *dev, struct device_attribute *attr,
 	spin_lock(&stats->lock);
 	update_time_in_state(stats);
 
-	for (i = 0; i < stats->max_states; i++) {
+	for (i = 0; i <= cdev->max_state; i++) {
 		len += sprintf(buf + len, "state%u\t%llu\n", i,
 			       ktime_to_ms(stats->time_in_state[i]));
 	}

@@ -740,7 +739,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 {
 	struct thermal_cooling_device *cdev = to_cooling_device(dev);
 	struct cooling_dev_stats *stats = cdev->stats;
-	int i, states = stats->max_states;
+	int i, states = cdev->max_state + 1;
 
 	spin_lock(&stats->lock);

@@ -749,7 +748,7 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
 	memset(stats->trans_table, 0,
 	       states * states * sizeof(*stats->trans_table));
 
-	for (i = 0; i < stats->max_states; i++)
+	for (i = 0; i < states; i++)
 		stats->time_in_state[i] = ktime_set(0, 0);
 
 	spin_unlock(&stats->lock);

@@ -767,7 +766,7 @@ static ssize_t trans_table_show(struct device *dev,
 
 	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
-	for (i = 0; i < stats->max_states; i++) {
+	for (i = 0; i <= cdev->max_state; i++) {
 		if (len >= PAGE_SIZE)
 			break;
 		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u  ", i);

@@ -777,17 +776,17 @@ static ssize_t trans_table_show(struct device *dev,
 
 	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
 
-	for (i = 0; i < stats->max_states; i++) {
+	for (i = 0; i <= cdev->max_state; i++) {
 		if (len >= PAGE_SIZE)
 			break;
 
 		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u:", i);
 
-		for (j = 0; j < stats->max_states; j++) {
+		for (j = 0; j <= cdev->max_state; j++) {
 			if (len >= PAGE_SIZE)
 				break;
 			len += snprintf(buf + len, PAGE_SIZE - len, "%8u ",
-				stats->trans_table[i * stats->max_states + j]);
+				stats->trans_table[i * (cdev->max_state + 1) + j]);
 		}
 		if (len >= PAGE_SIZE)
 			break;

@@ -823,14 +822,10 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
 {
 	const struct attribute_group *stats_attr_group = NULL;
 	struct cooling_dev_stats *stats;
-	unsigned long states;
+	/* Total number of states is highest state + 1 */
+	unsigned long states = cdev->max_state + 1;
 	int var;
 
-	if (cdev->ops->get_max_state(cdev, &states))
-		goto out;
-
-	states++; /* Total number of states is highest state + 1 */
-
 	var = sizeof(*stats);
 	var += sizeof(*stats->time_in_state) * states;
 	var += sizeof(*stats->trans_table) * states * states;

@@ -843,7 +838,6 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
 	stats->trans_table = (unsigned int *)(stats->time_in_state + states);
 	cdev->stats = stats;
 	stats->last_time = ktime_get();
-	stats->max_states = states;
 
 	spin_lock_init(&stats->lock);

Signature · 1 line

2.31.1.272.g89b43f80a514

Rafael J. Wysocki

maintainer

2022-10-25 17:00 UTC #3 lore raw

In reply to Viresh Kumar

Quoted 19 lines · Viresh Kumar

In cur_state_store(), the new state of the cooling device is received from user-space and is not validated by the thermal core but the same is left for the individual drivers to take care of. Apart from duplicating the code it leaves possibility for introducing bugs where a driver may not do it right.
Lets make the thermal core check the new state itself and store the max value in the cooling device structure.
Once this is merged, I will update the thermal drivers to stop checking for the same.
Based of v6.1-rc1.
Viresh Kumar (2):
thermal: Validate new state in cur_state_store()
thermal: sysfs: Reuse cdev->max_state

Quoted 7 lines

 drivers/thermal/gov_fair_share.c |  6 +-----
 drivers/thermal/thermal_core.c   | 15 ++++++-------
 drivers/thermal/thermal_sysfs.c  | 37 +++++++++++++-------------------
 include/linux/thermal.h          |  1 +
 4 files changed, 24 insertions(+), 35 deletions(-)

Both patches in the series applied as 6.2 material, thanks!