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

[PATCHv2] 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>
---
Automated setting genpd->owner when calling pm_genpd_init.
Similar to how usb_register_driver does it.

 drivers/base/power/domain.c | 22 +++++++++++++++++-----
 include/linux/pm_domain.h   | 10 ++++++++--
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0a01df608849..70c8b59bfed9 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:
@@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
 }
 
 /**
- * pm_genpd_init - Initialize a generic I/O PM domain object.
+ * __pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
  * @gov: PM domain governor to associate with the domain (may be NULL).
  * @is_off: Initial value of the domain's power_is_off field.
+ * @owner: module owner of this power domain object.
  *
  * Returns 0 on successful initialization, else a negative error code.
  */
-int pm_genpd_init(struct generic_pm_domain *genpd,
-		  struct dev_power_governor *gov, bool is_off)
+int __pm_genpd_init(struct generic_pm_domain *genpd,
+		  struct dev_power_governor *gov, bool is_off,
+		  struct module *owner)
 {
 	int ret;
 
@@ -1777,6 +1788,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
+	genpd->owner = owner;
 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
 	genpd->device_count = 0;
 	genpd->max_off_time_ns = -1;
@@ -1831,7 +1843,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pm_genpd_init);
+EXPORT_SYMBOL_GPL(__pm_genpd_init);
 
 static int genpd_remove(struct generic_pm_domain *genpd)
 {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 9ec78ee53652..21a3183e00bc 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 */
@@ -200,8 +202,12 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *subdomain);
 int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 			      struct generic_pm_domain *subdomain);
-int pm_genpd_init(struct generic_pm_domain *genpd,
-		  struct dev_power_governor *gov, bool is_off);
+int __pm_genpd_init(struct generic_pm_domain *genpd,
+		    struct dev_power_governor *gov, bool is_off,
+		    struct module *module);
+#define pm_genpd_init(genpd, gov, is_off) \
+	__pm_genpd_init(genpd, gov, is_off, THIS_MODULE)
+
 int pm_genpd_remove(struct generic_pm_domain *genpd);
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
 
-- 
2.11.0

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

[Rafael J. Wysocki]

On Wed, Jun 10, 2020 at 9:32 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 is unclear.

What exactly do you mean by "pm_domain" here?  I think that this is a
driver providing the PM domain callbacks, right?  If so, that needs to
be stated directly.

> This patch adds a module dependecy for every registered consumer.

What does this mean?

> Now a power domain driver can only be unloaded if no consumers are
> registered.

I guess what you mean is that a module providing PM domain callbacks
cannot be unloaded as long as there are any devices in it, right?  The
assumption being that this is the module that initializes the domain.

What about subdomains?

> Signed-off-by: Gustav Wiklander <gustavwi@axis.com>

Ulf, any comments?

> ---
> Automated setting genpd->owner when calling pm_genpd_init.
> Similar to how usb_register_driver does it.
>
>  drivers/base/power/domain.c | 22 +++++++++++++++++-----
>  include/linux/pm_domain.h   | 10 ++++++++--
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0a01df608849..70c8b59bfed9 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:
> @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>  }
>
>  /**
> - * pm_genpd_init - Initialize a generic I/O PM domain object.
> + * __pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
>   * @gov: PM domain governor to associate with the domain (may be NULL).
>   * @is_off: Initial value of the domain's power_is_off field.
> + * @owner: module owner of this power domain object.
>   *
>   * Returns 0 on successful initialization, else a negative error code.
>   */
> -int pm_genpd_init(struct generic_pm_domain *genpd,
> -                 struct dev_power_governor *gov, bool is_off)
> +int __pm_genpd_init(struct generic_pm_domain *genpd,
> +                 struct dev_power_governor *gov, bool is_off,
> +                 struct module *owner)
>  {
>         int ret;
>
> @@ -1777,6 +1788,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->gov = gov;
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
> +       genpd->owner = owner;
>         genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
>         genpd->device_count = 0;
>         genpd->max_off_time_ns = -1;
> @@ -1831,7 +1843,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(pm_genpd_init);
> +EXPORT_SYMBOL_GPL(__pm_genpd_init);
>
>  static int genpd_remove(struct generic_pm_domain *genpd)
>  {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 9ec78ee53652..21a3183e00bc 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 */
> @@ -200,8 +202,12 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>                            struct generic_pm_domain *subdomain);
>  int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                               struct generic_pm_domain *subdomain);
> -int pm_genpd_init(struct generic_pm_domain *genpd,
> -                 struct dev_power_governor *gov, bool is_off);
> +int __pm_genpd_init(struct generic_pm_domain *genpd,
> +                   struct dev_power_governor *gov, bool is_off,
> +                   struct module *module);
> +#define pm_genpd_init(genpd, gov, is_off) \
> +       __pm_genpd_init(genpd, gov, is_off, THIS_MODULE)

