lkml.org 
[lkml]   [2014]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend
On 07/16/2014 04:16 AM, Srivatsa S. Bhat wrote:
> On 07/16/2014 01:54 PM, Viresh Kumar wrote:
>> On 16 July 2014 04:17, Saravana Kannan <skannan@codeaurora.org> wrote:
>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>

<SNIP>

>>> -static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>>> - unsigned int cpu, struct device *dev)
>>> +static int cpufreq_change_policy_cpus(struct cpufreq_policy *policy,
>>> + unsigned int cpu, bool add)
>
> [...]
>
>>> -
>>> - if (!cpufreq_driver->setpolicy)
>>> - strncpy(per_cpu(cpufreq_cpu_governor, cpu),
>>> - policy->governor->name, CPUFREQ_NAME_LEN);
>>
>> Where is this gone? There are several instances of code just being
>> removed, this is the third one. Its really really tough to catch these
>> in this big of a patch. Believe me.
>>
>> You have to break this patch into multiple ones, see this on how to
>> break even simplest of the changes into multiple patches:
>> https://lkml.org/lkml/2013/9/6/400
>>
>> Its just impossible to catch bugs that you might have introduced here due
>> to the size of this patch. And its taking a LOT of time for me to review this.
>> As I have to keep diff in one tab, new cpufreq.c in one and the old cpufreq.c
>> in one and then compare..
>>
>
> True, this is still a pretty huge chunk. Saravana, at this stage, don't worry
> about making cpufreq work properly in each and every patch. Just ensure that
> every patch builds fine; that should be good enough. I hope this will help you
> in splitting up the patches further.

Thanks Srivatsa. This will definitely help split them up into smaller
chunks.

> One other thing: your changelog contains what we usually write in a cover-
> letter - *very* high-level goals of the patch. Ideally, you should explain
> the subtle details and the non-obvious decisions or trade-offs that you have
> made at various places in the code. Otherwise it becomes very hard to follow
> your thought-flow just by looking at the patch. So please split up the patch
> further and also make the changelogs useful to review the patch :-)

Thanks. Will do.

> The link that Viresh gave above also did a lot of code reorganization in
> cpufreq, so it should give you a good example of how to proceed.
>
> [...]
>
>>> __cpufreq_add_dev(dev, NULL);
>>> break;
>>>
>>> case CPU_DOWN_PREPARE:
>>> - __cpufreq_remove_dev_prepare(dev, NULL);
>>> - break;
>>> -
>>> - case CPU_POST_DEAD:
>>> - __cpufreq_remove_dev_finish(dev, NULL);
>>> - break;
>>> -
>>> - case CPU_DOWN_FAILED:
>>> - __cpufreq_add_dev(dev, NULL);
>>> + __cpufreq_remove_dev(dev, NULL);
>>
>> @Srivatsa: You might want to have a look at this, remove sequence was
>> separated for some purpose and I am just not able to concentrate enough
>> to think of that, just too many cases running in my mind :)
>>
>
> Yeah, we had split it into _remove_dev_prepare() and _remove_dev_finish()
> to avoid a few potential deadlocks. We wanted to call _remove_dev_prepare()
> in the DOWN_PREPARE stage and then call _remove_dev_finish() (which waits
> for the kobject refcount to drop) in the POST_DEAD stage. That is, we wanted
> to do the kobject cleanup after releasing the hotplug lock, and POST_DEAD stage
> was well-suited for that.
>
> Commit 1aee40ac9c8 (cpufreq: Invoke __cpufreq_remove_dev_finish() after
> releasing cpu_hotplug.lock) explains this in detail. Saravana, please take a
> look at that reasoning and ensure that your patch doesn't re-introduce those
> deadlock possibilities!

But all of that was needed _because_ we were creating and destroying
policies and kobjs all the time. We don't do that anymore. So, I don't
think any of that applies. We only destroy when the cpufreq driver is
unregistered. That's kinda of the point of this patchset.

Thoughts?

-Saravana

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation


\
 
 \ /
  Last update: 2014-07-16 22:21    [W:0.159 / U:0.812 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site