| Message ID | 20231129065748.19871-3-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Miscellaneous fixes and opp count increase | expand |
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;
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.
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 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 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 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
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;
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(-)