Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Thu, 30 Nov 2023 12:49:42 +0000
From:   Cristian Marussi <cristian.marussi@arm.com>
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Sibi Sankar <quic_sibis@quicinc.com>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        quic_mdtipton@quicinc.com, linux-arm-kernel@lists.infradead.org,
        quic_asartor@quicinc.com, quic_lingutla@quicinc.com
Subject: Re: [PATCH 2/3] firmware: arm_scmi: Fix freq/power truncation in the
 perf protocol
Message-ID: <ZWiE5nM83TZd3drT@pluto>
References: <20231129065748.19871-1-quic_sibis@quicinc.com>
 <20231129065748.19871-3-quic_sibis@quicinc.com>
 <ZWh6cuApg-sRbA2s@bogus>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZWh6cuApg-sRbA2s@bogus>
Precedence: bulk

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