linux-kernel - Re: [PATCH] cpufreq: Fix cpufreq regression after suspend/resume

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]

Date:	Thu, 11 Jul 2013 22:03:39 +0800
From:	Lan Tianyu <lantianyu1986@...il.com>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	Toralf Förster <toralf.foerster@....de>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	cpufreq@...r.kernel.org, Linux PM list <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Jarzmik, Robert" <robert.jarzmik@...el.com>,
	"R, Durgadoss" <durgadoss.r@...el.com>,
	Dirk Brandewie <dirk.brandewie@...il.com>, tianyu.lan@...el.com
Subject: Re: [PATCH] cpufreq: Fix cpufreq regression after suspend/resume

2013/7/11 Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>:
> On 07/11/2013 11:10 AM, Lan Tianyu wrote:
>> 2013/7/11 Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>:
> Oops! You are right. Hmm, this looks quite difficult to get right :(
> There are multiple challenges here:

If I understand correctly, the most concern is how to deal with per cpus'
cpufreq_policy structure. If something wrong , please correct me.

>
> 1. The sysfs files must not be removed during cpu_down, and not initialized
>
>    during cpu_up. That would help us preserve the file permissions.

For this case, cpufreq_policy must be reserved since all related cpufreq data
and kobj is also store into it. We can't release it.

> 2. But we should ensure that we really do the cpufreq-core parts of the cpu
>    initialization during cpu_up. If we fail to free some of the data-structures
>    during cpu_down, the cpu_up callback will think that a full-init is not
>    required and not do its job. That will make cpufreq behave erratically after
>    suspend/resume and take us back to square one.
>

cpufreq_add_dev() checks whether the cpu has been attached cpufreq_cpu_data
and associated kobj has been created. If yes, it would try to release it by
cpufreq_cpu_put() and return directly. This seems to be conflicted
with the above.

> 3. A full re-init in the cpu_up callback also involves memory allocations.
>    So if we don't release the memory in the cpu_down callback, we'll end up
>    in a memory leak.
>

Even we remove the previous check, cpu scaling driver's init()  also will change
some data of struct cpufreq_policy and this is also what we don't like to see.

> I tried to address all these in this patch, but you found yet another serious
> loop-hole. I guess I'm out of ideas now... if anybody has any thoughts on how
> to get this right, then I'm all ears. Else, we'll just revert the original
> commit like Rafael suggested and leave it upto userspace to save and restore
> the permissions across suspend/resume if it wants ;-(
>

How about implement scaling driver's suspend/resume callback()? Although this
needs to be dealt with case by case. If one's callbacks hasn't been implemented,
it would have to follow current rule.

>>> +                       unlock_policy_rwsem_read(cpu);
>>> +                       kobject_put(kobj);
>>> +
>>> +                       pr_debug("waiting for dropping of refcount\n");
>>> +                       wait_for_completion(cmp);
>>> +                       pr_debug("wait complete\n");
>>> +               }
>>> +
>>>         } else if (cpufreq_driver->target) {
>>>                 __cpufreq_governor(data, CPUFREQ_GOV_START);
>>>                 __cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
>>> @@ -1221,7 +1230,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>>>         if (cpu_is_offline(cpu))
>>>                 return 0;
>>>
>>> -       retval = __cpufreq_remove_dev(dev, sif);
>>> +       retval = __cpufreq_remove_dev(dev, sif, true);
>>>         return retval;
>>>  }
>
> Regards,
> Srivatsa S. Bhat
>



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