On 07/15/2014 11:05 PM, skannan@xxxxxxxxxxxxxx wrote: > > Srivatsa S. Bhat wrote: >> 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 :-) >> > > I don't think we disagree. To summarize this topic: I tried to keep the > policy->cpu an actual online CPU so as to not break existing semantics in > this patch. Viresh asked "why not fix it at boot?". My response was to > keep it an online CPU and give it a shot in a separate patch if we really > want that. It's too risky to do that in this patch and also not a > mandatory change for this patch. > > I think we can work out the details on the need to fixing policy->cpu at > boot and whether there's even a need for policy->cpu (when we already have > policy->cpus) in a separate thread after the dust settles on this one? > Sure, that sounds good! Regards, Srivatsa S. Bhat -- 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
