[1/8] cpuidle: Remove governor module format

Message ID 1371042535-22630-1-git-send-email-daniel.lezcano@linaro.org
State Accepted
Commit 137b944e100278d696826cf25c83014ac17473fe
Headers show

Commit Message

Daniel Lezcano June 12, 2013, 1:08 p.m.
The governors are defined as module in the code, but the Kconfig options do not
allow to compile them as module. This is not really a problem but the init
order is: the cpuidle init functions (framework and driver) and then the
governors. That leads to some weirdness in the cpuidle framework because the
function cpuidle_register_device calls cpuidle_enable_device which in turns
fails at the first attempt because no governor is registered. When the
governor is registered, the framework calls cpuidle_enable_device again which
will invoke the __cpuidle_register_device function. Of course, in order to make
this to work, the return code of cpuidle_enable_device is not checked by the
caller in cpuidle_register_device.

Instead of having this cyclic call graph and relying on a positive side effect
of the hackish back and forth call to cpuidle_enable_device, let's fix the
init order for the governor in order to clean up the cpuidle_enable_device
function.

Remove the module init code and replaced it with postcore_initcall, so we have:

 * cpuidle framework : core_initcall
 * cpuidle governors : postcore_initcall
 * cpuidle drivers   : device_initcall

Remove exit module code as it is dead code (governors aren't compiled as
module).

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/ladder.c |   12 +-----------
 drivers/cpuidle/governors/menu.c   |   12 +-----------
 2 files changed, 2 insertions(+), 22 deletions(-)

Comments

Rafael J. Wysocki June 12, 2013, 10:53 p.m. | #1
On Wednesday, June 12, 2013 03:08:48 PM Daniel Lezcano wrote:
> The governors are defined as module in the code, but the Kconfig options do not
> allow to compile them as module. This is not really a problem but the init
> order is: the cpuidle init functions (framework and driver) and then the
> governors. That leads to some weirdness in the cpuidle framework because the
> function cpuidle_register_device calls cpuidle_enable_device which in turns
> fails at the first attempt because no governor is registered. When the
> governor is registered, the framework calls cpuidle_enable_device again which
> will invoke the __cpuidle_register_device function. Of course, in order to make
> this to work, the return code of cpuidle_enable_device is not checked by the
> caller in cpuidle_register_device.
> 
> Instead of having this cyclic call graph and relying on a positive side effect
> of the hackish back and forth call to cpuidle_enable_device, let's fix the
> init order for the governor in order to clean up the cpuidle_enable_device
> function.
> 
> Remove the module init code and replaced it with postcore_initcall, so we have:
> 
>  * cpuidle framework : core_initcall
>  * cpuidle governors : postcore_initcall
>  * cpuidle drivers   : device_initcall
> 
> Remove exit module code as it is dead code (governors aren't compiled as
> module).
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

That's something I'd like to receive much earlier in the cycle (around -rc2).

Honestly, I'm not sure if it's going to make it into 3.11 even if nobody has
any comments.

Thanks,
Rafael


