Re: [PATCH] cpuidle: fix unremovable issue for module driver

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 07/30/2013 12:48 PM, Wang Dongsheng-B40534 wrote:
> 
> 
>> -----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:
>>> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>>>
>>> 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
> 
>> Can you describe the problem you are facing ? (a bit more than "I can't
>> unload the module").
>>
>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>>>
>>> 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)
>>>  	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