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@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
