Patchwork cpuidle: fix unremovable issue for module driver

login
register
mail settings
Submitter Dongsheng Wang
Date July 30, 2013, 6:55 a.m.
Message ID <1375167319-12821-1-git-send-email-dongsheng.wang@freescale.com>
Download mbox | patch
Permalink /patch/263206/
State Not Applicable
Headers show

Comments

Dongsheng Wang - July 30, 2013, 6:55 a.m.
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>
Daniel Lezcano - July 30, 2013, 9:28 a.m.
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);
>
Wang Dongsheng-B40534 - July 30, 2013, 10:48 a.m.
> -----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

>
Daniel Lezcano - July 30, 2013, 11:19 a.m.
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);
>>>
Rafael J. Wysocki - July 30, 2013, 1:33 p.m.
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
Rafael J. Wysocki - July 31, 2013, 11:05 p.m.
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

Patch

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