linux-kernel - Re: [PATCH v3 5/8] PM / devfreq: Get the available next frequency on update

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:   Wed, 11 Oct 2017 22:33:16 +0900
From:   Chanwoo Choi <cwchoi00@...il.com>
To:     "myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>
Cc:     Chanwoo Choi <cw00.choi@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        "rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
        Inki Dae <inki.dae@...sung.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v3 5/8] PM / devfreq: Get the available next frequency on update_devfreq()

On Wed, Oct 11, 2017 at 8:30 PM, MyungJoo Ham <myungjoo.ham@...sung.com> wrote:
>> The update_devfreq() considers only user frequency (min_freq/max_freq)
>> and the next target_freq provided by the governor. But, the commit
>> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
>> OPP as a cooling device. In result, the update_devfreq() have to
>> consider the 'opp->available' status in order to decicde the next freq
>> by the devfreq_recommended_opp().
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 1c4b377cacfb..3b9662ffe603 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>  int update_devfreq(struct devfreq *devfreq)
>>  {
>>       struct devfreq_freqs freqs;
>> +     struct dev_pm_opp *opp;
>>       unsigned long freq, cur_freq;
>>       int err = 0;
>>       u32 flags = 0;
>> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>>               return err;
>>
>>       /*
>> -      * Adjust the frequency with user freq and QoS.
>> +      * Adjust the frequency with user freq, QoS and available freq.
>>        *
>>        * List from the highest priority
>>        * max_freq
>> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>>               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>       }
>>
>> +     opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
>> +     if (IS_ERR(opp))
>> +             return PTR_ERR(opp);
>> +     freq = dev_pm_opp_get_freq(opp);
>> +     dev_pm_opp_put(opp);
>> +
>
> Is this really necessary?

The requirement is due to devfreq_cooling device using
dev_pm_opp_disable/enable().

I added the detailed explanation on cover letter as following:
If this code is not included, the notifiee using TRANSITION_NOTIFIER
receives the wrong next target_freq. On the cpufreq, cpufreq doesn't
use the 'dev_pm_opp_disable/enable()' function and then there is no
the same issue on cpufreq.

[Cover letter's description about this patch]
For example,
- devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
- OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
- simple_ondemand govenor decided the next target_freq (600Mhz)
|----------|-------------------------------------------------------------|
|Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
|Devfreq   |min_freq|       |       |        |        |         |max_freq|
|OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
|Ondmenad  |        |       |       |        |        |next_freq|        |
|------------------------------------------------------------------------|

In result,
- Before this patch, target_freq is 600Mhz
  and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
- After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
  and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.
--------------

>
> devfreq_recommended_opp is going to be called by the device driver
> invoked by devfreq->profile->target() function anyway.
>
> We are now going to call devfreq_recommended_opp twice in this context.
>
>>       if (devfreq->profile->get_cur_freq)
>>               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>>       else
>> --

Right. The devfreq_recommended_opp() is called twice.
I wish there was a better way.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics