On 07/30/2013 12:48 PM, Wang Dongsheng-B40534 wrote:

quoted

-----Original Message-----
From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
Sent: Tuesday, July 30, 2013 5:28 PM
To: Wang Dongsheng-B40534
Cc: rjw@sisk.pl; linux-pm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] cpuidle: fix unremovable issue for module driver

On 07/30/2013 08:55 AM, Dongsheng Wang wrote:

quoted

From: Wang Dongsheng <redacted>

After __cpuidle_register_device, the cpu incs are added up, but decs
are not, thus the module refcount is not match. So the module "exit"
function can not be executed when we do remove operation. Move
module_put into __cpuidle_register_device to fix it.

Sorry, I still don't get it :/

register->module_get
unregister->module_put

you change it by:

register->module_get
register->module_put
unregister->none

which is wrong.

module_get->set per cpu incs=1
module_put->set per cpu decs=1

module_refcount-> incs - decs;

"unregister" usually call in module->exit function. 
But if module_refcount is not zero, the module->exit() cannot be executed.

Ok, IIUC, the refcount is decremented in the unregister function but
this one is never called because the refcount is not zero.

Funny, that means the module format was *never* used at all as it does
not work.

I am wondering if we shouldn't just remove the module support for
cpuidle. Rafael ?

See, kernel/module.c +874
delete_module()-->try_stop_module();

Thanks, I believe I understand the refcount mechanism.

Test Log:
root:~# rmmod cpuidle-e500
module_refcount: incs[9], decs[1]
rmmod: can't unload 'cpuidle_e500': Resource temporarily unavailable

quoted

Can you describe the problem you are facing ? (a bit more than "I can't
unload the module").

quoted

Signed-off-by: Wang Dongsheng <redacted>

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index d75040d..e964ada 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c

@@ -351,11 +351,8 @@ EXPORT_SYMBOL_GPL(cpuidle_disable_device);

 static void __cpuidle_unregister_device(struct cpuidle_device *dev)

{
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-
 	list_del(&dev->device_list);
 	per_cpu(cpuidle_devices, dev->cpu) = NULL;
-	module_put(drv->owner);
 }

 static int __cpuidle_device_init(struct cpuidle_device *dev) @@
-384,6 +381,8 @@ static int __cpuidle_register_device(struct

cpuidle_device *dev)

quoted

 	per_cpu(cpuidle_devices, dev->cpu) = dev;
 	list_add(&dev->device_list, &cpuidle_detected_devices);

+	module_put(drv->owner);
+
 	ret = cpuidle_coupled_register_device(dev);
 	if (ret) {
 		__cpuidle_unregister_device(dev);

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog