linux-kernel - Re: [PATCH 2/3] firmware: arm_scmi: Fix freq/power truncation in the perf protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Nov 2023 12:05:06 +0000
From:   Sudeep Holla <sudeep.holla@....com>
To:     Sibi Sankar <quic_sibis@...cinc.com>
Cc:     <cristian.marussi@....com>, <linux-arm-msm@...r.kernel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        <linux-kernel@...r.kernel.org>, <quic_mdtipton@...cinc.com>,
        <linux-arm-kernel@...ts.infradead.org>, <quic_asartor@...cinc.com>,
        <quic_lingutla@...cinc.com>
Subject: Re: [PATCH 2/3] firmware: arm_scmi: Fix freq/power truncation in the
 perf protocol

On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote:
> Fix frequency and power truncation seen in the performance protocol by
> casting it with the correct type.
>

While I always remembered to handle this when reviewing the spec, seem to
have forgotten when it came to handling in the implementation :(. Thanks
for spotting this.

However I don't like the ugly type casting. I think we can do better. Also
looking at the code around the recently added level index mode, I think we
can simplify things like below patch.

Cristian,
What do you think ?

Regards,
Sudeep

-->8

 drivers/firmware/arm_scmi/perf.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index a648521e04a3..2e828b29efab 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -268,13 +268,14 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 		dom_info->sustained_perf_level =
 					le32_to_cpu(attr->sustained_perf_level);
 		if (!dom_info->sustained_freq_khz ||
-		    !dom_info->sustained_perf_level)
+		    !dom_info->sustained_perf_level ||
+		    dom_info->level_indexing_mode)
 			/* CPUFreq converts to kHz, hence default 1000 */
 			dom_info->mult_factor =	1000;
 		else
 			dom_info->mult_factor =
-					(dom_info->sustained_freq_khz * 1000) /
-					dom_info->sustained_perf_level;
+					(dom_info->sustained_freq_khz * 1000UL)
+					/ dom_info->sustained_perf_level;
 		strscpy(dom_info->info.name, attr->name,
 			SCMI_SHORT_NAME_MAX_SIZE);
 	}
@@ -804,9 +805,10 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,

 	for (idx = 0; idx < dom->opp_count; idx++) {
 		if (!dom->level_indexing_mode)
-			freq = dom->opp[idx].perf * dom->mult_factor;
+			freq = dom->opp[idx].perf;
 		else
-			freq = dom->opp[idx].indicative_freq * 1000;
+			freq = dom->opp[idx].indicative_freq;
+		freq *= dom->mult_factor;

 		data.level = dom->opp[idx].perf;
 		data.freq = freq;
@@ -879,7 +881,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
 		return ret;

 	if (!dom->level_indexing_mode) {
-		*freq = level * dom->mult_factor;
+		*freq = level;
 	} else {
 		struct scmi_opp *opp;

@@ -887,8 +889,9 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
 		if (!opp)
 			return -EIO;

-		*freq = opp->indicative_freq * 1000;
+		*freq = opp->indicative_freq;
 	}
+	freq *= dom->mult_factor;

 	return ret;
 }
@@ -908,9 +911,10 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph,

 	for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) {
 		if (!dom->level_indexing_mode)
-			opp_freq = opp->perf * dom->mult_factor;
+			opp_freq = opp->perf;
 		else
-			opp_freq = opp->indicative_freq * 1000;
+			opp_freq = opp->indicative_freq;
+		opp_freq *= dom->mult_factor;

 		if (opp_freq < *freq)
 			continue;