I'd prefer this to be a static inline function.

> +
>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>
> --

Thanks!

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

[Ulf Hansson]

On Wed, 10 Jun 2020 at 21:32, 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.

According to the comments from Rafael, yes, this needs some further
clarifications.

Moreover, we also need to deal with module reference counters when
adding/removing subdomains. Also pointed out by Rafael.

>
> Signed-off-by: Gustav Wiklander <gustavwi@axis.com>
> ---
> Automated setting genpd->owner when calling pm_genpd_init.
> Similar to how usb_register_driver does it.
>
>  drivers/base/power/domain.c | 22 +++++++++++++++++-----
>  include/linux/pm_domain.h   | 10 ++++++++--
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0a01df608849..70c8b59bfed9 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:
> @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>  }
>
>  /**
> - * pm_genpd_init - Initialize a generic I/O PM domain object.
> + * __pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
>   * @gov: PM domain governor to associate with the domain (may be NULL).
>   * @is_off: Initial value of the domain's power_is_off field.
> + * @owner: module owner of this power domain object.
>   *
>   * Returns 0 on successful initialization, else a negative error code.
>   */
> -int pm_genpd_init(struct generic_pm_domain *genpd,
> -                 struct dev_power_governor *gov, bool is_off)
> +int __pm_genpd_init(struct generic_pm_domain *genpd,
> +                 struct dev_power_governor *gov, bool is_off,
> +                 struct module *owner)

Please drop this new interface altogether. Instead we can just let the
caller of pm_genpd_init() to assign genpd->owner, rather than passing
it as a parameter.

>  {
>         int ret;
>
> @@ -1777,6 +1788,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->gov = gov;
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
> +       genpd->owner = owner;
>         genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
>         genpd->device_count = 0;
>         genpd->max_off_time_ns = -1;
> @@ -1831,7 +1843,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(pm_genpd_init);
> +EXPORT_SYMBOL_GPL(__pm_genpd_init);
>
>  static int genpd_remove(struct generic_pm_domain *genpd)
>  {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 9ec78ee53652..21a3183e00bc 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 */
> @@ -200,8 +202,12 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>                            struct generic_pm_domain *subdomain);
>  int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                               struct generic_pm_domain *subdomain);
> -int pm_genpd_init(struct generic_pm_domain *genpd,
> -                 struct dev_power_governor *gov, bool is_off);
> +int __pm_genpd_init(struct generic_pm_domain *genpd,
> +                   struct dev_power_governor *gov, bool is_off,
> +                   struct module *module);
> +#define pm_genpd_init(genpd, gov, is_off) \
> +       __pm_genpd_init(genpd, gov, is_off, THIS_MODULE)
> +
>  int pm_genpd_remove(struct generic_pm_domain *genpd);
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>
> --
> 2.11.0
>

Kind regards
Uffe

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

[Greg Kroah-Hartman]

On Fri, Jul 10, 2020 at 12:18:57PM +0200, Ulf Hansson wrote:
> On Wed, 10 Jun 2020 at 21:32, 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.
> 
> According to the comments from Rafael, yes, this needs some further
> clarifications.
> 
> Moreover, we also need to deal with module reference counters when
> adding/removing subdomains. Also pointed out by Rafael.
> 
> >
> > Signed-off-by: Gustav Wiklander <gustavwi@axis.com>
> > ---
> > Automated setting genpd->owner when calling pm_genpd_init.
> > Similar to how usb_register_driver does it.
> >
> >  drivers/base/power/domain.c | 22 +++++++++++++++++-----
> >  include/linux/pm_domain.h   | 10 ++++++++--
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 0a01df608849..70c8b59bfed9 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:
> > @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
> >  }
> >
> >  /**
> > - * pm_genpd_init - Initialize a generic I/O PM domain object.
> > + * __pm_genpd_init - Initialize a generic I/O PM domain object.
> >   * @genpd: PM domain object to initialize.
> >   * @gov: PM domain governor to associate with the domain (may be NULL).
> >   * @is_off: Initial value of the domain's power_is_off field.
> > + * @owner: module owner of this power domain object.
> >   *
> >   * Returns 0 on successful initialization, else a negative error code.
> >   */
> > -int pm_genpd_init(struct generic_pm_domain *genpd,
> > -                 struct dev_power_governor *gov, bool is_off)
> > +int __pm_genpd_init(struct generic_pm_domain *genpd,
> > +                 struct dev_power_governor *gov, bool is_off,
> > +                 struct module *owner)
> 
> Please drop this new interface altogether. Instead we can just let the
> caller of pm_genpd_init() to assign genpd->owner, rather than passing
> it as a parameter.

