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]

Message-id: <59E060FF.1060507@samsung.com>
Date:   Fri, 13 Oct 2017 15:45:19 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     "myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>
Cc:     cwchoi00@...il.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()

Hi,

On 2017년 10월 11일 22:33, Chanwoo Choi wrote:
> 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 got the better solution. If struct devfreq contains the 'scaling_min/max_freq'
variable, this issue could be fixed. I'll update it with scaling_min/max_freq'
variables on v4.

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