From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 04/11] x86/intel_pstate: relocate the driver register function Date: Thu, 18 Jun 2015 15:30:12 +0100 Message-ID: <5582F2140200007800086ADF@mail.emea.novell.com> References: <1434011230-17052-1-git-send-email-wei.w.wang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1434011230-17052-1-git-send-email-wei.w.wang@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Wang Cc: andrew.cooper3@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 11.06.15 at 10:27, wrote: > Register the CPU hotplug notifier when the driver is > registered, and move the driver register function to > the cpufreq.c. The first half of the sentence fails to say why. And I suppose if you explained that (to yourself) you'd figure that the change is wrong (or at least altering behavior in a way that needs more explanation to be verifiably correct): The calls to cpufreq_register_driver() sit in __initcall handlers, yet what you replace is a presmp_initcall. I.e. all APs being brought up at boot time won't get the callback invoked for them anymore. I suppose you tested your series on a system where the new driver will kick in. You of course also need to test the case where this isn't the case - everything needs to continue to function there. > --- a/xen/drivers/cpufreq/cpufreq.c > +++ b/xen/drivers/cpufreq/cpufreq.c > @@ -630,12 +630,21 @@ static struct notifier_block cpu_nfb = { > .notifier_call = cpu_callback > }; > > -static int __init cpufreq_presmp_init(void) > +int cpufreq_register_driver(struct cpufreq_driver *driver_data) > { > void *cpu = (void *)(long)smp_processor_id(); > cpu_callback(&cpu_nfb, CPU_ONLINE, cpu); > + if (!driver_data || !driver_data->init > + || !driver_data->verify || !driver_data->exit > + || (!driver_data->target == !driver_data->setpolicy)) Do you really want/need to enforce this policy (target set if and only if setpolicy is not set) here? And if that's to uniformly hold, the two could be put into a union... Also - coding style. > --- a/xen/include/acpi/cpufreq/cpufreq.h > +++ b/xen/include/acpi/cpufreq/cpufreq.h > @@ -171,32 +171,8 @@ struct cpufreq_driver { > > extern struct cpufreq_driver *cpufreq_driver; > > -static __inline__ > -int cpufreq_register_driver(struct cpufreq_driver *driver_data) > -{ > - if (!driver_data || > - !driver_data->init || > - !driver_data->exit || > - !driver_data->verify || > - !driver_data->target) > - return -EINVAL; > - > - if (cpufreq_driver) > - return -EBUSY; > - > - cpufreq_driver = driver_data; > - return 0; > -} > - > -static __inline__ > -int cpufreq_unregister_driver(struct cpufreq_driver *driver) > -{ > - if (!cpufreq_driver || (driver != cpufreq_driver)) > - return -EINVAL; > - > - cpufreq_driver = NULL; > - return 0; > -} > +extern int cpufreq_register_driver(struct cpufreq_driver *driver_data); > +extern int cpufreq_unregister_driver(struct cpufreq_driver *driver); What's this declaration good for when there's no implementation? Jan