Messages in this thread | | | Date | Wed, 23 Mar 2011 15:55:01 +0530 | From | Trinabh Gupta <> | Subject | Re: [RFC PATCH V4 5/5] cpuidle: cpuidle driver for apm |
| |
Hi Stephen,
Thanks for reviewing.
On 03/23/2011 06:44 AM, Stephen Rothwell wrote: > Hi, > > On Tue, 22 Mar 2011 18:03:40 +0530 Trinabh Gupta<trinabh@linux.vnet.ibm.com> wrote: >> >> +static int apm_setup_cpuidle(int cpu) >> +{ >> + struct cpuidle_device *dev = kzalloc(sizeof(struct cpuidle_device), >> + GFP_KERNEL); > > Same as xen comments: no NULL check. > >> + int count = CPUIDLE_DRIVER_STATE_START; >> + dev->cpu = cpu; >> + dev->drv =&apm_idle_driver; > > Also wondering why you would ever have a different idle routine on > different cpus?
Yes, this is an ongoing debate. Apparently it is a possibility because of ACPI bugs. CPU's can have asymmetric C-states and overall different idle routines on different cpus. Please refer to http://lkml.org/lkml/2009/9/24/132 and https://lkml.org/lkml/2011/2/10/37 for a discussion around this.
I have posted a patch series that does global registration i.e same idle routines for each cpu. Please check http://lkml.org/lkml/2011/3/22/161 . That series applies on top of this series. Global registration significantly simplifies the design, but still we are not sure about the direction to take.
> >> + >> + dev->states[count] = state_apm_idle; >> + count++; >> + >> + dev->state_count = count; >> + >> + if (cpuidle_register_device(dev)) >> + return -EIO; > > And we leak dev. > >> +static void apm_idle_exit(void) >> +{ >> + cpuidle_unregister_driver(&apm_idle_driver); > > Do we leak the per cpu device structure here?
I will see how we can save per cpu device structure pointers so that we can free them.
> >> + return; > > Unnecessary return statement. >
Thanks, -Trinabh
| |