* [PATCH] PM / Domains: Add module ref count for each consumer @ 2020-06-10 14:39 Gustav Wiklander 2020-06-10 14:52 ` Greg KH 2020-06-10 14:57 ` Rafael J. Wysocki 0 siblings, 2 replies; 5+ messages in thread From: Gustav Wiklander @ 2020-06-10 14:39 UTC (permalink / raw) To: linux-pm Cc: rjw, khilman, ulf.hansson, len.brown, pavel, gregkh, kernel, Gustav Wiklander From: Gustav Wiklander <gustavwi@axis.com> Currently a pm_domain can be unloaded without regard for consumers. This patch adds a module dependecy for every registered consumer. Now a power domain driver can only be unloaded if no consumers are registered. Signed-off-by: Gustav Wiklander <gustavwi@axis.com> --- drivers/base/power/domain.c | 11 ++++++++++- include/linux/pm_domain.h | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 0a01df608849..80723f6d5e6b 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1499,11 +1499,18 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, if (IS_ERR(gpd_data)) return PTR_ERR(gpd_data); + if (!try_module_get(genpd->owner)) { + ret = -ENODEV; + goto out; + } + gpd_data->cpu = genpd_get_cpu(genpd, base_dev); ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; - if (ret) + if (ret) { + module_put(genpd->owner); goto out; + } genpd_lock(genpd); @@ -1579,6 +1586,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, genpd_free_dev_data(dev, gpd_data); + module_put(genpd->owner); + return 0; out: diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 9ec78ee53652..777c1b30e5af 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -9,6 +9,7 @@ #define _LINUX_PM_DOMAIN_H #include <linux/device.h> +#include <linux/module.h> #include <linux/mutex.h> #include <linux/pm.h> #include <linux/err.h> @@ -93,6 +94,7 @@ struct opp_table; struct generic_pm_domain { struct device dev; + struct module *owner; /* Module owner of the PM domain */ struct dev_pm_domain domain; /* PM domain operations */ struct list_head gpd_list_node; /* Node in the global PM domains list */ struct list_head master_links; /* Links with PM domain as a master */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PM / Domains: Add module ref count for each consumer 2020-06-10 14:39 [PATCH] PM / Domains: Add module ref count for each consumer Gustav Wiklander @ 2020-06-10 14:52 ` Greg KH 2020-06-10 17:24 ` Gustav Wiklander 2020-06-10 14:57 ` Rafael J. Wysocki 1 sibling, 1 reply; 5+ messages in thread From: Greg KH @ 2020-06-10 14:52 UTC (permalink / raw) To: Gustav Wiklander Cc: linux-pm, rjw, khilman, ulf.hansson, len.brown, pavel, kernel, Gustav Wiklander On Wed, Jun 10, 2020 at 04:39:43PM +0200, Gustav Wiklander wrote: > From: Gustav Wiklander <gustavwi@axis.com> > > Currently a pm_domain can be unloaded without regard for consumers. > This patch adds a module dependecy for every registered consumer. > Now a power domain driver can only be unloaded if no consumers are > registered. What is the problem with doing this? Shouldn't when a power domain is unregistered, the consumers are properly torn down? Some subsystems allow you to unload a module at any point in time, and properly clean things up. What is the problem today that you are trying to solve with this (remember, removing modules only happens by developers, no real-world system ever automatically onloads a module.) > > Signed-off-by: Gustav Wiklander <gustavwi@axis.com> > --- > drivers/base/power/domain.c | 11 ++++++++++- > include/linux/pm_domain.h | 2 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 0a01df608849..80723f6d5e6b 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1499,11 +1499,18 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > if (IS_ERR(gpd_data)) > return PTR_ERR(gpd_data); > > + if (!try_module_get(genpd->owner)) { > + ret = -ENODEV; > + goto out; > + } > + > gpd_data->cpu = genpd_get_cpu(genpd, base_dev); > > ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; > - if (ret) > + if (ret) { > + module_put(genpd->owner); > goto out; > + } > > genpd_lock(genpd); > > @@ -1579,6 +1586,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > > genpd_free_dev_data(dev, gpd_data); > > + module_put(genpd->owner); > + > return 0; > > out: > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 9ec78ee53652..777c1b30e5af 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -9,6 +9,7 @@ > #define _LINUX_PM_DOMAIN_H > > #include <linux/device.h> > +#include <linux/module.h> > #include <linux/mutex.h> > #include <linux/pm.h> > #include <linux/err.h> > @@ -93,6 +94,7 @@ struct opp_table; > > struct generic_pm_domain { > struct device dev; > + struct module *owner; /* Module owner of the PM domain */ But you did not actually set the owner field anywhere :( Make this an automatic thing, look how functions like usb_register_driver() and friends to it so that you do not have to go around and try to add it by hand to every driver. And then go back in a year and fix up the remaining ones you missed. And then in another year... thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM / Domains: Add module ref count for each consumer 2020-06-10 14:52 ` Greg KH @ 2020-06-10 17:24 ` Gustav Wiklander 2020-07-05 9:37 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Gustav Wiklander @ 2020-06-10 17:24 UTC (permalink / raw) To: Greg KH, Gustav Wiklander Cc: linux-pm, rjw, khilman, ulf.hansson, len.brown, pavel, kernel On 6/10/20 4:52 PM, Greg KH wrote: > On Wed, Jun 10, 2020 at 04:39:43PM +0200, Gustav Wiklander wrote: >> From: Gustav Wiklander <gustavwi@axis.com> >> >> Currently a pm_domain can be unloaded without regard for consumers. >> This patch adds a module dependecy for every registered consumer. >> Now a power domain driver can only be unloaded if no consumers are >> registered. > > What is the problem with doing this? Shouldn't when a power domain is > unregistered, the consumers are properly torn down? Some subsystems > allow you to unload a module at any point in time, and properly clean > things up. What is the problem today that you are trying to solve with > this (remember, removing modules only happens by developers, no > real-world system ever automatically onloads a module.) > > Hi Greg and Rafael, Thanks for the quick reply. PM domains shall call pm_genpd_remove at removal. As described in the definition pm_genpd_remove will fail if any device is still associated to it. *However*, at module removal the kernel ignores device removal failure and still unloads the powerdomain module. I am currently working on a PM driver and a couple of PM consumers. For quick development I load and unload them. If I unload the PM provider before the PM consumers the kernel will crash when unloading the PM consumer because callbacks trigger accesses to struct generic_pm_domain *genpd which was allocated by the PM provider driver. Secondly after unloading the PM domain the PM consumers are in an undefined state and could very well crash because they assume power is ON and do not handle unresponsive hardware. Best regards Gustav Wiklander [ 710.239233][ T267] Unable to handle kernel paging request at virtual address ffffff8000bb8048 [ 710.239265][ T267] Mem abort info: [ 710.239291][ T267] ESR = 0x86000007 [ 710.239318][ T267] Exception class = IABT (current EL), IL = 32 bits [ 710.239348][ T267] SET = 0, FnV = 0 [ 710.239375][ T267] EA = 0, S1PTW = 0 [ 710.239404][ T267] swapper pgtable: 4k pages, 39-bit VAs, pgdp = ffffff800915d000 [ 710.239436][ T267] [ffffff8000bb8048] pgd=00000000bfffe003, pud=00000000bfffe003, pmd=0000000088d58003, pte=0000000000000000 [ 710.239492][ T267] Internal error: Oops: 86000007 [#1] PREEMPT SMP [ 710.239515][ T267] Modules linked in: test_power_consumer(O-) [last unloaded: test_power_provider] [ 710.239571][ T267] Process rmmod (pid: 267, stack limit = 0xffffffc008fcc000) [ 710.239600][ T267] CPU: 0 PID: 267 Comm: rmmod Kdump: loaded Tainted: G O 4.19.110-axis8-devel #1 [ 710.239629][ T267] Hardware name: AXIS Wombat Virtual Machine (DT) [ 710.239655][ T267] pstate: 60400005 (nZCv daif +PAN -UAO) [ 710.239678][ T267] pc : 0xffffff8000bb8048 [ 710.239704][ T267] lr : genpd_power_on.part.0+0xd0/0x290 [ 710.239726][ T267] sp : ffffffc008fcfb60 [ 710.239747][ T267] x29: ffffffc008fcfb60 x28: ffffffc03fbce040 [ 710.239776][ T267] x27: ffffffc0091eabb0 x26: ffffffc0091ea7b0 [ 710.239804][ T267] x25: ffffffc0091ea880 x24: 0000000000000000 [ 710.239832][ T267] x23: 000000a55d7d66d0 x22: ffffffc0091eaca8 [ 710.239861][ T267] x21: ffffffc0091ea880 x20: ffffffc0091eac88 [ 710.239889][ T267] x19: 0000000000000000 x18: 0000000000000000 [ 710.239917][ T267] x17: 0000000000000000 x16: 0000000000000000 [ 710.239945][ T267] x15: ffffffc03f8d1510 x14: ffffffc03f37bcb0 [ 710.239973][ T267] x13: 0000000000000000 x12: 0000000000000025 [ 710.240002][ T267] x11: ffffffc03f37bb60 x10: 0000000000000040 [ 710.240030][ T267] x9 : ffffffc03f8d1510 x8 : 0000000000000001 [ 710.240058][ T267] x7 : 0000000000000000 x6 : 0000000a56772fe6 [ 710.240086][ T267] x5 : 00ffffffffffffff x4 : 0006dac2c0000000 [ 710.240114][ T267] x3 : 0000000000000017 x2 : 0000000000001c1c [ 710.240142][ T267] x1 : ffffff8000bb8048 x0 : ffffffc0091ea880 [ 710.240169][ T267] Call trace: [ 710.240190][ T267] 0xffffff8000bb8048 [ 710.240216][ T267] genpd_runtime_resume+0xc8/0x288 [ 710.240242][ T267] __rpm_callback+0xe4/0x230 [ 710.240266][ T267] rpm_callback+0x34/0x98 [ 710.240291][ T267] rpm_resume+0x5e0/0x7e8 [ 710.240315][ T267] __pm_runtime_resume+0x64/0xa8 [ 710.240340][ T267] device_release_driver_internal+0x158/0x238 [ 710.240366][ T267] driver_detach+0x8c/0xc8 [ 710.240389][ T267] bus_remove_driver+0x78/0xe0 [ 710.240414][ T267] driver_unregister+0x34/0x60 [ 710.240439][ T267] platform_driver_unregister+0x20/0x30 [ 710.240467][ T267] modexit+0x24/0xd14 [test_power_consumer] [ 710.240494][ T267] __arm64_sys_delete_module+0x168/0x218 >> >> Signed-off-by: Gustav Wiklander <gustavwi@axis.com> >> --- >> drivers/base/power/domain.c | 11 ++++++++++- >> include/linux/pm_domain.h | 2 ++ >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 0a01df608849..80723f6d5e6b 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1499,11 +1499,18 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, >> if (IS_ERR(gpd_data)) >> return PTR_ERR(gpd_data); >> >> + if (!try_module_get(genpd->owner)) { >> + ret = -ENODEV; >> + goto out; >> + } >> + >> gpd_data->cpu = genpd_get_cpu(genpd, base_dev); >> >> ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; >> - if (ret) >> + if (ret) { >> + module_put(genpd->owner); >> goto out; >> + } >> >> genpd_lock(genpd); >> >> @@ -1579,6 +1586,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, >> >> genpd_free_dev_data(dev, gpd_data); >> >> + module_put(genpd->owner); >> + >> return 0; >> >> out: >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 9ec78ee53652..777c1b30e5af 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -9,6 +9,7 @@ >> #define _LINUX_PM_DOMAIN_H >> >> #include <linux/device.h> >> +#include <linux/module.h> >> #include <linux/mutex.h> >> #include <linux/pm.h> >> #include <linux/err.h> >> @@ -93,6 +94,7 @@ struct opp_table; >> >> struct generic_pm_domain { >> struct device dev; >> + struct module *owner; /* Module owner of the PM domain */ > > But you did not actually set the owner field anywhere :( > > Make this an automatic thing, look how functions like > usb_register_driver() and friends to it so that you do not have to go > around and try to add it by hand to every driver. And then go back in a > year and fix up the remaining ones you missed. And then in another > year... > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM / Domains: Add module ref count for each consumer 2020-06-10 17:24 ` Gustav Wiklander @ 2020-07-05 9:37 ` Greg KH 0 siblings, 0 replies; 5+ messages in thread From: Greg KH @ 2020-07-05 9:37 UTC (permalink / raw) To: Gustav Wiklander Cc: Gustav Wiklander, linux-pm, rjw, khilman, ulf.hansson, len.brown, pavel, kernel On Wed, Jun 10, 2020 at 07:24:02PM +0200, Gustav Wiklander wrote: > On 6/10/20 4:52 PM, Greg KH wrote: > > On Wed, Jun 10, 2020 at 04:39:43PM +0200, Gustav Wiklander wrote: > > > From: Gustav Wiklander <gustavwi@axis.com> > > > > > > Currently a pm_domain can be unloaded without regard for consumers. > > > This patch adds a module dependecy for every registered consumer. > > > Now a power domain driver can only be unloaded if no consumers are > > > registered. > > > > What is the problem with doing this? Shouldn't when a power domain is > > unregistered, the consumers are properly torn down? Some subsystems > > allow you to unload a module at any point in time, and properly clean > > things up. What is the problem today that you are trying to solve with > > this (remember, removing modules only happens by developers, no > > real-world system ever automatically onloads a module.) > > > > > Hi Greg and Rafael, > > Thanks for the quick reply. > > PM domains shall call pm_genpd_remove at removal. As described in the > definition pm_genpd_remove will fail if any device is still associated to > it. *However*, at module removal the kernel ignores device removal failure > and still unloads the powerdomain module. So shouldn't the driver that controls that device be fixed up to properly remove the association first? Is there any in-kernel code that has this problem today? Adding module reference logic for an operation that has to be initiated by a developer only, seems like it's not really needed. thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM / Domains: Add module ref count for each consumer 2020-06-10 14:39 [PATCH] PM / Domains: Add module ref count for each consumer Gustav Wiklander 2020-06-10 14:52 ` Greg KH @ 2020-06-10 14:57 ` Rafael J. Wysocki 1 sibling, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2020-06-10 14:57 UTC (permalink / raw) To: Gustav Wiklander Cc: Linux PM, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, Len Brown, Pavel Machek, Greg Kroah-Hartman, kernel, Gustav Wiklander On Wed, Jun 10, 2020 at 4:39 PM Gustav Wiklander <gustav.wiklander@axis.com> wrote: > > From: Gustav Wiklander <gustavwi@axis.com> > > Currently a pm_domain can be unloaded without regard for consumers. > This patch adds a module dependecy for every registered consumer. > Now a power domain driver can only be unloaded if no consumers are > registered. > > Signed-off-by: Gustav Wiklander <gustavwi@axis.com> The genpd code is not modular, though. What problem exactly are you trying to address? > --- > drivers/base/power/domain.c | 11 ++++++++++- > include/linux/pm_domain.h | 2 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 0a01df608849..80723f6d5e6b 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1499,11 +1499,18 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > if (IS_ERR(gpd_data)) > return PTR_ERR(gpd_data); > > + if (!try_module_get(genpd->owner)) { > + ret = -ENODEV; > + goto out; > + } > + > gpd_data->cpu = genpd_get_cpu(genpd, base_dev); > > ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; > - if (ret) > + if (ret) { > + module_put(genpd->owner); > goto out; > + } > > genpd_lock(genpd); > > @@ -1579,6 +1586,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > > genpd_free_dev_data(dev, gpd_data); > > + module_put(genpd->owner); > + > return 0; > > out: > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 9ec78ee53652..777c1b30e5af 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -9,6 +9,7 @@ > #define _LINUX_PM_DOMAIN_H > > #include <linux/device.h> > +#include <linux/module.h> > #include <linux/mutex.h> > #include <linux/pm.h> > #include <linux/err.h> > @@ -93,6 +94,7 @@ struct opp_table; > > struct generic_pm_domain { > struct device dev; > + struct module *owner; /* Module owner of the PM domain */ > struct dev_pm_domain domain; /* PM domain operations */ > struct list_head gpd_list_node; /* Node in the global PM domains list */ > struct list_head master_links; /* Links with PM domain as a master */ > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-05 9:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-10 14:39 [PATCH] PM / Domains: Add module ref count for each consumer Gustav Wiklander 2020-06-10 14:52 ` Greg KH 2020-06-10 17:24 ` Gustav Wiklander 2020-07-05 9:37 ` Greg KH 2020-06-10 14:57 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).