| 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
| ||
|
Date: Sat, 16 Nov 2013 23:23:59 +0800
From: Lan Tianyu <tianyu.lan@...el.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
CC: viresh.kumar@...aro.org, cpufreq@...r.kernel.org,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [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@...el.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
--
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/
Powered by blists - more mailing lists