lkml.org 
[lkml]   [2013]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Update PATCH 1/1] Cpufreq: Make governor data on nonboot cpus across system suspend/resume
    On 11/16/2013 10:41 PM, Rafael J. Wysocki wrote:
    > On Saturday, November 16, 2013 11:59:59 AM Lan Tianyu wrote:
    >> On 11/16/2013 08:38 AM, Rafael J. Wysocki wrote:
    >>> On Friday, November 15, 2013 04:15:34 PM Lan Tianyu wrote:
    >>>> Currently, governor of nonboot cpus will be put to EXIT when system suspend.
    >>>> Since all these cpus will be unplugged and the governor usage_count decreases
    >>>> to zero. The governor data and its sysfs interfaces will be freed or released.
    >>>> This makes user config of these governors loss during suspend and resume.
    >>>
    >>> First off, do we have a pointer to a bug report related to that?
    >>>
    >>
    >> No, I found this bug when I tried to resolve other similar bug.
    >> https://bugzilla.kernel.org/show_bug.cgi?id=63081. I still have no idea
    >> about bug 63081 and asked reporter to try this patch.
    >>
    >>> Second, what does need to be done to reproduce this problem?
    >>>
    >>
    >> Defaultly, all cpus use ondemand governor after bootup. Change one
    >> non-boot cpu's governor to conservative,
    >
    > Well, why would anyone want to do that? Just out of curiosity ...

    Just use this way to produce the issue. But on the laptop, I think
    fewer people will do the same thing. Just like Viresh said, this also
    will affect the systems of governor per policy.

    >
    >> modify conservative config via sysfs interface and then do system suspend.
    >> After resume, the config of conservative is reset. On my machine, all cpus
    >> have owen policy.
    >
    > So this is acpi-cpufreq, right?
    >

    Yes, it's acpi-cpufreq driver.

    > The patch looks basically OK to me, but ->
    >
    >>>> This doesn't happen on the governor covering boot cpu because it isn't
    >>>> unplugged during system suspend.
    >>>>
    >>>> To fix this issue, skipping governor exit during system suspend and check
    >>>> policy governor data to determine whether the governor is really needed
    >>>> to be initialized when do init. If not, return EALREADY to indicate the
    >>>> governor has been initialized and should do nothing. __cpufreq_governor()
    >>>> convert EALREADY to 0 as return value for INIT event since governor is
    >>>> still under INIT state and can do START operation.
    >>>>
    >>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
    >>>> ---
    >>>> Fix some typos
    >>>>
    >>>> drivers/cpufreq/cpufreq.c | 5 ++++-
    >>>> drivers/cpufreq/cpufreq_governor.c | 13 ++++++++++++-
    >>>> 2 files changed, 16 insertions(+), 2 deletions(-)
    >>>>
    >>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
    >>>> index 02d534d..38f2e4a 100644
    >>>> --- a/drivers/cpufreq/cpufreq.c
    >>>> +++ b/drivers/cpufreq/cpufreq.c
    >>>> @@ -1239,7 +1239,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
    >>>>
    >>>> /* If cpu is last user of policy, free policy */
    >>>> if (cpus == 1) {
    >>>> - if (has_target()) {
    >>>> + if (has_target() && !frozen) {
    >>>> ret = __cpufreq_governor(policy,
    >>>> CPUFREQ_GOV_POLICY_EXIT);
    >>>> if (ret) {
    >>>> @@ -1822,6 +1822,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
    >>>> ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
    >>>> module_put(policy->governor->owner);
    >>>>
    >>>> + if ((event == CPUFREQ_GOV_POLICY_INIT) && ret == -EALREADY)
    >>>> + ret = 0;
    >>>> +
    >
    > -> I'd prefer this check to be combined with the one done to determine whether
    > or not we need to do the module_put(). Something like
    >
    > if (event == CPUFREQ_GOV_POLICY_EXIT && ret) {
    > module_put(policy->governor->owner);
    > if (ret == -EALREADY)
    > return 0;
    > } else if (event == CPUFREQ_GOV_POLICY_EXIT && !ret) {
    > module_put(policy->governor->owner);
    > }
    >

    Ok. I will update soon.

    > Thanks!
    >
    >>>> return ret;
    >>>> }
    >>>>
    >>>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
    >>>> index 0806c31..ddb93af 100644
    >>>> --- a/drivers/cpufreq/cpufreq_governor.c
    >>>> +++ b/drivers/cpufreq/cpufreq_governor.c
    >>>> @@ -204,9 +204,20 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
    >>>>
    >>>> switch (event) {
    >>>> case CPUFREQ_GOV_POLICY_INIT:
    >>>> + /*
    >>>> + * In order to keep governor data across suspend/resume,
    >>>> + * Governor doesn't exit when suspend and will be
    >>>> + * reinitialized when resume. Here check policy governor
    >>>> + * data to determine whether the governor has been exited.
    >>>> + * If not, return EALREADY.
    >>>> + */
    >>>> if (have_governor_per_policy()) {
    >>>> - WARN_ON(dbs_data);
    >>>> + if (dbs_data)
    >>>> + return -EALREADY;
    >>>> } else if (dbs_data) {
    >>>> + if (policy->governor_data == dbs_data)
    >>>> + return -EALREADY;
    >>>> +
    >>>> dbs_data->usage_count++;
    >>>> policy->governor_data = dbs_data;
    >>>> return 0;
    >>>>
    >>
    >>
    >>


    --
    Best Regards
    Tianyu Lan


    \
     
     \ /
      Last update: 2013-11-16 16:41    [W:3.970 / U:0.784 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site