| Message ID | 1375167319-12821-1-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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); >
> -----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 >
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); >>>
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
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
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);