linux-kernel - Re: [PATCH V4 2/3] 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:	Fri, 23 May 2014 09:54:56 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Stephen Warren <swarren@...dotorg.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 Mailing List <linux-kernel@...r.kernel.org>,
	Arvind Chauhan <arvind.chauhan@....com>,
	Stephen Warren <swarren@...dia.com>,
	Doug Anderson <dianders@...omium.org>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

On 22 May 2014 22:07, Stephen Warren <swarren@...dotorg.org> wrote:
> It seems rather odd to set both freqs->old and freqs->new to the
> intermediate frequency upon success. It feels like it would make more
> sense to remove that final else clause above, and do the following where
> this function is called:
>
>         intermediate_freq = freqs.new;
>         if (intermediate_freq)
>                 freqs.old = intermediate_freq
>         freqs.new = freq_table[index].frequency

Looks better.

> (or even return the intermediate frequency from the function)

It can return errors as well and so I didn't tried to return frequency
from it. Though there are routines that return both error and freq
from such calls :)

> ?
>
> But I suppose isolating all the inside __target_intermediate() isn't so bad.

:)

>>  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};
>> +     unsigned int intermediate_freq = 0;
>>       int retval = -EINVAL;
>>       bool notify;
>>
>>       notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
>> -
>>       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) {
>> +                     retval = __target_intermediate(policy, &freqs, index);
>> +                     if (retval)
>> +                             return retval;
>
> Shouldn't this all be outside the if (notify) block, so that
> __target_intermediate is always called, and it's just the notify
> callbacks that gets skipped if (!notify)?

So, this is logic I had:

Should we support 'intermediate freq' infrastructure when driver wants
to handle notifications themselves?

Probably not.

The whole point of implementing this 'intermediate freq' infrastructure is to
get rid of code redundancy while sending notifications. If drivers want to
handle notifications then they better handle intermediate freqs as well in
their target_index() callback. They don't need to implement another routine
for intermediate stuff..

--
viresh
--
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/