No, I asked for this type of interface because it does not require any
developer to "remember" to set this value or not, and it does not
require you to go and fix the whole kernel.  This is the correct way to
do this, see the many other driver subsystems that do this today for
that reason.

thanks,

greg k-h

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

[Ulf Hansson]

On Fri, 10 Jul 2020 at 12:37, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jul 10, 2020 at 12:18:57PM +0200, Ulf Hansson wrote:
> > On Wed, 10 Jun 2020 at 21:32, 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.
> >
> > According to the comments from Rafael, yes, this needs some further
> > clarifications.
> >
> > Moreover, we also need to deal with module reference counters when
> > adding/removing subdomains. Also pointed out by Rafael.
> >
> > >
> > > Signed-off-by: Gustav Wiklander <gustavwi@axis.com>
> > > ---
> > > Automated setting genpd->owner when calling pm_genpd_init.
> > > Similar to how usb_register_driver does it.
> > >
> > >  drivers/base/power/domain.c | 22 +++++++++++++++++-----
> > >  include/linux/pm_domain.h   | 10 ++++++++--
> > >  2 files changed, 25 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > index 0a01df608849..70c8b59bfed9 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:
> > > @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
> > >  }
> > >
> > >  /**
> > > - * pm_genpd_init - Initialize a generic I/O PM domain object.
> > > + * __pm_genpd_init - Initialize a generic I/O PM domain object.
> > >   * @genpd: PM domain object to initialize.
> > >   * @gov: PM domain governor to associate with the domain (may be NULL).
> > >   * @is_off: Initial value of the domain's power_is_off field.
> > > + * @owner: module owner of this power domain object.
> > >   *
> > >   * Returns 0 on successful initialization, else a negative error code.
> > >   */
> > > -int pm_genpd_init(struct generic_pm_domain *genpd,
> > > -                 struct dev_power_governor *gov, bool is_off)
> > > +int __pm_genpd_init(struct generic_pm_domain *genpd,
> > > +                 struct dev_power_governor *gov, bool is_off,
> > > +                 struct module *owner)
> >
> > Please drop this new interface altogether. Instead we can just let the
> > caller of pm_genpd_init() to assign genpd->owner, rather than passing
> > it as a parameter.
>
> No, I asked for this type of interface because it does not require any
> developer to "remember" to set this value or not, and it does not
> require you to go and fix the whole kernel.  This is the correct way to
> do this, see the many other driver subsystems that do this today for
> that reason.

Well, in many cases I would agree with you, but not for genpd.

We have and are still, continuously finding new configurations that
are needed for a genpd. And we don't want a new in-parameter to be
added each time that happens.

What I have been thinking of, is to introduce a "genpd init data
struct" that we can pass as an in-parameter at registration point
instead. Although, I think that means a bit more work and it would not
be fair to force Gustav to implement this.

Kind regards
Uffe

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

[Greg Kroah-Hartman]

On Fri, Jul 10, 2020 at 02:01:15PM +0200, Ulf Hansson wrote:
> On Fri, 10 Jul 2020 at 12:37, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jul 10, 2020 at 12:18:57PM +0200, Ulf Hansson wrote:
> > > On Wed, 10 Jun 2020 at 21:32, 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.
> > >
> > > According to the comments from Rafael, yes, this needs some further
> > > clarifications.
> > >
> > > Moreover, we also need to deal with module reference counters when
> > > adding/removing subdomains. Also pointed out by Rafael.
> > >
> > > >
> > > > Signed-off-by: Gustav Wiklander <gustavwi@axis.com>
> > > > ---
> > > > Automated setting genpd->owner when calling pm_genpd_init.
> > > > Similar to how usb_register_driver does it.
> > > >
> > > >  drivers/base/power/domain.c | 22 +++++++++++++++++-----
> > > >  include/linux/pm_domain.h   | 10 ++++++++--
> > > >  2 files changed, 25 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > > index 0a01df608849..70c8b59bfed9 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:
> > > > @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
> > > >  }
> > > >
> > > >  /**
> > > > - * pm_genpd_init - Initialize a generic I/O PM domain object.
> > > > + * __pm_genpd_init - Initialize a generic I/O PM domain object.
> > > >   * @genpd: PM domain object to initialize.
> > > >   * @gov: PM domain governor to associate with the domain (may be NULL).
> > > >   * @is_off: Initial value of the domain's power_is_off field.
> > > > + * @owner: module owner of this power domain object.
> > > >   *
> > > >   * Returns 0 on successful initialization, else a negative error code.
> > > >   */
> > > > -int pm_genpd_init(struct generic_pm_domain *genpd,
> > > > -                 struct dev_power_governor *gov, bool is_off)
> > > > +int __pm_genpd_init(struct generic_pm_domain *genpd,
> > > > +                 struct dev_power_governor *gov, bool is_off,
> > > > +                 struct module *owner)
> > >
> > > Please drop this new interface altogether. Instead we can just let the
> > > caller of pm_genpd_init() to assign genpd->owner, rather than passing
> > > it as a parameter.
> >
> > No, I asked for this type of interface because it does not require any
> > developer to "remember" to set this value or not, and it does not
> > require you to go and fix the whole kernel.  This is the correct way to
> > do this, see the many other driver subsystems that do this today for
> > that reason.
> 
> Well, in many cases I would agree with you, but not for genpd.
> 
> We have and are still, continuously finding new configurations that
> are needed for a genpd. And we don't want a new in-parameter to be
> added each time that happens.

