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

[Cristian Marussi]

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