linux-kernel - Re: [PATCH 2/2] cpufreq: scmi: Update Energy Model with allowed performance limits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZjIK0-7ixgp0WQ4x@pluto>
Date: Wed, 1 May 2024 10:26:43 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Lukasz Luba <lukasz.luba@....com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	dietmar.eggemann@....com, linux-arm-kernel@...ts.infradead.org,
	sudeep.holla@....com, linux-samsung-soc@...r.kernel.org,
	rafael@...nel.org, viresh.kumar@...aro.org, quic_sibis@...cinc.com
Subject: Re: [PATCH 2/2] cpufreq: scmi: Update Energy Model with allowed
 performance limits

On Wed, Apr 03, 2024 at 05:23:15PM +0100, Lukasz Luba wrote:
> The Energy Model (EM) supports performance limits updates. Use the SCMI
> notifications to get information from FW about allowed frequency scope for
> the CPUs.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index d946b7a082584..90c8448578cb1 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -185,12 +185,25 @@ static int scmi_limit_notify_cb(struct notifier_block *nb, unsigned long event,
>  {
>  	struct scmi_data *priv = container_of(nb, struct scmi_data, limit_notify_nb);
>  	struct scmi_perf_limits_report *limit_notify = data;
> +	unsigned int limit_freq_max_khz, limit_freq_min_khz;
>  	struct cpufreq_policy *policy = priv->policy;
> -	unsigned int limit_freq_khz;
> +	struct em_perf_domain *pd;
> +	int ret;
> +
> +	limit_freq_max_khz = limit_notify->range_max_freq / HZ_PER_KHZ;
> +	limit_freq_min_khz = limit_notify->range_min_freq / HZ_PER_KHZ;

Note that these values could be zeroed if the notification is good but
the range_min/range_max values could NOT be mapped to a frequency
equivalent (due to some FW errors).

I would probably have to add a warn about this error in the core SCMI
notification path (or drop the notif as a whole); if not here you could
end-up just setting max/min to 0 if the fw has messed up the
notification range_min/range_max.

Or is it just that, especially max_feq = 0 is NOT plausible value and you
will need anyway to check it here ?

>  
> -	limit_freq_khz = limit_notify->range_max_freq / HZ_PER_KHZ;
> +	pd = em_cpu_get(policy->cpu);
> +	if (pd) {
> +		ret = em_update_performance_limits(pd, limit_freq_min_khz,
> +						   limit_freq_max_khz);
> +		if (ret)
> +			dev_warn(priv->cpu_dev,
> +				 "EM perf limits update failed\n");
> +	}
>  
> -	policy->max = clamp(limit_freq_khz, policy->cpuinfo.min_freq, policy->cpuinfo.max_freq);
> +	policy->max = clamp(limit_freq_max_khz, policy->cpuinfo.min_freq,
> +			    policy->cpuinfo.max_freq);

FWIW, regarding the SCMI bits.

LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@....com>

Thanks,
Cristian