[PATCH] cpuidle: fix unremovable issue for module driver

[PATCH] cpuidle: fix unremovable issue for module driver

[Dongsheng Wang]

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.

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);
-- 
1.8.0

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

[Daniel Lezcano]

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.

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

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

[Wang Dongsheng-B40534]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRGFuaWVsIExlemNhbm8g
W21haWx0bzpkYW5pZWwubGV6Y2Fub0BsaW5hcm8ub3JnXQ0KPiBTZW50OiBUdWVzZGF5LCBKdWx5
IDMwLCAyMDEzIDU6MjggUE0NCj4gVG86IFdhbmcgRG9uZ3NoZW5nLUI0MDUzNA0KPiBDYzogcmp3
QHNpc2sucGw7IGxpbnV4LXBtQHZnZXIua2VybmVsLm9yZzsgbGludXhwcGMtZGV2QGxpc3RzLm96
bGFicy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSF0gY3B1aWRsZTogZml4IHVucmVtb3ZhYmxl
IGlzc3VlIGZvciBtb2R1bGUgZHJpdmVyDQo+IA0KPiBPbiAwNy8zMC8yMDEzIDA4OjU1IEFNLCBE
b25nc2hlbmcgV2FuZyB3cm90ZToNCj4gPiBGcm9tOiBXYW5nIERvbmdzaGVuZyA8ZG9uZ3NoZW5n
LndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPg0KPiA+IEFmdGVyIF9fY3B1aWRsZV9yZWdpc3Rlcl9k
ZXZpY2UsIHRoZSBjcHUgaW5jcyBhcmUgYWRkZWQgdXAsIGJ1dCBkZWNzDQo+ID4gYXJlIG5vdCwg
dGh1cyB0aGUgbW9kdWxlIHJlZmNvdW50IGlzIG5vdCBtYXRjaC4gU28gdGhlIG1vZHVsZSAiZXhp
dCINCj4gPiBmdW5jdGlvbiBjYW4gbm90IGJlIGV4ZWN1dGVkIHdoZW4gd2UgZG8gcmVtb3ZlIG9w
ZXJhdGlvbi4gTW92ZQ0KPiA+IG1vZHVsZV9wdXQgaW50byBfX2NwdWlkbGVfcmVnaXN0ZXJfZGV2
aWNlIHRvIGZpeCBpdC4NCj4gDQo+IFNvcnJ5LCBJIHN0aWxsIGRvbid0IGdldCBpdCA6Lw0KPiAN
Cj4gcmVnaXN0ZXItPm1vZHVsZV9nZXQNCj4gdW5yZWdpc3Rlci0+bW9kdWxlX3B1dA0KPiANCj4g
eW91IGNoYW5nZSBpdCBieToNCj4gDQo+IHJlZ2lzdGVyLT5tb2R1bGVfZ2V0DQo+IHJlZ2lzdGVy
LT5tb2R1bGVfcHV0DQo+IHVucmVnaXN0ZXItPm5vbmUNCj4gDQo+IHdoaWNoIGlzIHdyb25nLg0K
PiANCm1vZHVsZV9nZXQtPnNldCBwZXIgY3B1IGluY3M9MQ0KbW9kdWxlX3B1dC0+c2V0IHBlciBj
cHUgZGVjcz0xDQoNCm1vZHVsZV9yZWZjb3VudC0+IGluY3MgLSBkZWNzOw0KDQoidW5yZWdpc3Rl
ciIgdXN1YWxseSBjYWxsIGluIG1vZHVsZS0+ZXhpdCBmdW5jdGlvbi4gDQpCdXQgaWYgbW9kdWxl
X3JlZmNvdW50IGlzIG5vdCB6ZXJvLCB0aGUgbW9kdWxlLT5leGl0KCkgY2Fubm90IGJlIGV4ZWN1
dGVkLg0KDQpTZWUsIGtlcm5lbC9tb2R1bGUuYyArODc0DQpkZWxldGVfbW9kdWxlKCktLT50cnlf
c3RvcF9tb2R1bGUoKTsNCg0KVGVzdCBMb2c6DQpyb290On4jIHJtbW9kIGNwdWlkbGUtZTUwMA0K
bW9kdWxlX3JlZmNvdW50OiBpbmNzWzldLCBkZWNzWzFdDQpybW1vZDogY2FuJ3QgdW5sb2FkICdj
cHVpZGxlX2U1MDAnOiBSZXNvdXJjZSB0ZW1wb3JhcmlseSB1bmF2YWlsYWJsZQ0KDQo+IENhbiB5
b3UgZGVzY3JpYmUgdGhlIHByb2JsZW0geW91IGFyZSBmYWNpbmcgPyAoYSBiaXQgbW9yZSB0aGFu
ICJJIGNhbid0DQo+IHVubG9hZCB0aGUgbW9kdWxlIikuDQo+IA0KPiA+IFNpZ25lZC1vZmYtYnk6
IFdhbmcgRG9uZ3NoZW5nIDxkb25nc2hlbmcud2FuZ0BmcmVlc2NhbGUuY29tPg0KPiA+DQo+ID4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvY3B1aWRsZS9jcHVpZGxlLmMgYi9kcml2ZXJzL2NwdWlkbGUv
Y3B1aWRsZS5jDQo+ID4gaW5kZXggZDc1MDQwZC4uZTk2NGFkYSAxMDA2NDQNCj4gPiAtLS0gYS9k
cml2ZXJzL2NwdWlkbGUvY3B1aWRsZS5jDQo+ID4gKysrIGIvZHJpdmVycy9jcHVpZGxlL2NwdWlk
bGUuYw0KPiA+IEBAIC0zNTEsMTEgKzM1MSw4IEBAIEVYUE9SVF9TWU1CT0xfR1BMKGNwdWlkbGVf
ZGlzYWJsZV9kZXZpY2UpOw0KPiA+DQo+ID4gIHN0YXRpYyB2b2lkIF9fY3B1aWRsZV91bnJlZ2lz
dGVyX2RldmljZShzdHJ1Y3QgY3B1aWRsZV9kZXZpY2UgKmRldikNCj4gPiB7DQo+ID4gLQlzdHJ1
Y3QgY3B1aWRsZV9kcml2ZXIgKmRydiA9IGNwdWlkbGVfZ2V0X2NwdV9kcml2ZXIoZGV2KTsNCj4g
PiAtDQo+ID4gIAlsaXN0X2RlbCgmZGV2LT5kZXZpY2VfbGlzdCk7DQo+ID4gIAlwZXJfY3B1KGNw
dWlkbGVfZGV2aWNlcywgZGV2LT5jcHUpID0gTlVMTDsNCj4gPiAtCW1vZHVsZV9wdXQoZHJ2LT5v
d25lcik7DQo+ID4gIH0NCj4gPg0KPiA+ICBzdGF0aWMgaW50IF9fY3B1aWRsZV9kZXZpY2VfaW5p
dChzdHJ1Y3QgY3B1aWRsZV9kZXZpY2UgKmRldikgQEANCj4gPiAtMzg0LDYgKzM4MSw4IEBAIHN0
YXRpYyBpbnQgX19jcHVpZGxlX3JlZ2lzdGVyX2RldmljZShzdHJ1Y3QNCj4gY3B1aWRsZV9kZXZp
Y2UgKmRldikNCj4gPiAgCXBlcl9jcHUoY3B1aWRsZV9kZXZpY2VzLCBkZXYtPmNwdSkgPSBkZXY7
DQo+ID4gIAlsaXN0X2FkZCgmZGV2LT5kZXZpY2VfbGlzdCwgJmNwdWlkbGVfZGV0ZWN0ZWRfZGV2
aWNlcyk7DQo+ID4NCj4gPiArCW1vZHVsZV9wdXQoZHJ2LT5vd25lcik7DQo+ID4gKw0KPiA+ICAJ
cmV0ID0gY3B1aWRsZV9jb3VwbGVkX3JlZ2lzdGVyX2RldmljZShkZXYpOw0KPiA+ICAJaWYgKHJl
dCkgew0KPiA+ICAJCV9fY3B1aWRsZV91bnJlZ2lzdGVyX2RldmljZShkZXYpOw0KPiA+DQo+IA0K
PiANCj4gLS0NCj4gIDxodHRwOi8vd3d3LmxpbmFyby5vcmcvPiBMaW5hcm8ub3JnIOKUgiBPcGVu
IHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MNCj4gDQo+IEZvbGxvdyBMaW5hcm86ICA8aHR0
cDovL3d3dy5mYWNlYm9vay5jb20vcGFnZXMvTGluYXJvPiBGYWNlYm9vayB8DQo+IDxodHRwOi8v
dHdpdHRlci5jb20vIyEvbGluYXJvb3JnPiBUd2l0dGVyIHwNCj4gPGh0dHA6Ly93d3cubGluYXJv
Lm9yZy9saW5hcm8tYmxvZy8+IEJsb2cNCj4gDQoNCg==

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

[Daniel Lezcano]

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

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

[Rafael J. Wysocki]

On Tuesday, July 30, 2013 01:19:46 PM Daniel Lezcano wrote:
> 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 ?

That would be the simplest thing to do and possibly the most correct one too,
but I need to double check how inte_idle/ACPI idle interactions depend on that.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Rafael J. Wysocki]

On Tuesday, July 30, 2013 03:33:44 PM Rafael J. Wysocki wrote:
> On Tuesday, July 30, 2013 01:19:46 PM Daniel Lezcano wrote:
> > 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 ?
> 
> That would be the simplest thing to do and possibly the most correct one too,
> but I need to double check how inte_idle/ACPI idle interactions depend on that.

As I thought, the ordering between intel_idle and the ACPI processor driver's
idle part depends on the latter being modular.

Also I'm not sure if the core is at fault here.  It evidently expects that
drivers will be registered before devices, so the driver module cannot be
released as long as there are any active devices.  This sounds logical from
the correctness viewpoint.

However, from the usability viewpoint it is not very convenient.  It would be
nicer if the driver could be unregistered and registered while devices are
there, but I'm afraid that would require some code reorganization.

The $subject patch isn't a correct change anyway in my opinion, because it
tries to kind of sidestep the core's assumptions.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.