From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751837AbdG1LAg (ORCPT ); Fri, 28 Jul 2017 07:00:36 -0400 Received: from mail-pg0-f48.google.com ([74.125.83.48]:33347 "EHLO mail-pg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751768AbdG1LAe (ORCPT ); Fri, 28 Jul 2017 07:00:34 -0400 Date: Fri, 28 Jul 2017 16:30:30 +0530 From: Viresh Kumar To: Ulf Hansson Cc: Rafael Wysocki , Kevin Hilman , "linux-pm@vger.kernel.org" , Vincent Guittot , Stephen Boyd , Nishanth Menon , Rob Herring , Lina Iyer , Rajendra Nayak , Sudeep Holla , "linux-kernel@vger.kernel.org" , Len Brown , Pavel Machek , Andy Gross , David Brown Subject: Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains Message-ID: <20170728110030.GG352@vireshk-i7> References: <52daf22d2c9d92b0e61c8c5c5a88516d7a08a31a.1498026827.git.viresh.kumar@linaro.org> <20170719123751.GF352@vireshk-i7> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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