[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]

> -----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.

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

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

[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.