| Message ID | 20231130204343.503076-1-sudeep.holla@arm.com |
|---|---|
| State | Accepted |
| Commit | 8e3c98d9187e09274fc000a7d1a77b070a42d259 |
| Headers | show |
| Series | [1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64 | expand |
On Thu, Nov 30, 2023 at 08:43:42PM +0000, Sudeep Holla wrote: > Fix the frequency truncation for all values equal to or greater 4GHz by > updating the multiplier 'mult_factor' to u64 type. It is also possible > that the multiplier itself can be greater than or equal to 2^32. So we need > to also fix the equation computing the value of the multiplier. > > Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol") > Reported-by: Sibi Sankar <quic_sibis@quicinc.com> > Closes: https://lore.kernel.org/all/20231129065748.19871-3-quic_sibis@quicinc.com/ > Cc: Cristian Marussi <cristian.marussi@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/perf.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 81dd5c5e5533..8ce449922e55 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -152,7 +152,7 @@ struct perf_dom_info { > u32 opp_count; > u32 sustained_freq_khz; > u32 sustained_perf_level; > - u32 mult_factor; > + u64 mult_factor; I have now changed this to unsigned long instead of u64 to fix the 32-bit build failure[1].
On Fri, Dec 01, 2023 at 02:39:35PM +0000, Sudeep Holla wrote: > On Thu, Nov 30, 2023 at 08:43:42PM +0000, Sudeep Holla wrote: > > Fix the frequency truncation for all values equal to or greater 4GHz by > > updating the multiplier 'mult_factor' to u64 type. It is also possible > > that the multiplier itself can be greater than or equal to 2^32. So we need > > to also fix the equation computing the value of the multiplier. > > > > Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol") > > Reported-by: Sibi Sankar <quic_sibis@quicinc.com> > > Closes: https://lore.kernel.org/all/20231129065748.19871-3-quic_sibis@quicinc.com/ > > Cc: Cristian Marussi <cristian.marussi@arm.com> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > drivers/firmware/arm_scmi/perf.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > > index 81dd5c5e5533..8ce449922e55 100644 > > --- a/drivers/firmware/arm_scmi/perf.c > > +++ b/drivers/firmware/arm_scmi/perf.c > > @@ -152,7 +152,7 @@ struct perf_dom_info { > > u32 opp_count; > > u32 sustained_freq_khz; > > u32 sustained_perf_level; > > - u32 mult_factor; > > + u64 mult_factor; > > I have now changed this to unsigned long instead of u64 to fix the 32-bit > build failure[1]. Right, I was caught a few times too by this kind of failures on v7 :D ... but this 32bit issue makes me wonder what to do in such a case... ...I mean, on 32bit if the calculated freq oveflows, there is just nothing we can do on v7 without overcomplicating the code...but I suppose it is unplausible to have such high freq on a v7... as a palliative I can only think of some sort of overflow check (only on v7) that could trigger a warning ... but it is hardly worth the effort probably.. Thanks, Cristian
On Thu, Nov 30, 2023 at 08:43:43PM +0000, Sudeep Holla wrote: > The multiplier is already promoted to u64, however the frequency > calculations done when using level indexing mode doesn't use the > multiplier computed. It instead hardcodes the multiplier value of 1000 > at all the usage sites. > > Clean that up by assigning the multiplier value of 1000 when using > the perf level indexing mode and upadte the frequency calculations to ^update > use the multiplier instead. It should fix the possible frequency > truncation for all the values greater than or equal to 4GHz. > > Fixes: 31c7c1397a33 ("firmware: arm_scmi: Add v3.2 perf level indexing mode support") > Reported-by: Sibi Sankar <quic_sibis@quicinc.com> > Closes: https://lore.kernel.org/all/20231129065748.19871-3-quic_sibis@quicinc.com/ > Cc: Cristian Marussi <cristian.marussi@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- Other than the typo, LGTM. Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian > drivers/firmware/arm_scmi/perf.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 8ce449922e55..875dcb71bb65 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -268,7 +268,8 @@ 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 > @@ -806,7 +807,7 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph, > if (!dom->level_indexing_mode) > freq = dom->opp[idx].perf * dom->mult_factor; > else > - freq = dom->opp[idx].indicative_freq * 1000; > + freq = dom->opp[idx].indicative_freq * dom->mult_factor; > > data.level = dom->opp[idx].perf; > data.freq = freq; > @@ -853,7 +854,8 @@ static int scmi_dvfs_freq_set(const struct scmi_protocol_handle *ph, u32 domain, > } else { > struct scmi_opp *opp; > > - opp = LOOKUP_BY_FREQ(dom->opps_by_freq, freq / 1000); > + opp = LOOKUP_BY_FREQ(dom->opps_by_freq, > + freq / dom->mult_factor); > if (!opp) > return -EIO; > > @@ -887,7 +889,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 = opp->indicative_freq * dom->mult_factor; > } > > return ret; > @@ -910,7 +912,7 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph, > if (!dom->level_indexing_mode) > opp_freq = opp->perf * dom->mult_factor; > else > - opp_freq = opp->indicative_freq * 1000; > + opp_freq = opp->indicative_freq * dom->mult_factor; > > if (opp_freq < *freq) > continue; > -- > 2.43.0 >
On Fri, Dec 01, 2023 at 04:17:56PM +0000, Cristian Marussi wrote: > On Fri, Dec 01, 2023 at 02:39:35PM +0000, Sudeep Holla wrote: > > On Thu, Nov 30, 2023 at 08:43:42PM +0000, Sudeep Holla wrote: > > > Fix the frequency truncation for all values equal to or greater 4GHz by > > > updating the multiplier 'mult_factor' to u64 type. It is also possible > > > that the multiplier itself can be greater than or equal to 2^32. So we need > > > to also fix the equation computing the value of the multiplier. > > > > > > Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol") > > > Reported-by: Sibi Sankar <quic_sibis@quicinc.com> > > > Closes: https://lore.kernel.org/all/20231129065748.19871-3-quic_sibis@quicinc.com/ > > > Cc: Cristian Marussi <cristian.marussi@arm.com> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > drivers/firmware/arm_scmi/perf.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > > > index 81dd5c5e5533..8ce449922e55 100644 > > > --- a/drivers/firmware/arm_scmi/perf.c > > > +++ b/drivers/firmware/arm_scmi/perf.c > > > @@ -152,7 +152,7 @@ struct perf_dom_info { > > > u32 opp_count; > > > u32 sustained_freq_khz; > > > u32 sustained_perf_level; > > > - u32 mult_factor; > > > + u64 mult_factor; > > > > I have now changed this to unsigned long instead of u64 to fix the 32-bit > > build failure[1]. > > Right, I was caught a few times too by this kind of failures on v7 :D > 😄 > ... but this 32bit issue makes me wonder what to do in such a case... > Same here, but the frequency calculations are also unsigned long in higher layers, so I don't see any point in making it u64(also 32-bit doesn't support 32bit value to be divided by a 64bit value which adds unnecessary complications here). > ...I mean, on 32bit if the calculated freq oveflows, there is just > nothing we can do on v7 without overcomplicating the code...but I suppose > it is unplausible to have such high freq on a v7... Yes this is exactly the argument I made myself and got convinced to keep it unsigned long(KISS approach) unless we need it on v7. > as a palliative I can only think of some sort of overflow check (only on v7) > that could trigger a warning ... but it is hardly worth the effort > probably.. > Not sure myself. -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index 81dd5c5e5533..8ce449922e55 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -152,7 +152,7 @@ struct perf_dom_info { u32 opp_count; u32 sustained_freq_khz; u32 sustained_perf_level; - u32 mult_factor; + u64 mult_factor; struct scmi_perf_domain_info info; struct scmi_opp opp[MAX_OPPS]; struct scmi_fc_info *fc_info; @@ -273,8 +273,8 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph, 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); }
Fix the frequency truncation for all values equal to or greater 4GHz by updating the multiplier 'mult_factor' to u64 type. It is also possible that the multiplier itself can be greater than or equal to 2^32. So we need to also fix the equation computing the value of the multiplier. Fixes: a9e3fbfaa0ff ("firmware: arm_scmi: add initial support for performance protocol") Reported-by: Sibi Sankar <quic_sibis@quicinc.com> Closes: https://lore.kernel.org/all/20231129065748.19871-3-quic_sibis@quicinc.com/ Cc: Cristian Marussi <cristian.marussi@arm.com> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/perf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.43.0