linux-kernel - Re: [PATCH V3 2/4] cpufreq: add support for intermediate (stable) frequencies

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]

Date:	Tue, 20 May 2014 09:48:18 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Arvind Chauhan <arvind.chauhan@....com>,
	Stephen Warren <swarren@...dia.com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Russell King <linux@....linux.org.uk>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: [PATCH V3 2/4] cpufreq: add support for intermediate (stable) frequencies

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@...aro.org> wrote:

> +1.7 get_intermediate and target_intermediate
> +--------------------------------------------
> +
> +Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION unset.
> +
> +get_intermediate should return a stable intermediate frequency platform wants to
> +switch to, and target_intermediate() should set CPU to to that frequency, before
> +jumping to the frequency corresponding to 'index'. Core will take care of
> +sending notifications and driver doesn't have to handle them in
> +target_intermediate() or target_index().

Is it worth documenting that if we implement target_intermediate()
that target_index() must not fail?  That means that any failure-prone
things (like setting a regulator) should happen in target_index().


>  2. Frequency Table Helpers
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9bf12a2..f38f2f2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
>  static int __target_index(struct cpufreq_policy *policy,
>                           struct cpufreq_frequency_table *freq_table, int index)
>  {
> -       struct cpufreq_freqs freqs;
> +       struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0};
>         int retval = -EINVAL;
>         bool notify;
>
>         notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
> +       if (!notify)
> +               goto skip_notify;

I'm personally not a fan of using goto here.  All you're trying to do
is avoiding a level of indentation?  If it really matters that much
then create a sub function.  IMHO goto should generally be reserved
for error handling.


>
> -       if (notify) {
> -               freqs.old = policy->cur;
> -               freqs.new = freq_table[index].frequency;
> -               freqs.flags = 0;
> +       /* Handle switching to intermediate frequency */
> +       if (cpufreq_driver->get_intermediate) {
> +               freqs.new = cpufreq_driver->get_intermediate(policy, index);

Would it be worth it to change this to?

intermediate = 0
if (cpufreq_driver->get_intermediate)
  intermediate = cpufreq_driver->get_intermediate();
if (intermediate)

...the idea being that a driver may use an intermediate frequency for
some transitions but not for all.  For instance: on tegra if you
happen to change to the exact clock frequency of the intermediate PLL
it just stays there.  There's no need for two notifications in that
case.  There may be other systems that can optimize some transitions
to totally skip the intermediate stage (maybe you've got an
non-glitching divider somewhere so you can optimize a transition from
1.4GHz to 700MHz to go w/ no intermediate jump).


> -               pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
> +               pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n",
>                                 __func__, policy->cpu, freqs.old, freqs.new);
>
>                 cpufreq_freq_transition_begin(policy, &freqs);
> +               retval = cpufreq_driver->target_intermediate(policy, freqs.new);

It feels like you want to pass in "index" here too, just in case.  A
driver may need to make decisions about other clocks based on the
eventual final frequency.  They could cache it themselves from the
get_intermediate() call, but that seems ugly.


> @@ -2361,7 +2384,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>             !(driver_data->setpolicy || driver_data->target_index ||
>                     driver_data->target) ||
>              (driver_data->setpolicy && (driver_data->target_index ||
> -                   driver_data->target)))
> +                   driver_data->target)) ||
> +            (!!driver_data->get_intermediate ^ !!driver_data->target_intermediate))

I'm OK with the !! trick, but using ^ here seems more confusing.  Why
not use "!="?
  (!!driver_data->get_intermediate != !!driver_data->target_intermediate))

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/