THIS_MODULE is "special", other config options you need should go in a
structure as you say.  This way we get the compiler to fix the option to
always be the correct one, no need for a developer to remember it,
totally different from all other driver/subsystem options.

thanks,

greg k-h

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

[Ulf Hansson]

On Fri, 10 Jul 2020 at 14:37, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jul 10, 2020 at 02:01:15PM +0200, Ulf Hansson wrote:
> > On Fri, 10 Jul 2020 at 12:37, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jul 10, 2020 at 12:18:57PM +0200, Ulf Hansson wrote:
> > > > On Wed, 10 Jun 2020 at 21:32, 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.
> > > >
> > > > According to the comments from Rafael, yes, this needs some further
> > > > clarifications.
> > > >
> > > > Moreover, we also need to deal with module reference counters when
> > > > adding/removing subdomains. Also pointed out by Rafael.
> > > >
> > > > >
> > > > > Signed-off-by: Gustav Wiklander <gustavwi@axis.com>
> > > > > ---
> > > > > Automated setting genpd->owner when calling pm_genpd_init.
> > > > > Similar to how usb_register_driver does it.
> > > > >
> > > > >  drivers/base/power/domain.c | 22 +++++++++++++++++-----
> > > > >  include/linux/pm_domain.h   | 10 ++++++++--
> > > > >  2 files changed, 25 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > > > index 0a01df608849..70c8b59bfed9 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:
> > > > > @@ -1755,15 +1764,17 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * pm_genpd_init - Initialize a generic I/O PM domain object.
> > > > > + * __pm_genpd_init - Initialize a generic I/O PM domain object.
> > > > >   * @genpd: PM domain object to initialize.
> > > > >   * @gov: PM domain governor to associate with the domain (may be NULL).
> > > > >   * @is_off: Initial value of the domain's power_is_off field.
> > > > > + * @owner: module owner of this power domain object.
> > > > >   *
> > > > >   * Returns 0 on successful initialization, else a negative error code.
> > > > >   */
> > > > > -int pm_genpd_init(struct generic_pm_domain *genpd,
> > > > > -                 struct dev_power_governor *gov, bool is_off)
> > > > > +int __pm_genpd_init(struct generic_pm_domain *genpd,
> > > > > +                 struct dev_power_governor *gov, bool is_off,
> > > > > +                 struct module *owner)
> > > >
> > > > Please drop this new interface altogether. Instead we can just let the
> > > > caller of pm_genpd_init() to assign genpd->owner, rather than passing
> > > > it as a parameter.
> > >
> > > No, I asked for this type of interface because it does not require any
> > > developer to "remember" to set this value or not, and it does not
> > > require you to go and fix the whole kernel.  This is the correct way to
> > > do this, see the many other driver subsystems that do this today for
> > > that reason.
> >
> > Well, in many cases I would agree with you, but not for genpd.
> >
> > We have and are still, continuously finding new configurations that
> > are needed for a genpd. And we don't want a new in-parameter to be
> > added each time that happens.
>
> THIS_MODULE is "special", other config options you need should go in a
> structure as you say.  This way we get the compiler to fix the option to
> always be the correct one, no need for a developer to remember it,
> totally different from all other driver/subsystem options.

Right. I didn't consider that THIS_MODULE is special, that's a good point.

In that case, I am okay with it (moving it to an static inline
function is also preferred by me, as pointed out by Rafael)

Kind regards
Uffe