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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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