linux-kernel - Re: [PATCH 1/2] cpufreq: intel_pstate: Avoid missing HWP max updates in passive mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201023061018.euom3sfxw7uvtmya@vireshk-i7>
Date:   Fri, 23 Oct 2020 11:40:18 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH 1/2] cpufreq: intel_pstate: Avoid missing HWP max updates
 in passive mode

On 22-10-20, 13:57, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2182,6 +2182,9 @@ int __cpufreq_driver_target(struct cpufr
>  	pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n",
>  		 policy->cpu, target_freq, relation, old_target_freq);
>  
> +	if (cpufreq_driver->target)
> +		return cpufreq_driver->target(policy, target_freq, relation);
> +
>  	/*
>  	 * This might look like a redundant call as we are checking it again
>  	 * after finding index. But it is left intentionally for cases where
> @@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufr
>  	/* Save last value to restore later on errors */
>  	policy->restore_freq = policy->cur;
>  
> -	if (cpufreq_driver->target)
> -		return cpufreq_driver->target(policy, target_freq, relation);
> -
>  	if (!cpufreq_driver->target_index)
>  		return -EINVAL;

>From what I understood, you want your driver to get notified about
policy->min/max changes and right now they are making it work from
within the target() callback. Your commit log talks about policy->max
and powersave combination, I think the same will be true in case of
policy->min and performance ? And also with any other governor (like
schedutil) if the target_freq doesn't change for a while.

And IMHO, this change is more like a band-aid which is going to remove
the check of target != cur for all target() type drivers (which aren't
many) and it feels like a penalty on them (which is also there for
intel-cpufreq without hwp), and that we will get into the same problem
for target_index() drivers as well if they want to do something
similar in future, i.e. skip checking for same-freq.

Maybe adding a new flag for the cpufreq-driver for force-updates would
be a better solution ? Which will make this very much driver
dependent.

-- 
viresh