Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains

[Viresh Kumar]

On 21-07-17, 10:35, Ulf Hansson wrote:
> >> > +/*
> >> > + * Returns true if anyone in genpd's parent hierarchy has
> >> > + * set_performance_state() set.
> >> > + */
> >> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> >> > +{
> >>
> >> So this function will be become in-directly called by generic drivers
> >> that supports DVFS of the genpd for their devices.
> >>
> >> I think the data you validate here would be better to be pre-validated
> >> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
> >> result stored in a variable in the genpd struct. Especially when a
> >> subdomain is added, that is a point when you can verify the
> >> *_performance_state() callbacks, and thus make sure it's a correct
> >> setup from the topology point of view.

Looks like I have to keep this routine as is and your solution may not
work well. :(

> > Something like this ?
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 4a898e095a1d..182c1911ea9c 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> >         return NOTIFY_DONE;
> >  }
> >
> > -/*
> > - * Returns true if anyone in genpd's parent hierarchy has
> > - * set_performance_state() set.
> > - */
> > -static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> > -{
> > -       struct gpd_link *link;
> > -
> > -       if (genpd->set_performance_state)
> > -               return true;
> > -
> > -       list_for_each_entry(link, &genpd->slave_links, slave_node) {
> > -               if (genpd_has_set_performance_state(link->master))
> > -                       return true;
> > -       }
> > -
> > -       return false;
> > -}
> > -
> >  /**
> >   * pm_genpd_has_performance_state - Checks if power domain does performance
> >   * state management.
> > @@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
> >
> >         /* The parent domain must have set get_performance_state() */
> >         if (!IS_ERR(genpd) && genpd->get_performance_state) {
> > -               if (genpd_has_set_performance_state(genpd))
> > +               if (genpd->can_set_performance_state)
> >                         return true;
> >
> >                 /*
> > @@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
> >         if (genpd_status_on(subdomain))
> >                 genpd_sd_counter_inc(genpd);
> >
> > +       subdomain->can_set_performance_state += genpd->can_set_performance_state;
> > +
> >   out:
> >         genpd_unlock(genpd);
> >         genpd_unlock(subdomain);
> > @@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> >                 if (genpd_status_on(subdomain))
> >                         genpd_sd_counter_dec(genpd);
> >
> > +               subdomain->can_set_performance_state -= genpd->can_set_performance_state;
> > +
> >                 ret = 0;
> >                 break;
> >         }
> > @@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> >         genpd->max_off_time_changed = true;
> >         genpd->provider = NULL;
> >         genpd->has_provider = false;
> > +       genpd->can_set_performance_state = !!genpd->set_performance_state;
> >         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
> >         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
> >         genpd->domain.ops.prepare = pm_genpd_prepare;
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index bf90177208a2..995d0cb1bc14 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -64,6 +64,7 @@ struct generic_pm_domain {
> >         unsigned int suspended_count;   /* System suspend device counter */
> >         unsigned int prepared_count;    /* Suspend counter of prepared devices */
> >         unsigned int performance_state; /* Max requested performance state */
> > +       unsigned int can_set_performance_state; /* Number of parent domains supporting set state */
> >         int (*power_off)(struct generic_pm_domain *domain);
> >         int (*power_on)(struct generic_pm_domain *domain);
> >         int (*get_performance_state)(struct device *dev, unsigned long rate);
> >
> 
> Yes!

The above diff will work fine only for the case where the master
domain has all its masters set properly before genpd_add_subdomain()
is called for the subdomain, as the genpd->can_set_performance_state
count wouldn't change after that. But if the masters of the
master are linked to the master after genpd_add_subdomain() is called
for the subdomain, then we wouldn't be update the
subdomain->can_set_performance_state field later.

For example, consider this scenario:

               Domain A (has set_performance_state())

       Domain B                Domain C        (both don't have set_performance_state())

       Domain D                Domain E         (both don't have set_performance_state(), but have get_performance_state())


and here is the call sequence:

genpd_add_subdomain(B, D); can_set_performance_state of B and D = 0;
genpd_add_subdomain(C, E); ... C and E = 0;
genpd_add_subdomain(A, B); ... A = 1, B = 1;
genpd_add_subdomain(A, C); ... A = 1, C = 1;

While the count is set properly for A, B and C, it isn't propagated to
C and E. :(

Though everything would have worked fine if we had this sequence:

genpd_add_subdomain(A, B); ... A = 1, B = 1;
genpd_add_subdomain(A, C); ... A = 1, C = 1;
genpd_add_subdomain(B, D); ... D = 1 ;
genpd_add_subdomain(C, E); ... E = 1;

How to fix it? I tried solving that by propagating the count to all
the subdomains of the subdomain getting added here. But that requires
locking and we can't do that in the reverse direction :(

Anyway, genpd_has_set_performance_state() is supposed to be called
only ONCE by the drivers and so its fine if we have to traverse the
list of subdomains there.

I will keep the original code unless you suggest a good way of getting
around that.

-- 
viresh