[PATCH] PM / Domains: Add module ref count for each consumer

[PATCH] PM / Domains: Add module ref count for each consumer

[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

Re: [PATCH] PM / Domains: Add module ref count for each consumer

[Greg KH]

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

Re: [PATCH] PM / Domains: Add module ref count for each consumer

[Rafael J. Wysocki]

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
>

Re: [PATCH] PM / Domains: Add module ref count for each consumer

[Gustav Wiklander]

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

Re: [PATCH] PM / Domains: Add module ref count for each consumer

[Greg KH]

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