Skip to contentSkip to conversation

powercap/intel_rapl: fix and tidy up error handling

PATCH In review Jacob Pan 2 participants 4 messages Nov 23, 2016 – Nov 28, 2016 Power management core

lore
Share & tools
lore

Jacob Pan

author
#0 lore raw

PATCH 1/1 powercap/intel_rapl: fix and tidy up error handling

Commit e1399ba20eee ("powercap / RAPL: handle missing MSRs") added contraint_to_pl() function to return index into an array. But it can potentially return -EINVAL if powercap layer sends an out of range constraint ID. This patch adds sanity check.

Unnecessary RAPL domain pointer check is removed since it must be initialized before calling rapl_unit_xlate().

---
 drivers/powercap/intel_rapl.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)
drivers/powercap/intel_rapl.c +24 -1
index 65ed88a..f6937d0 100644
@@ -429,6 +429,7 @@ static int contraint_to_pl(struct rapl_domain *rd, int cid)
 			return i;
 		}
 	}
+	pr_err("Cannot find matching power limit for constraint %d\n", cid);
 
 	return -EINVAL;
 }
@@ -444,6 +445,10 @@ static int set_power_limit(struct powercap_zone *power_zone, int cid,
 	get_online_cpus();
 	rd = power_zone_to_rapl_domain(power_zone);
 	id = contraint_to_pl(rd, cid);
+	if (id < 0) {
+		ret = id;
+		goto set_exit;
+	}
 
 	rp = rd->rp;
 
@@ -483,6 +488,11 @@ static int get_current_power_limit(struct powercap_zone *power_zone, int cid,
 	get_online_cpus();
 	rd = power_zone_to_rapl_domain(power_zone);
 	id = contraint_to_pl(rd, cid);
+	if (id < 0) {
+		ret = id;
+		goto get_exit;
+	}
+
 	switch (rd->rpl[id].prim_id) {
 	case PL1_ENABLE:
 		prim = POWER_LIMIT1;
@@ -499,6 +509,7 @@ static int get_current_power_limit(struct powercap_zone *power_zone, int cid,
 	else
 		*data = val;
 
+get_exit:
 	put_online_cpus();
 
 	return ret;
@@ -514,6 +525,10 @@ static int set_time_window(struct powercap_zone *power_zone, int cid,
 	get_online_cpus();
 	rd = power_zone_to_rapl_domain(power_zone);
 	id = contraint_to_pl(rd, cid);
+	if (id < 0) {
+		ret = id;
+		goto set_time_exit;
+	}
 
 	switch (rd->rpl[id].prim_id) {
 	case PL1_ENABLE:
@@ -525,6 +540,8 @@ static int set_time_window(struct powercap_zone *power_zone, int cid,
 	default:
 		ret = -EINVAL;
 	}
+
+set_time_exit:
 	put_online_cpus();
 	return ret;
 }
@@ -539,6 +556,10 @@ static int get_time_window(struct powercap_zone *power_zone, int cid, u64 *data)
 	get_online_cpus();
 	rd = power_zone_to_rapl_domain(power_zone);
 	id = contraint_to_pl(rd, cid);
+	if (id < 0) {
+		ret = id;
+		goto get_time_exit;
+	}
 
 	switch (rd->rpl[id].prim_id) {
 	case PL1_ENABLE:
@@ -553,6 +574,8 @@ static int get_time_window(struct powercap_zone *power_zone, int cid, u64 *data)
 	}
 	if (!ret)
 		*data = val;
+
+get_time_exit:
 	put_online_cpus();
 
 	return ret;
@@ -694,7 +717,7 @@ static u64 rapl_unit_xlate(struct rapl_domain *rd, enum unit_type type,
 	case ENERGY_UNIT:
 		scale = ENERGY_UNIT_SCALE;
 		/* per domain unit takes precedence */
-		if (rd && rd->domain_energy_unit)
+		if (rd->domain_energy_unit)
 			units = rd->domain_energy_unit;
 		else
 			units = rp->energy_unit;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 10 lines · Jacob Pan

Commit e1399ba20eee ("powercap / RAPL: handle missing MSRs") added contraint_to_pl() function to return index into an array. But it can potentially return -EINVAL if powercap layer sends an out of range constraint ID. This patch adds sanity check.

Unnecessary RAPL domain pointer check is removed since it must be initialized before calling rapl_unit_xlate().

Does this depend on the Thomas' series only?

Also, it looks like a Fixes: tag should be present here.

Quoted 2 lines

Thanks,
Rafael

author
#2 lore raw
Quoted 7 lines · On Thu, 24 Nov 2016 22:06:59 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 5 lines · Jacob Pan

Commit e1399ba20eee ("powercap / RAPL: handle missing MSRs") added contraint_to_pl() function to return index into an array. But it can potentially return -EINVAL if powercap layer sends an out of range constraint ID. This patch adds sanity check.

Quoted 8 lines
Quoted 5 lines

Unnecessary RAPL domain pointer check is removed since it must be initialized before calling rapl_unit_xlate().

Does this depend on the Thomas' series only?

yes, it is on top of Thomas' series.

Quoted 2 lines

Also, it looks like a Fixes: tag should be present here.

OK. will add that.

Quoted 5 lines
Quoted 2 lines

Thanks,
Rafael

[Jacob Pan]

author
#3 lore raw
Quoted 2 lines · On Mon, 28 Nov 2016 09:31:03 -0800 Jacob Pan <[email protected]>…
Quoted 2 lines

Also, it looks like a Fixes: tag should be present here.

I just resent the patch with Fixes: tag.

Thanks,

Jacob