| 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
| ||
|
Message-ID: <CAGTfZH0HDuexEHR5aqBxWifFKL1fBT1K80XGFX27YqHtDf_7hw@mail.gmail.com>
Date: Sun, 18 Dec 2016 01:39:41 +0900
From: Chanwoo Choi <cwchoi00@...il.com>
To: Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>
Cc: Chanwoo Choi <cw00.choi@...sung.com>, hl <hl@...k-chips.com>,
"myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"dbasehore@...omium.org" <dbasehore@...omium.org>,
"dianders@...omium.org" <dianders@...omium.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-rockchip@...ts.infradead.org"
<linux-rockchip@...ts.infradead.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support
2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>:
> Hey guys,
>
> Chanwoo Choi wrote:
>> Hi Lin,
>>
>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@...sung.com>:
>>> Hi Lin,
>>>
>>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> On 2016년 11월 24일 17:34, hl wrote:
>>>>> Hi Chanwoo Choi,
>>>>>
>>>>>
>>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>>> Hi Lin,
>>>>>>
>>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>>> Hi Chanwoo Choi,
>>>>>>>
>>>>>>> I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>>
>>>>>>> &dmc_opp_table {
>>>>>>> opp06 {
>>>>>>> opp-suspend;
>>>>>>> };
>>>>>>> };
>>>>>> Two approaches use the 'opp-suspend' property.
>>>>>>
>>>>>> I think that the method to support suspend-opp have to
>>>>>> guarantee following conditions:
>>>>>> - Support the all of devfreq's governors.
>>>>> As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>>> which will ingore governor.
>>>>
>>>> Other approach already support the all of governors.
>>>> Before calling the mail, I discussed with Myungjoo Ham.
>>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>>
>>> It is not correct expression. We need to wait the reply from Myungjoo
>>> to clarify this.
>>>
>>>>
>>>> To Myungjoo,
>>>> Please add your opinion how to support the suspend frequency.
>>>
>>>>
>>>>>> - Devfreq framework have the responsibility to change the
>>>>>> frequency/voltage for suspend-opp. If we uses the
>>>>>> new devfreq_suspend(), each devfreq device don't care
>>>>>> how to support the suspend-opp. Just the developer of each
>>>>>> devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>>
>>>> No, the frequency should be handled by governor or framework.
>>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>>> The governor and core of devfreq can decide the next frequency/voltage.
>>>> You can refer to the cpufreq subsystem.
>>>>
>>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>>
>>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>>> It think this way is easy.
>>>>
>>>> But,
>>>> If the each devfreq device want to decide the next frequency/voltage only for
>>>> suspend state. We can check the cpufreq subsystem.
>>>>
>>>> If specific devfreq device want to handle the suspend frequency,
>>>> each devfreq will add the own suspend/resume functions as following:
>>>>
>>>> struct devfreq_dev_profile {
>>>> int (*suspend)(struct devfreq *dev); // new function pointer
>>>> int (*resume)(struct devfreq *dev); // new function pointer
>>>> } a_profile;
>>>>
>>>> a_profile = devfreq_generic_suspend;
>>>>
>>>> The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>> int devfreq_generic_suspend(struce devfreq *dev) {
>>>> ...
>>>> devfreq->profile->target(..., devfreq->suspend_freq);
>>>> ...
>>>> }
>>>>
>>>> or
>>>>
>>>> a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>>
>>>> The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>> instead of devfreq->profile->target();
>>>>
>>>> The devfreq call the 'devfreq->profile->suspend()'
>>>> to support the suspend frequency.
>>>>
>>>> Regards,
>>>> Chanwoo Choi
>>>
>>> The key difference between two approaches:
>>>
>>> Your approach:
>>> - The each developer should add the 'opp-suspend' property to the dts file.
>>> - The each devfreq should call the devfreq_suspend_device()
>>> to support the suspend frequency.
>>>
>>> If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>> can support the suspend frequency.
>>>
>>> Other approach:
>>> - The each developer only should add the 'opp-suspend' property to the dts file
>>> without the additional behavior.
>>>
>>> In the cpufreq subsystem,
>>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>>> without the additional behavior.
>>
>> I'm missing the use-case when using the devfreq_suspend_device()
>> before entering the suspend mode. We should consider the case when
>> devfreq device
>> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
>> is exported function, each devfreq device call this function on the fly
>> without entering the suspend mode.
>>
>> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
>> So, I make the following patch. This patch set the suspend frequency
>> in devfreq_suspend_device() after stoping the governor.
>> It consider the all governors of devfreq.
>>
>> What do you think?
>> If you are ok, I'll send this patch with your author.
> The problem I see here is that we need to keep track of the suspended
> state when suspending the (entire) devfreq subsystem. When doing that,
> we don't know if any device driver has previously called
> devfreq_suspend_device() and might end up calling it twice.
>
> Same thing on devfreq subsystem resume.
>
> I've prepared a new RFC of my series (going to send it shortly), but I'm
> not so happy with the current design. I think it would be much cleaner
> to keep some suspend_refcount in struct devfreq so that I can call
> devfreq_suspend_device() multiple times, while keeping a sane internal
> state.
I agree the devfreq need reference count for devfreq_suspend/resume_device.
This patch focus on when changing the suspend frequency.
>
> Something like devfreq_device_runtime_{put,get}() perhaps?
Why do devfreq need new additional functions?
I think the devfreq_suspend/resume_device are enough.
Thanks,
Chanwoo Choi
>
> - Tobias
>
>
>
>>
>> int devfreq_suspend_device(struct devfreq *devfreq)
>> {
>> + int ret = 0;
>> +
>> if (!devfreq)
>> return -EINVAL;
>>
>> if (!devfreq->governor)
>> return 0;
>>
>> - return devfreq->governor->event_handler(devfreq,
>> + ret = devfreq->governor->event_handler(devfreq,
>> DEVFREQ_GOV_SUSPEND, NULL);
>> + if (ret < 0) {
>> + dev_err(devfreq->dev.parent, "failed to suspend governor\n");
>> + return ret;
>> + }
>> +
>> + if (devfreq->suspend_freq) {
>> + ret = devfreq->profile->target(devfreq->dev.parent,
>> + &devfreq->suspend_freq, 0);
>> + if (ret < 0) {
>> + dev_err(devfreq->dev.parent,
>> + "failed to set suspend-freq\n");
>> + return ret;
>> + }
>> + dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
>> + devfreq->suspend_freq);
>> + }
>> +
>> + return 0;
>> }
>> EXPORT_SYMBOL(devfreq_suspend_device);
>>
>
Powered by blists - more mailing lists