On 28 July 2017 at 13:00, Viresh Kumar [off-list ref] wrote:

On 21-07-17, 10:35, Ulf Hansson wrote:

quoted

quoted

quoted

quoted

+/*
+ * 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. :(

quoted

quoted

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 :(

Yeah, you are right!

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.

Let's invent a new genpd flag, GENPD_FLAG_PERF_STATE!

The creator of the genpd then needs to set this before calling
pm_genpd_init(). Similar as we are dealing with GENPD_FLAG_PM_CLK.

The requirement for GENPD_FLAG_PERF_STATES, is to have the
->get_performance_state() assigned. This shall be verified during
pm_genpd_init().

The pm_genpd_has_performance_state() then only need to return true, in
cases the device's genpd has GENPD_FLAG_PERF_STATE set - else false.

Regarding ->set_performance_state(), let's just make it optional - and
when trying to set a new performance state, just walk the genpd
hierarchy, from bottom to up, then invoke the callback when it's
assigned.

Kind regards
Uffe