| Message ID | 20211027082237.26759-2-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: > > Only limited number of CPUHP_AP_ONLINE_DYN callbacks can be registered, > thus cpuhp_remove_state() should be invoked to release the resource when > it is not used. > > Fixes: fb1013a01673 ("intel_idle: Convert to hotplug state machine") > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/idle/intel_idle.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index ae9d8c43e6a5..e7f2a5f85bf9 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -1676,6 +1676,8 @@ static int intel_idle_cpu_online(unsigned int cpu) > return 0; > } > > +static enum cpuhp_state intel_idle_cpuhp_state; > + > /** > * intel_idle_cpuidle_unregister - unregister from cpuidle framework > */ > @@ -1683,6 +1685,8 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) > { > int i; > > + if (intel_idle_cpuhp_state > 0) > + cpuhp_remove_state(intel_idle_cpuhp_state); It would be more straightforward to do that directly in intel_idle_init(), because intel_idle_cpuhp_state could be a local variable in that function then. > for_each_online_cpu(i) > cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); > cpuidle_unregister_driver(drv); > @@ -1710,11 +1714,11 @@ static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv) > return retval; > } > > - retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", > - intel_idle_cpu_online, NULL); > - if (retval < 0) { > + intel_idle_cpuhp_state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > + "idle/intel:online", intel_idle_cpu_online, NULL); > + if (intel_idle_cpuhp_state < 0) { > intel_idle_cpuidle_unregister(drv); > - return retval; > + return intel_idle_cpuhp_state; > } > return 0; > } > -- > 2.17.1 >
On Wed, 2021-11-24 at 14:29 +0100, Rafael J. Wysocki wrote: > On Wed, Oct 27, 2021 at 10:07 AM Zhang Rui <rui.zhang@intel.com> > wrote: > > > > Only limited number of CPUHP_AP_ONLINE_DYN callbacks can be > > registered, > > thus cpuhp_remove_state() should be invoked to release the resource > > when > > it is not used. > > > > Fixes: fb1013a01673 ("intel_idle: Convert to hotplug state > > machine") > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > --- > > drivers/idle/intel_idle.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > > index ae9d8c43e6a5..e7f2a5f85bf9 100644 > > --- a/drivers/idle/intel_idle.c > > +++ b/drivers/idle/intel_idle.c > > @@ -1676,6 +1676,8 @@ static int intel_idle_cpu_online(unsigned int > > cpu) > > return 0; > > } > > > > +static enum cpuhp_state intel_idle_cpuhp_state; > > + > > /** > > * intel_idle_cpuidle_unregister - unregister from cpuidle > > framework > > */ > > @@ -1683,6 +1685,8 @@ static void __init > > intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) > > { > > int i; > > > > + if (intel_idle_cpuhp_state > 0) > > + cpuhp_remove_state(intel_idle_cpuhp_state); > > It would be more straightforward to do that directly in > intel_idle_init(), because intel_idle_cpuhp_state could be a local > variable in that function then. I rechecked the code. in intel_idle_init(), cpuhp_setup_state() is the last step, so either it succeeds and we never unload the driver, or it fails and the intel_idle_init() fails directly. So nothing needs to be fixed if we don't support intel_idle driver module unload. thanks, rui > > > for_each_online_cpu(i) > > cpuidle_unregister_device(per_cpu_ptr(intel_idle_cp > > uidle_devices, i)); > > cpuidle_unregister_driver(drv); > > @@ -1710,11 +1714,11 @@ static int __init > > intel_idle_cpuidle_register(struct cpuidle_driver *drv) > > return retval; > > } > > > > - retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > > "idle/intel:online", > > - intel_idle_cpu_online, NULL); > > - if (retval < 0) { > > + intel_idle_cpuhp_state = > > cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > > + "idle/intel:online", intel_idle_cpu_online, NULL); > > + if (intel_idle_cpuhp_state < 0) { > > intel_idle_cpuidle_unregister(drv); > > - return retval; > > + return intel_idle_cpuhp_state; > > } > > return 0; > > } > > -- > > 2.17.1 > >
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index ae9d8c43e6a5..e7f2a5f85bf9 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1676,6 +1676,8 @@ static int intel_idle_cpu_online(unsigned int cpu) return 0; } +static enum cpuhp_state intel_idle_cpuhp_state; + /** * intel_idle_cpuidle_unregister - unregister from cpuidle framework */ @@ -1683,6 +1685,8 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) { int i; + if (intel_idle_cpuhp_state > 0) + cpuhp_remove_state(intel_idle_cpuhp_state); for_each_online_cpu(i) cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i)); cpuidle_unregister_driver(drv); @@ -1710,11 +1714,11 @@ static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv) return retval; } - retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online", - intel_idle_cpu_online, NULL); - if (retval < 0) { + intel_idle_cpuhp_state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, + "idle/intel:online", intel_idle_cpu_online, NULL); + if (intel_idle_cpuhp_state < 0) { intel_idle_cpuidle_unregister(drv); - return retval; + return intel_idle_cpuhp_state; } return 0; }
Only limited number of CPUHP_AP_ONLINE_DYN callbacks can be registered, thus cpuhp_remove_state() should be invoked to release the resource when it is not used. Fixes: fb1013a01673 ("intel_idle: Convert to hotplug state machine") Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/idle/intel_idle.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)