> ---
>  drivers/cpuidle/governors/ladder.c |   12 +-----------
>  drivers/cpuidle/governors/menu.c   |   12 +-----------
>  2 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
> index 9b78405..9f08e8c 100644
> --- a/drivers/cpuidle/governors/ladder.c
> +++ b/drivers/cpuidle/governors/ladder.c
> @@ -192,14 +192,4 @@ static int __init init_ladder(void)
>  	return cpuidle_register_governor(&ladder_governor);
>  }
>  
> -/**
> - * exit_ladder - exits the governor
> - */
> -static void __exit exit_ladder(void)
> -{
> -	cpuidle_unregister_governor(&ladder_governor);
> -}
> -
> -MODULE_LICENSE("GPL");
> -module_init(init_ladder);
> -module_exit(exit_ladder);
> +postcore_initcall(init_ladder);
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index fe343a0..743138c 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -540,14 +540,4 @@ static int __init init_menu(void)
>  	return cpuidle_register_governor(&menu_governor);
>  }
>  
> -/**
> - * exit_menu - exits the governor
> - */
> -static void __exit exit_menu(void)
> -{
> -	cpuidle_unregister_governor(&menu_governor);
> -}
> -
> -MODULE_LICENSE("GPL");
> -module_init(init_menu);
> -module_exit(exit_menu);
> +postcore_initcall(init_menu);
>
Daniel Lezcano June 13, 2013, 11:57 a.m. | #2
On 06/13/2013 12:53 AM, Rafael J. Wysocki wrote:
> On Wednesday, June 12, 2013 03:08:48 PM Daniel Lezcano wrote:
>> The governors are defined as module in the code, but the Kconfig options do not
>> allow to compile them as module. This is not really a problem but the init
>> order is: the cpuidle init functions (framework and driver) and then the
>> governors. That leads to some weirdness in the cpuidle framework because the
>> function cpuidle_register_device calls cpuidle_enable_device which in turns
>> fails at the first attempt because no governor is registered. When the
>> governor is registered, the framework calls cpuidle_enable_device again which
>> will invoke the __cpuidle_register_device function. Of course, in order to make
>> this to work, the return code of cpuidle_enable_device is not checked by the
>> caller in cpuidle_register_device.
>>
>> Instead of having this cyclic call graph and relying on a positive side effect
>> of the hackish back and forth call to cpuidle_enable_device, let's fix the
>> init order for the governor in order to clean up the cpuidle_enable_device
>> function.
>>
>> Remove the module init code and replaced it with postcore_initcall, so we have:
>>
>>  * cpuidle framework : core_initcall
>>  * cpuidle governors : postcore_initcall
>>  * cpuidle drivers   : device_initcall
>>
>> Remove exit module code as it is dead code (governors aren't compiled as
>> module).
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> That's something I'd like to receive much earlier in the cycle (around -rc2).
> 
> Honestly, I'm not sure if it's going to make it into 3.11 even if nobody has
> any comments.

It is ok, choose what is the more convenient to stabilize the kernel.

>> ---
>>  drivers/cpuidle/governors/ladder.c |   12 +-----------
>>  drivers/cpuidle/governors/menu.c   |   12 +-----------
>>  2 files changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
>> index 9b78405..9f08e8c 100644
>> --- a/drivers/cpuidle/governors/ladder.c
>> +++ b/drivers/cpuidle/governors/ladder.c
>> @@ -192,14 +192,4 @@ static int __init init_ladder(void)
>>  	return cpuidle_register_governor(&ladder_governor);
>>  }
>>  
>> -/**
>> - * exit_ladder - exits the governor
>> - */
>> -static void __exit exit_ladder(void)
>> -{
>> -	cpuidle_unregister_governor(&ladder_governor);
>> -}
>> -
>> -MODULE_LICENSE("GPL");
>> -module_init(init_ladder);
>> -module_exit(exit_ladder);
>> +postcore_initcall(init_ladder);
>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>> index fe343a0..743138c 100644
>> --- a/drivers/cpuidle/governors/menu.c
>> +++ b/drivers/cpuidle/governors/menu.c
>> @@ -540,14 +540,4 @@ static int __init init_menu(void)
>>  	return cpuidle_register_governor(&menu_governor);
>>  }
>>  
>> -/**
>> - * exit_menu - exits the governor
>> - */
>> -static void __exit exit_menu(void)
>> -{
>> -	cpuidle_unregister_governor(&menu_governor);
>> -}
>> -
>> -MODULE_LICENSE("GPL");
>> -module_init(init_menu);
>> -module_exit(exit_menu);
>> +postcore_initcall(init_menu);
>>

Patch

diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
index 9b78405..9f08e8c 100644
--- a/drivers/cpuidle/governors/ladder.c
+++ b/drivers/cpuidle/governors/ladder.c
@@ -192,14 +192,4 @@  static int __init init_ladder(void)
 	return cpuidle_register_governor(&ladder_governor);
 }
 
-/**
- * exit_ladder - exits the governor
- */
-static void __exit exit_ladder(void)
-{
-	cpuidle_unregister_governor(&ladder_governor);
-}
-
-MODULE_LICENSE("GPL");
-module_init(init_ladder);
-module_exit(exit_ladder);
+postcore_initcall(init_ladder);
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index fe343a0..743138c 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -540,14 +540,4 @@  static int __init init_menu(void)
 	return cpuidle_register_governor(&menu_governor);
 }
 
-/**
- * exit_menu - exits the governor
- */
-static void __exit exit_menu(void)
-{
-	cpuidle_unregister_governor(&menu_governor);
-}
-
-MODULE_LICENSE("GPL");
-module_init(init_menu);
-module_exit(exit_menu);
+postcore_initcall(init_menu);