Re: [Xen-devel] [PATCH v4 07/11] x86/intel_pstate: the main boby of the intel_pstate driver

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.07.15 at 11:30, <wei.w.wang@xxxxxxxxx> wrote:
> On 24/07/2015 21:54,  Jan Beulich wrote:
>> >>> On 25.06.15 at 13:16, <wei.w.wang@xxxxxxxxx> wrote:
>> > +int __init intel_pstate_init(void)
>> > +{
>> > +  int cpu, rc = 0;
>> > +  const struct x86_cpu_id *id;
>> > +  struct cpu_defaults *cpu_info;
>> > +
>> > +  id = x86_match_cpu(intel_pstate_cpu_ids);
>> > +  if (!id)
>> > +          return -ENODEV;
>> > +
>> > +  cpu_info = (struct cpu_defaults *)id->driver_data;
>> > +
>> > +  copy_pid_params(&cpu_info->pid_policy);
>> > +  copy_cpu_funcs(&cpu_info->funcs);
>> > +
>> > +  if (intel_pstate_msrs_not_valid())
>> > +          return -ENODEV;
>> > +
>> > +  all_cpu_data = xzalloc_array(struct cpudata *, NR_CPUS);
>> > +  if (!all_cpu_data)
>> > +          return -ENOMEM;
>> > +
>> > +  rc = cpufreq_register_driver(&intel_pstate_driver);
>> > +  if (rc)
>> > +          goto out;
>> > +
>> > +  return rc;
>> > +out:
>> > +  for_each_online_cpu(cpu) {
>> > +          if (all_cpu_data[cpu]) {
>> > +                  kill_timer(&all_cpu_data[cpu]->timer);
>> > +                  xfree(all_cpu_data[cpu]);
>> > +          }
>> > +  }
>> 
>> I have a hard time seeing where in this function the setup happens that is
>> being undone here (keeping in mind that the notifier registration inside
>> cpufreq_register_driver() doesn't actually call the notifier function).
>> 
>> And then, looking at the diff between this and what Linux 4.2-rc3 has (which
>> admittedly looks a little newer than what you sent, so I already subtract
>> some of the delta), it is significantly larger than the source file itself. 
> That
>> surely doesn't suggest a clone-with- minimal-delta. Yet as said before - 
>> either
>> you do that, or you accept us picking at things you inherited from Linux.
> 
> I think it's better to choose the latter - picking out things that are useful 
> for us from Linux.
> Can you please take a look this patch and summarize the comments? Thanks.

I'm sorry, but for a first round I'd rather expect _you_ to go through
the code you intend to add and spot possible problems. Only then, on
a submission where you state that you did so, would I want to invest
time in sanity checking things.

And then I hope you realize that the clone-with-minimal-delta would
have benefits on the maintenance side going forward (fewer manual
adjustments needed due to non-applying Linux side changes).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel