On 07/15/2014 11:06 AM, Saravana Kannan wrote: > On 07/14/2014 09:35 PM, Viresh Kumar wrote: >> On 15 July 2014 00:38, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote: >>> Yeah, it definitely crashes if policy->cpu if an offline cpu. Because >>> the >>> mutex would be uninitialized if it's stopped after boot or it would >>> never >>> have been initialized (depending on how you fix policy->cpu at boot). >>> >>> Look at this snippet on the actual tree and it should be pretty evident. >> >> Yeah, I missed it. So the problem is we initialize timer_mutex's for >> policy->cpus. So we need to do that just for policy->cpu and also we >> don't >> need a per-cpu timer_mutex anymore. >> > > Btw, I tried to take a stab at removing any assumption in cpufreq code > about policy->cpu being ONLINE. Wait, allowing an offline CPU to be the policy->cpu (i.e., the CPU which is considered as the master of the policy/group) is just absurd. If there is no leader, there is no army. We should NOT sacrifice sane semantics for the sake of simplifying the code. > There are 160 instances of those of with > 23 are in cpufreq.c > And that explains why. It is just *natural* to assume that the CPUs governed by a policy are online. Especially so for the CPU which is supposed to be the policy leader. Let us please not change that - it will become counter-intuitive if we do so. [ The other reason is that physical hotplug is also possible on some systems... in that case your code might make a CPU which is not even present (but possible) as the policy->cpu.. and great 'fun' will ensue after that ;-( ] The goal of this patchset should be to just de-couple the sysfs files/ownership from the policy->cpu to an extent where it doesn't matter who owns those files, and probably make it easier to do CPU hotplug without having to destroy and recreate the files on every hotplug operation. This is exactly why the _implementation_ matters in this particular case - if we can't achieve the simplification by keeping sane semantics, then we shouldn't do the simplification! That said, I think we should keep trying - we haven't exhausted all ideas yet :-) Regards, Srivatsa S. Bhat > So, even if we are sure cpufreq.c is fine, it's 137 other uses spread > across all the other files. I definitely don't want to try and fix those > as part of this patch. Way too risky and hard to get the test coverage > it would need. Even some of the acpi cpufreq drivers seem to be making > this assumption. > > Btw, I think v3 is done. I did some testing and it was fine. But made > some minor changes. Will test tomorrow to make sure I didn't break > anything with the minor changes and then send them out. > -- 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
