Fix frequency and power truncation seen in the performance protocol by
casting it with the correct type.
Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol")
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
drivers/firmware/arm_scmi/perf.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index a648521e04a3..3344ce3a2026 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -804,9 +804,9 @@ 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 = (unsigned long)dom->opp[idx].perf * dom->mult_factor;
else
- freq = dom->opp[idx].indicative_freq * 1000;
+ freq = (unsigned long)dom->opp[idx].indicative_freq * 1000;
data.level = dom->opp[idx].perf;
data.freq = freq;
@@ -879,7 +879,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 = (unsigned long)level * dom->mult_factor;
} else {
struct scmi_opp *opp;
@@ -887,7 +887,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
if (!opp)
return -EIO;
- *freq = opp->indicative_freq * 1000;
+ *freq = (unsigned long)opp->indicative_freq * 1000;
}
return ret;
@@ -908,9 +908,9 @@ 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 = (unsigned long)opp->perf * dom->mult_factor;
else
- opp_freq = opp->indicative_freq * 1000;
+ opp_freq = (unsigned long)opp->indicative_freq * 1000;
if (opp_freq < *freq)
continue;
--
2.17.1On 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;
On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote:
> 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 ?
>
Hi
the cleanup seems nice in general to compact the mult_factor multipliers
in one place, and regarding addressing the problem of truncation without
the need of the explicit casting, should not be enough to change to
additionally also change mult_factor to be an u64 ?
Not tested so I could miss something...
Thanks,
Cristian
> 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;
>
On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: > > 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 ? > > > > Hi > > the cleanup seems nice in general to compact the mult_factor multipliers > in one place, and regarding addressing the problem of truncation without > the need of the explicit casting, should not be enough to change to > additionally also change mult_factor to be an u64 ? > I started exactly with that, but when I completed the patch, there was no explicit need for it, so dropped it again. I can bump mult_factor to be u64 but do you see any other place that would need it apart from having single statement that does multiplication and assignment ? I am exploiting the conditional based on level_indexing_mode here but I agree it may help in backporting if I make mult_factor u64. -- Regards, Sudeep
On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: > On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: > > On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: > > > 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 ? > > > > > > > Hi > > > > the cleanup seems nice in general to compact the mult_factor multipliers > > in one place, and regarding addressing the problem of truncation without > > the need of the explicit casting, should not be enough to change to > > additionally also change mult_factor to be an u64 ? > > > > I started exactly with that, but when I completed the patch, there was no > explicit need for it, so dropped it again. I can bump mult_factor to be > u64 but do you see any other place that would need it apart from having > single statement that does multiplication and assignment ? I am exploiting > the conditional based on level_indexing_mode here but I agree it may help > in backporting if I make mult_factor u64. > Ah right freq *= dom->multi_fact; does the trick..but cannot this by itself (under unplausibl conds) overflow and does not fit into a u32 mult_factor ? dom_info->mult_factor = (dom_info->sustained_freq_khz * 1000UL) / dom_info->sustained_perf_level; Thanks, Cristian
On Thu, Nov 30, 2023 at 04:25:44PM +0000, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: > > I started exactly with that, but when I completed the patch, there was no > > explicit need for it, so dropped it again. I can bump mult_factor to be > > u64 but do you see any other place that would need it apart from having > > single statement that does multiplication and assignment ? I am exploiting > > the conditional based on level_indexing_mode here but I agree it may help > > in backporting if I make mult_factor u64. > > > > Ah right > > freq *= dom->multi_fact; > > does the trick..but cannot this by itself (under unplausibl conds) > overflow and does not fit into a u32 mult_factor ? > > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) > / dom_info->sustained_perf_level; Agreed. Also thinking about backports, I think making it u64 is simple fix. I will also thinking of splitting the changes so that fixes are more appropriate. I will try to post something soonish. -- Regards, Sudeep
On 11/30/23 21:55, Cristian Marussi wrote: > On Thu, Nov 30, 2023 at 01:56:56PM +0000, Sudeep Holla wrote: >> On Thu, Nov 30, 2023 at 12:49:42PM +0000, Cristian Marussi wrote: >>> On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote: >>>> 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 ? >>>> >>> >>> Hi >>> >>> the cleanup seems nice in general to compact the mult_factor multipliers >>> in one place, and regarding addressing the problem of truncation without >>> the need of the explicit casting, should not be enough to change to >>> additionally also change mult_factor to be an u64 ? >>> >> >> I started exactly with that, but when I completed the patch, there was no >> explicit need for it, so dropped it again. I can bump mult_factor to be >> u64 but do you see any other place that would need it apart from having >> single statement that does multiplication and assignment ? I am exploiting >> the conditional based on level_indexing_mode here but I agree it may help >> in backporting if I make mult_factor u64. >> > > Ah right > > freq *= dom->multi_fact; > > does the trick..but cannot this by itself (under unplausibl conds) > overflow and does not fit into a u32 mult_factor ? > > dom_info->mult_factor = > (dom_info->sustained_freq_khz * 1000UL) wouldn't having the 1000UL ensure that we don't truncate though? Anyway will drop the patch when I re-spin the series. -Sibi > / dom_info->sustained_perf_level; > > > Thanks, > Cristian >
On Fri, Dec 01, 2023 at 01:02:25AM +0530, Sibi Sankar wrote: > > On 11/30/23 21:55, Cristian Marussi wrote: > > > > Ah right > > > > freq *= dom->multi_fact; > > > > does the trick..but cannot this by itself (under unplausibl conds) > > overflow and does not fit into a u32 mult_factor ? > > > > dom_info->mult_factor = > > (dom_info->sustained_freq_khz * 1000UL) > > wouldn't having the 1000UL ensure that we don't truncate though? Correct but the point was mult_factor itself can be >= 2^32 > Anyway will drop the patch when I re-spin the series. > Are you re-spining just to change 24 to 32 in PATCH 3/3, if so no need. I have already applied 1 and 3 here[1]. Just waiting for the builder results to confirm it -- Regards, Sudeep [1] https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi/updates
© 2016 - 2025 Red Hat, Inc.