[PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[Sudeep Holla]

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;
 	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);
 	}
--
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] firmware: arm_scmi: Fix possible frequency truncation when using level indexing mode

[Sudeep Holla]

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
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>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] firmware: arm_scmi: Fix possible frequency truncation when using level indexing mode

[Cristian Marussi]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[Sudeep Holla]

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].

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/all/20231201085914.4ad45eb2@canb.auug.org.au

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[Cristian Marussi]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[Sudeep Holla]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64

[Sudeep Holla]

On Thu, 30 Nov 2023 20:43:42 +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.
>

Applied to sudeep.holla/linux (for-next/scmi/fixes), thanks!

[1/2] firmware: arm_scmi: Fix frequency truncation by promoting multiplier to u64
      (Applied changing u64 to unsigned long to fix armv7 build)
      https://git.kernel.org/sudeep.holla/c/8e3c98d9187e
[2/2] firmware: arm_scmi: Fix possible frequency truncation when using level indexing mode
      https://git.kernel.org/sudeep.holla/c/77f5032e94f2
--
Regards,
Sudeep


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel