linux-kernel - Re: [PATCH 3/3] PM / devfreq: account suspend/resume for stats

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: <CAJ0PZbRuc4o4+-c9uaQL4QO-Urwm_kd_me_uXsAUWz6Lw2WMLw@mail.gmail.com>
Date:	Tue, 5 Feb 2013 15:29:27 +0900
From:	MyungJoo Ham <myungjoo.ham@...il.com>
To:	Rajagopal Venkat <rajagopal.venkat@...aro.org>
Cc:	jonghwa3.lee@...sung.com, kyungmin.park@...sung.com,
	mturquette@...aro.org, rjw@...k.pl, patches@...aro.org,
	linaro-dev@...ts.linaro.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] PM / devfreq: account suspend/resume for stats

On Tue, Jan 15, 2013 at 8:46 PM, Rajagopal Venkat
<rajagopal.venkat@...aro.org> wrote:
> On 14 January 2013 20:18, MyungJoo Ham <myungjoo.ham@...sung.com> wrote:
>> On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat
>> <rajagopal.venkat@...aro.org> wrote:
>>> devfreq stats is not taking device suspend and resume into
>>> account. Fix it.
>>>
>>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@...aro.org>
>>
>> With monitor_suspend(), we are suspending the DVFS mechanism of a
>> device, not the device itself.
>>
>> Thus, the device may keep its frequency running and we may assume that
>> the frequency is constant as the DVFS mechanism is in suspend.
>>
>> Why do you want to stop the statistics as well?
>>
>
> There seems to be misunderstanding of devfreq_monitor_suspend() and
> devfreq_monitor_resume() apis usage.
>
> Typically devfreq_monitor_suspend() would be called to suspend the device
> devfreq when device is entering idle state by powering off(clocks and voltage).
> When device itself is off, it's incorrect to allow stats to continue.

Ah.. yes... you're right. It is meant to be called by suspend of
pm-sleep or pm-suspend.
This will be applied soon.


Acked-by: MyungJoo Ham <myungjoo.ham@...sung.com>

>
>>
>>
>> Again, as in the other patch, this is about the semantics of the
>> "devfreq statistics".
>>
>> It does not seem to be a problem of which is correct and which is not,
>> but it seems to be a problem of which is more convenient.
>>
>> Could you please give me some cases where your semantics is more helpful?
>
> When gpu is powered off, gpu devfreq should be suspended and hence the stats.



>
>>
>>
>>
>> I've been using the stat feature like this:
>>
>> # cat stat; run benchmark; cat stat
>> and look at the differences with any ops done during "run benchmark".
>>
>>
>>
>> Cheers,
>> MyungJoo.
>>
>>> ---
>>>  drivers/devfreq/devfreq.c |   15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 2843a22..4c50235 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>>>                 return;
>>>         }
>>>
>>> +       devfreq_update_status(devfreq, devfreq->previous_freq);
>>>         devfreq->stop_polling = true;
>>>         mutex_unlock(&devfreq->lock);
>>>         cancel_delayed_work_sync(&devfreq->work);
>>> @@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend);
>>>   */
>>>  void devfreq_monitor_resume(struct devfreq *devfreq)
>>>  {
>>> +       unsigned long freq;
>>> +
>>>         mutex_lock(&devfreq->lock);
>>>         if (!devfreq->stop_polling)
>>>                 goto out;
>>> @@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>>                         devfreq->profile->polling_ms)
>>>                 queue_delayed_work(devfreq_wq, &devfreq->work,
>>>                         msecs_to_jiffies(devfreq->profile->polling_ms));
>>> +
>>> +       devfreq->last_stat_updated = jiffies;
>>>         devfreq->stop_polling = false;
>>>
>>> +       if (devfreq->profile->get_cur_freq &&
>>> +               !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
>>> +               devfreq->previous_freq = freq;
>>> +
>>>  out:
>>>         mutex_unlock(&devfreq->lock);
>>>  }
>>> @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att
>>>  {
>>>         struct devfreq *devfreq = to_devfreq(dev);
>>>         ssize_t len;
>>> -       int i, j, err;
>>> +       int i, j;
>>>         unsigned int max_state = devfreq->profile->max_state;
>>>
>>> -       err = devfreq_update_status(devfreq, devfreq->previous_freq);
>>> -       if (err)
>>> +       if (!devfreq->stop_polling &&
>>> +                       devfreq_update_status(devfreq, devfreq->previous_freq))
>>>                 return 0;
>>>
>>>         len = sprintf(buf, "   From  :   To\n");
>>> --
>>> 1.7.10.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> MyungJoo Ham, Ph.D.
>> Mobile Software Platform Lab, DMC Business, Samsung Electronics
>
>
>
> --
> Regards,
> Rajagopal



-- 
MyungJoo Ham, Ph.D.
System S/W Lab, S/W Center, Samsung Electronics
--
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/