| Message ID | 1370641990-13884-1-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Friday, June 07, 2013 11:53:09 PM Daniel Lezcano wrote: > Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver > support. The code added a couple of new API to register the driver per cpu. > That led to some code complexity to handle the kernel config options when > the multiple driver support is enabled or not, which is not really necessary. > The code has to be compatible when the multiple driver support is not enabled, > and the multiple driver support has to be compatible with the old api. > > This patch removes this API, which is not yet used by any driver but needed > for the HMP cpuidle drivers which will come soon, and replaces its usage > by a cpumask pointer in the cpuidle driver structure telling what cpus are > handled by the driver. That let the API cpuidle_[un]register_driver to be used > for the multiple driver support and also the cpuidle_[un]register functions, > added recently in the cpuidle framework. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Queued up for 3.11 with a modified changelog. Thanks, Rafael > --- > [V4] > - uninitialize per cpu driver variable on error > - rollback indentation in cpuidle.h > [V3] > - moved the kerneldoc and comments to a separate patch > [V2]: > - fixed bad refcount check > - inverted clockevent notify off order at unregister time > > drivers/cpuidle/cpuidle.c | 4 +- > drivers/cpuidle/driver.c | 192 +++++++++++++++++++-------------------------- > include/linux/cpuidle.h | 6 +- > 3 files changed, 84 insertions(+), 118 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index c3a93fe..fdc432f 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -466,7 +466,7 @@ void cpuidle_unregister(struct cpuidle_driver *drv) > int cpu; > struct cpuidle_device *device; > > - for_each_possible_cpu(cpu) { > + for_each_cpu(cpu, drv->cpumask) { > device = &per_cpu(cpuidle_dev, cpu); > cpuidle_unregister_device(device); > } > @@ -498,7 +498,7 @@ int cpuidle_register(struct cpuidle_driver *drv, > return ret; > } > > - for_each_possible_cpu(cpu) { > + for_each_cpu(cpu, drv->cpumask) { > device = &per_cpu(cpuidle_dev, cpu); > device->cpu = cpu; > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 8dfaaae..e75fa54 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -18,167 +18,140 @@ > > DEFINE_SPINLOCK(cpuidle_driver_lock); > > -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); > -static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > > -static void cpuidle_setup_broadcast_timer(void *arg) > +static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); > + > +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > { > - int cpu = smp_processor_id(); > - clockevents_notify((long)(arg), &cpu); > + return per_cpu(cpuidle_drivers, cpu); > } > > -static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu) > +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) > { > - int i; > - > - drv->refcnt = 0; > + int cpu; > > - for (i = drv->state_count - 1; i >= 0 ; i--) { > + for_each_cpu(cpu, drv->cpumask) { > > - if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) > + if (drv != __cpuidle_get_cpu_driver(cpu)) > continue; > > - drv->bctimer = 1; > - on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, > - (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); > - break; > + per_cpu(cpuidle_drivers, cpu) = NULL; > } > } > > -static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) > +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) > { > - if (!drv || !drv->state_count) > - return -EINVAL; > - > - if (cpuidle_disabled()) > - return -ENODEV; > + int cpu; > > - if (__cpuidle_get_cpu_driver(cpu)) > - return -EBUSY; > + for_each_cpu(cpu, drv->cpumask) { > > - __cpuidle_driver_init(drv, cpu); > + if (__cpuidle_get_cpu_driver(cpu)) { > + __cpuidle_unset_driver(drv); > + return -EBUSY; > + } > > - __cpuidle_set_cpu_driver(drv, cpu); > + per_cpu(cpuidle_drivers, cpu) = drv; > + } > > return 0; > } > > -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu) > -{ > - if (drv != __cpuidle_get_cpu_driver(cpu)) > - return; > +#else > > - if (!WARN_ON(drv->refcnt > 0)) > - __cpuidle_set_cpu_driver(NULL, cpu); > +static struct cpuidle_driver *cpuidle_curr_driver; > > - if (drv->bctimer) { > - drv->bctimer = 0; > - on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, > - (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); > - } > +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > +{ > + return cpuidle_curr_driver; > } > > -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) > +{ > + if (cpuidle_curr_driver) > + return -EBUSY; > > -static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); > + cpuidle_curr_driver = drv; > > -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) > -{ > - per_cpu(cpuidle_drivers, cpu) = drv; > + return 0; > } > > -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) > { > - return per_cpu(cpuidle_drivers, cpu); > + if (drv == cpuidle_curr_driver) > + cpuidle_curr_driver = NULL; > } > > -static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv) > +#endif > + > +static void cpuidle_setup_broadcast_timer(void *arg) > { > - int cpu; > - for_each_present_cpu(cpu) > - __cpuidle_unregister_driver(drv, cpu); > + int cpu = smp_processor_id(); > + clockevents_notify((long)(arg), &cpu); > } > > -static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) > +static int __cpuidle_driver_init(struct cpuidle_driver *drv) > { > - int ret = 0; > - int i, cpu; > + int i; > > - for_each_present_cpu(cpu) { > - ret = __cpuidle_register_driver(drv, cpu); > - if (ret) > - break; > - } > + drv->refcnt = 0; > > - if (ret) > - for_each_present_cpu(i) { > - if (i == cpu) > - break; > - __cpuidle_unregister_driver(drv, i); > - } > + if (!drv->cpumask) > + drv->cpumask = (struct cpumask *)cpu_possible_mask; > > + for (i = drv->state_count - 1; i >= 0 ; i--) { > > - return ret; > + if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) > + continue; > + > + drv->bctimer = 1; > + break; > + } > + > + return 0; > } > > -int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu) > +static int __cpuidle_register_driver(struct cpuidle_driver *drv) > { > int ret; > > - spin_lock(&cpuidle_driver_lock); > - ret = __cpuidle_register_driver(drv, cpu); > - spin_unlock(&cpuidle_driver_lock); > + if (!drv || !drv->state_count) > + return -EINVAL; > > - return ret; > -} > + if (cpuidle_disabled()) > + return -ENODEV; > > -void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu) > -{ > - spin_lock(&cpuidle_driver_lock); > - __cpuidle_unregister_driver(drv, cpu); > - spin_unlock(&cpuidle_driver_lock); > -} > + ret = __cpuidle_driver_init(drv); > + if (ret) > + return ret; > > -/** > - * cpuidle_register_driver - registers a driver > - * @drv: the driver > - */ > -int cpuidle_register_driver(struct cpuidle_driver *drv) > -{ > - int ret; > + ret = __cpuidle_set_driver(drv); > + if (ret) > + return ret; > > - spin_lock(&cpuidle_driver_lock); > - ret = __cpuidle_register_all_cpu_driver(drv); > - spin_unlock(&cpuidle_driver_lock); > + if (drv->bctimer) > + on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, > + (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); > > - return ret; > + return 0; > } > -EXPORT_SYMBOL_GPL(cpuidle_register_driver); > > /** > * cpuidle_unregister_driver - unregisters a driver > * @drv: the driver > */ > -void cpuidle_unregister_driver(struct cpuidle_driver *drv) > +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) > { > - spin_lock(&cpuidle_driver_lock); > - __cpuidle_unregister_all_cpu_driver(drv); > - spin_unlock(&cpuidle_driver_lock); > -} > -EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); > - > -#else > - > -static struct cpuidle_driver *cpuidle_curr_driver; > + if (WARN_ON(drv->refcnt > 0)) > + return; > > -static inline void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) > -{ > - cpuidle_curr_driver = drv; > -} > + if (drv->bctimer) { > + drv->bctimer = 0; > + on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, > + (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); > + } > > -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > -{ > - return cpuidle_curr_driver; > + __cpuidle_unset_driver(drv); > } > > /** > @@ -187,13 +160,11 @@ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > */ > int cpuidle_register_driver(struct cpuidle_driver *drv) > { > - int ret, cpu; > + int ret; > > - cpu = get_cpu(); > spin_lock(&cpuidle_driver_lock); > - ret = __cpuidle_register_driver(drv, cpu); > + ret = __cpuidle_register_driver(drv); > spin_unlock(&cpuidle_driver_lock); > - put_cpu(); > > return ret; > } > @@ -205,16 +176,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver); > */ > void cpuidle_unregister_driver(struct cpuidle_driver *drv) > { > - int cpu; > - > - cpu = get_cpu(); > spin_lock(&cpuidle_driver_lock); > - __cpuidle_unregister_driver(drv, cpu); > + __cpuidle_unregister_driver(drv); > spin_unlock(&cpuidle_driver_lock); > - put_cpu(); > } > EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); > -#endif > > /** > * cpuidle_get_driver - return the current driver > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 8f04062..0bc4b74 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -111,6 +111,9 @@ struct cpuidle_driver { > struct cpuidle_state states[CPUIDLE_STATE_MAX]; > int state_count; > int safe_state_index; > + > + /* the driver handles the cpus in cpumask */ > + struct cpumask *cpumask; > }; > > #ifdef CONFIG_CPU_IDLE > @@ -135,9 +138,6 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev); > extern int cpuidle_play_dead(void); > > extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev); > -extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu); > -extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu); > - > #else > static inline void disable_cpuidle(void) { } > static inline int cpuidle_idle_call(void) { return -ENODEV; } >
On 8 June 2013 03:23, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver > support. The code added a couple of new API to register the driver per cpu. > That led to some code complexity to handle the kernel config options when > the multiple driver support is enabled or not, which is not really necessary. > The code has to be compatible when the multiple driver support is not enabled, > and the multiple driver support has to be compatible with the old api. > > This patch removes this API, which is not yet used by any driver but needed > for the HMP cpuidle drivers which will come soon, and replaces its usage > by a cpumask pointer in the cpuidle driver structure telling what cpus are > handled by the driver. That let the API cpuidle_[un]register_driver to be used > for the multiple driver support and also the cpuidle_[un]register functions, > added recently in the cpuidle framework. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Sorry for jumping onto this thread so late :( The current code in cpuidle/driver.c contains two definitions of these routines: __cpuidle_get_cpu_driver() and __cpuidle_{set|unset}_driver(), enclosed withing #if/else.. These duplicate routines exist only to save some space where we don't have multiple drivers for a platform, right? So, that we don't waste up space for per-cpu variables for holding cpuidle_driver.. But what about multi platform kernels? This config option would be enabled then and we would finally run into the same problem again.. The worst side of this is: We will run separate code paths for a platform which doesn't have support for multiple drivers in the multiplatform kernel.. Even if it works, it looks a bit wrong design wise.. Either we can have a runtime variable in cpuidle_driver or somewhere else that will let us know if we want multiple drivers for our platform or not OR do the per-cpu stuff for everybody.. I was about to rewrite that stuff and send a patch for it, but then thought probably its better to discuss it first.. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2013 07:21 AM, Viresh Kumar wrote: > On 8 June 2013 03:23, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver >> support. The code added a couple of new API to register the driver per cpu. >> That led to some code complexity to handle the kernel config options when >> the multiple driver support is enabled or not, which is not really necessary. >> The code has to be compatible when the multiple driver support is not enabled, >> and the multiple driver support has to be compatible with the old api. >> >> This patch removes this API, which is not yet used by any driver but needed >> for the HMP cpuidle drivers which will come soon, and replaces its usage >> by a cpumask pointer in the cpuidle driver structure telling what cpus are >> handled by the driver. That let the API cpuidle_[un]register_driver to be used >> for the multiple driver support and also the cpuidle_[un]register functions, >> added recently in the cpuidle framework. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > Sorry for jumping onto this thread so late :( > The current code in cpuidle/driver.c contains two definitions of these routines: > __cpuidle_get_cpu_driver() and __cpuidle_{set|unset}_driver(), enclosed > withing #if/else.. > > These duplicate routines exist only to save some space where we don't > have multiple drivers for a platform, right? So, that we don't waste up > space for per-cpu variables for holding cpuidle_driver.. That's right. > But what about multi platform kernels? This config option would be enabled > then and we would finally run into the same problem again.. > > The worst side of this is: We will run separate code paths for a platform > which doesn't have support for multiple drivers in the multiplatform kernel.. > Even if it works, it looks a bit wrong design wise.. Where is it wrong in design ? If the multiple driver support is enabled in the kernel but the driver handles all the cpu, it works. > Either we can have a runtime variable in cpuidle_driver or somewhere else > that will let us know if we want multiple drivers for our platform or not > > OR > > do the per-cpu stuff for everybody.. I don't think it is worth to add more code complexity to save an extra small chunk of memory on ARM. If we don't want to support the multiple driver, the option is disabled and we use a single variable. That is the case for x86. If we want to enable it for multiple platforms support on ARM, then we need per-cpu and we lose a bit of per-cpu memory. -- Daniel
On 25 September 2013 20:32, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Where is it wrong in design ? If the multiple driver support is enabled > in the kernel but the driver handles all the cpu, it works. Yeah It works but I don't really support two separate code paths based on how kernel is compiled.. For example: Consider any ARM system which doesn't need multiple drivers support and so must be using the non-per-cpu variable for holding drivers pointer.. Now the same driver is included as part of a multiplatform kernel and now the same driver/platform is using the per-cpu variables for storing driver pointer.. Even sysfs will have files with name "driver" to print drivers name for CPUs.. I know it doesn't break anything but it didn't looked good design wise to me.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index c3a93fe..fdc432f 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -466,7 +466,7 @@ void cpuidle_unregister(struct cpuidle_driver *drv) int cpu; struct cpuidle_device *device; - for_each_possible_cpu(cpu) { + for_each_cpu(cpu, drv->cpumask) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_unregister_device(device); } @@ -498,7 +498,7 @@ int cpuidle_register(struct cpuidle_driver *drv, return ret; } - for_each_possible_cpu(cpu) { + for_each_cpu(cpu, drv->cpumask) { device = &per_cpu(cpuidle_dev, cpu); device->cpu = cpu; diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 8dfaaae..e75fa54 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -18,167 +18,140 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); -static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS -static void cpuidle_setup_broadcast_timer(void *arg) +static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); + +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { - int cpu = smp_processor_id(); - clockevents_notify((long)(arg), &cpu); + return per_cpu(cpuidle_drivers, cpu); } -static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu) +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) { - int i; - - drv->refcnt = 0; + int cpu; - for (i = drv->state_count - 1; i >= 0 ; i--) { + for_each_cpu(cpu, drv->cpumask) { - if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) + if (drv != __cpuidle_get_cpu_driver(cpu)) continue; - drv->bctimer = 1; - on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, - (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); - break; + per_cpu(cpuidle_drivers, cpu) = NULL; } } -static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) { - if (!drv || !drv->state_count) - return -EINVAL; - - if (cpuidle_disabled()) - return -ENODEV; + int cpu; - if (__cpuidle_get_cpu_driver(cpu)) - return -EBUSY; + for_each_cpu(cpu, drv->cpumask) { - __cpuidle_driver_init(drv, cpu); + if (__cpuidle_get_cpu_driver(cpu)) { + __cpuidle_unset_driver(drv); + return -EBUSY; + } - __cpuidle_set_cpu_driver(drv, cpu); + per_cpu(cpuidle_drivers, cpu) = drv; + } return 0; } -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu) -{ - if (drv != __cpuidle_get_cpu_driver(cpu)) - return; +#else - if (!WARN_ON(drv->refcnt > 0)) - __cpuidle_set_cpu_driver(NULL, cpu); +static struct cpuidle_driver *cpuidle_curr_driver; - if (drv->bctimer) { - drv->bctimer = 0; - on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, - (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); - } +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +{ + return cpuidle_curr_driver; } -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS +static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) +{ + if (cpuidle_curr_driver) + return -EBUSY; -static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); + cpuidle_curr_driver = drv; -static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) -{ - per_cpu(cpuidle_drivers, cpu) = drv; + return 0; } -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) { - return per_cpu(cpuidle_drivers, cpu); + if (drv == cpuidle_curr_driver) + cpuidle_curr_driver = NULL; } -static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv) +#endif + +static void cpuidle_setup_broadcast_timer(void *arg) { - int cpu; - for_each_present_cpu(cpu) - __cpuidle_unregister_driver(drv, cpu); + int cpu = smp_processor_id(); + clockevents_notify((long)(arg), &cpu); } -static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) +static int __cpuidle_driver_init(struct cpuidle_driver *drv) { - int ret = 0; - int i, cpu; + int i; - for_each_present_cpu(cpu) { - ret = __cpuidle_register_driver(drv, cpu); - if (ret) - break; - } + drv->refcnt = 0; - if (ret) - for_each_present_cpu(i) { - if (i == cpu) - break; - __cpuidle_unregister_driver(drv, i); - } + if (!drv->cpumask) + drv->cpumask = (struct cpumask *)cpu_possible_mask; + for (i = drv->state_count - 1; i >= 0 ; i--) { - return ret; + if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) + continue; + + drv->bctimer = 1; + break; + } + + return 0; } -int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu) +static int __cpuidle_register_driver(struct cpuidle_driver *drv) { int ret; - spin_lock(&cpuidle_driver_lock); - ret = __cpuidle_register_driver(drv, cpu); - spin_unlock(&cpuidle_driver_lock); + if (!drv || !drv->state_count) + return -EINVAL; - return ret; -} + if (cpuidle_disabled()) + return -ENODEV; -void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu) -{ - spin_lock(&cpuidle_driver_lock); - __cpuidle_unregister_driver(drv, cpu); - spin_unlock(&cpuidle_driver_lock); -} + ret = __cpuidle_driver_init(drv); + if (ret) + return ret; -/** - * cpuidle_register_driver - registers a driver - * @drv: the driver - */ -int cpuidle_register_driver(struct cpuidle_driver *drv) -{ - int ret; + ret = __cpuidle_set_driver(drv); + if (ret) + return ret; - spin_lock(&cpuidle_driver_lock); - ret = __cpuidle_register_all_cpu_driver(drv); - spin_unlock(&cpuidle_driver_lock); + if (drv->bctimer) + on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, + (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); - return ret; + return 0; } -EXPORT_SYMBOL_GPL(cpuidle_register_driver); /** * cpuidle_unregister_driver - unregisters a driver * @drv: the driver */ -void cpuidle_unregister_driver(struct cpuidle_driver *drv) +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) { - spin_lock(&cpuidle_driver_lock); - __cpuidle_unregister_all_cpu_driver(drv); - spin_unlock(&cpuidle_driver_lock); -} -EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); - -#else - -static struct cpuidle_driver *cpuidle_curr_driver; + if (WARN_ON(drv->refcnt > 0)) + return; -static inline void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) -{ - cpuidle_curr_driver = drv; -} + if (drv->bctimer) { + drv->bctimer = 0; + on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, + (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); + } -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) -{ - return cpuidle_curr_driver; + __cpuidle_unset_driver(drv); } /** @@ -187,13 +160,11 @@ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) */ int cpuidle_register_driver(struct cpuidle_driver *drv) { - int ret, cpu; + int ret; - cpu = get_cpu(); spin_lock(&cpuidle_driver_lock); - ret = __cpuidle_register_driver(drv, cpu); + ret = __cpuidle_register_driver(drv); spin_unlock(&cpuidle_driver_lock); - put_cpu(); return ret; } @@ -205,16 +176,11 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver); */ void cpuidle_unregister_driver(struct cpuidle_driver *drv) { - int cpu; - - cpu = get_cpu(); spin_lock(&cpuidle_driver_lock); - __cpuidle_unregister_driver(drv, cpu); + __cpuidle_unregister_driver(drv); spin_unlock(&cpuidle_driver_lock); - put_cpu(); } EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); -#endif /** * cpuidle_get_driver - return the current driver diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 8f04062..0bc4b74 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -111,6 +111,9 @@ struct cpuidle_driver { struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; + + /* the driver handles the cpus in cpumask */ + struct cpumask *cpumask; }; #ifdef CONFIG_CPU_IDLE @@ -135,9 +138,6 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev); extern int cpuidle_play_dead(void); extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev); -extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu); -extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu); - #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; }
Commit bf4d1b5ddb78f86078ac6ae0415802d5f0c68f92 brought the multiple driver support. The code added a couple of new API to register the driver per cpu. That led to some code complexity to handle the kernel config options when the multiple driver support is enabled or not, which is not really necessary. The code has to be compatible when the multiple driver support is not enabled, and the multiple driver support has to be compatible with the old api. This patch removes this API, which is not yet used by any driver but needed for the HMP cpuidle drivers which will come soon, and replaces its usage by a cpumask pointer in the cpuidle driver structure telling what cpus are handled by the driver. That let the API cpuidle_[un]register_driver to be used for the multiple driver support and also the cpuidle_[un]register functions, added recently in the cpuidle framework. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- [V4] - uninitialize per cpu driver variable on error - rollback indentation in cpuidle.h [V3] - moved the kerneldoc and comments to a separate patch [V2]: - fixed bad refcount check - inverted clockevent notify off order at unregister time drivers/cpuidle/cpuidle.c | 4 +- drivers/cpuidle/driver.c | 192 +++++++++++++++++++-------------------------- include/linux/cpuidle.h | 6 +- 3 files changed, 84 insertions(+), 118 deletions(-)