| Message ID | 20211027082237.26759-1-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/3] intel_idle: cleanup code for handling with cpuidle framework | expand |
On Wed, Oct 27, 2021 at 10:07 AM Zhang Rui <rui.zhang@intel.com> wrote: > > Introduce two helper functions and move all the code that deals with > the cpuidle framework into them. So what exactly is the benefit? The role of intel_idle_cpuidle_devices_uninit() was to contain the loop over CPUs and now it is combined with the driver unregistration which looks confusing to me, because this function is only called in one place in the error patch of intel_idle_init(). The existing code flow is easier to follow for me TBH and it is fewer lines of code. > No functional change in this patch. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/idle/intel_idle.c | 67 ++++++++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 29 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index e6c543b5ee1d..ae9d8c43e6a5 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -1677,14 +1677,46 @@ static int intel_idle_cpu_online(unsigned int cpu) > } > > /** > - * intel_idle_cpuidle_devices_uninit - Unregister all cpuidle devices. > + * intel_idle_cpuidle_unregister - unregister from cpuidle framework > */ > -static void __init intel_idle_cpuidle_devices_uninit(void) > +static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) > { > int i; > > for_each_online_cpu(i) > cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); > + cpuidle_unregister_driver(drv); > + free_percpu(intel_idle_cpuidle_devices); > +} > + > +/** > + * intel_idle_cpuidle_register - register to cpuidle framework > + */ > +static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv) > +{ > + int retval; > + > + intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); > + if (!intel_idle_cpuidle_devices) > + return -ENOMEM; > + > + retval = cpuidle_register_driver(drv); > + if (retval) { > + struct cpuidle_driver *drv = cpuidle_get_driver(); > + > + printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"), > + drv ? drv->name : "none"); > + free_percpu(intel_idle_cpuidle_devices); > + return retval; > + } > + > + retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", > + intel_idle_cpu_online, NULL); > + if (retval < 0) { > + intel_idle_cpuidle_unregister(drv); > + return retval; > + } > + return 0; > } > > static int __init intel_idle_init(void) > @@ -1740,37 +1772,14 @@ static int __init intel_idle_init(void) > pr_debug("v" INTEL_IDLE_VERSION " model 0x%X\n", > boot_cpu_data.x86_model); > > - intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); > - if (!intel_idle_cpuidle_devices) > - return -ENOMEM; > - > intel_idle_cpuidle_driver_init(&intel_idle_driver); > > - retval = cpuidle_register_driver(&intel_idle_driver); > - if (retval) { > - struct cpuidle_driver *drv = cpuidle_get_driver(); > - printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"), > - drv ? drv->name : "none"); > - goto init_driver_fail; > - } > - > - retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", > - intel_idle_cpu_online, NULL); > - if (retval < 0) > - goto hp_setup_fail; > - > - pr_debug("Local APIC timer is reliable in %s\n", > - boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1"); > + retval = intel_idle_cpuidle_register(&intel_idle_driver); > + if (!retval) > + pr_debug("Local APIC timer is reliable in %s\n", > + boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1"); > > - return 0; > - > -hp_setup_fail: > - intel_idle_cpuidle_devices_uninit(); > - cpuidle_unregister_driver(&intel_idle_driver); > -init_driver_fail: > - free_percpu(intel_idle_cpuidle_devices); > return retval; > - > } > device_initcall(intel_idle_init); > > -- > 2.17.1 >
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index e6c543b5ee1d..ae9d8c43e6a5 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1677,14 +1677,46 @@ static int intel_idle_cpu_online(unsigned int cpu) } /** - * intel_idle_cpuidle_devices_uninit - Unregister all cpuidle devices. + * intel_idle_cpuidle_unregister - unregister from cpuidle framework */ -static void __init intel_idle_cpuidle_devices_uninit(void) +static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) { int i; for_each_online_cpu(i) cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); + cpuidle_unregister_driver(drv); + free_percpu(intel_idle_cpuidle_devices); +} + +/** + * intel_idle_cpuidle_register - register to cpuidle framework + */ +static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv) +{ + int retval; + + intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); + if (!intel_idle_cpuidle_devices) + return -ENOMEM; + + retval = cpuidle_register_driver(drv); + if (retval) { + struct cpuidle_driver *drv = cpuidle_get_driver(); + + printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"), + drv ? drv->name : "none"); + free_percpu(intel_idle_cpuidle_devices); + return retval; + } + + retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", + intel_idle_cpu_online, NULL); + if (retval < 0) { + intel_idle_cpuidle_unregister(drv); + return retval; + } + return 0; } static int __init intel_idle_init(void) @@ -1740,37 +1772,14 @@ static int __init intel_idle_init(void) pr_debug("v" INTEL_IDLE_VERSION " model 0x%X\n", boot_cpu_data.x86_model); - intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); - if (!intel_idle_cpuidle_devices) - return -ENOMEM; - intel_idle_cpuidle_driver_init(&intel_idle_driver); - retval = cpuidle_register_driver(&intel_idle_driver); - if (retval) { - struct cpuidle_driver *drv = cpuidle_get_driver(); - printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"), - drv ? drv->name : "none"); - goto init_driver_fail; - } - - retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", - intel_idle_cpu_online, NULL); - if (retval < 0) - goto hp_setup_fail; - - pr_debug("Local APIC timer is reliable in %s\n", - boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1"); + retval = intel_idle_cpuidle_register(&intel_idle_driver); + if (!retval) + pr_debug("Local APIC timer is reliable in %s\n", + boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1"); - return 0; - -hp_setup_fail: - intel_idle_cpuidle_devices_uninit(); - cpuidle_unregister_driver(&intel_idle_driver); -init_driver_fail: - free_percpu(intel_idle_cpuidle_devices); return retval; - } device_initcall(intel_idle_init);
Introduce two helper functions and move all the code that deals with the cpuidle framework into them. No functional change in this patch. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/idle/intel_idle.c | 67 ++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 29 deletions(-)