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

[Greg KH <gregkh@linuxfoundation.org>]

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