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

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

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