[Xen-devel] Re: [RFC PATCH V4 5/5] cpuidle: cpuidle driver for apm

[Trinabh Gupta <trinabh@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>  
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel