Re: [PATCH V3 6/6] PM / Domains: Propagate performance state updates

[Viresh Kumar]

On 13-12-18, 16:53, Ulf Hansson wrote:
> On Wed, 12 Dec 2018 at 11:58, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> >  update_state:
> > -       return _genpd_set_performance_state(genpd, state);
> > +       return _genpd_set_performance_state(genpd, state, depth);
> 
> Instead of calling _genpd_set_performance_state() from here, I suggest
> to let the caller do it. Simply return the aggregated new state, if it
> needs to be updated - and zero if no update is needed.
> 
> Why? I think it may clarify and simplify the code, in regards to the
> actual set/propagation of state changes. Another side-effect, is that
> you should be able to avoid the forward declaration of
> _genpd_reeval_performance_state(), which I think is nice as well.

_genpd_reeval_performance_state() is currently called from 3 different
places and with the suggested change those sites will have this diff.

-               ret = _genpd_reeval_performance_state(master, master_state,
-                                                     depth + 1);
+               master_state = _genpd_reeval_performance_state(master,
+                                               master_state);
+               ret = _genpd_set_performance_state(genpd, master_state, depth);

To be honest, I don't like it. Probably because I don't find the extra
declaration of _genpd_reeval_performance_state() that bad. If two
routines are always going to get called together it is worth calling
the second one from the first one for me.

But anyway, I am fine with it if you are. Please let me know.

> >  }
> >
> >  /**
> > @@ -332,7 +407,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> >         prev = gpd_data->performance_state;
> >         gpd_data->performance_state = state;
> >
> > -       ret = _genpd_reeval_performance_state(genpd, state);
> > +       ret = _genpd_reeval_performance_state(genpd, state, 0);
> >         if (ret)
> >                 gpd_data->performance_state = prev;
> >
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 9ad101362aef..dd364abb649a 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -136,6 +136,10 @@ struct gpd_link {
> >         struct list_head master_node;
> >         struct generic_pm_domain *slave;
> >         struct list_head slave_node;
> > +
> > +       /* Sub-domain's per-master domain performance state */
> > +       unsigned int performance_state;
> > +       unsigned int prev_performance_state;
> 
> Probably a leftover from the earlier versions, please remove.

No, these are still getting used.

-- 
viresh