| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170728110030.GG352@vireshk-i7>
Date: Fri, 28 Jul 2017 16:30:30 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Rafael Wysocki <rjw@...ysocki.net>,
Kevin Hilman <khilman@...nel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Nishanth Menon <nm@...com>, Rob Herring <robh+dt@...nel.org>,
Lina Iyer <lina.iyer@...aro.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
Sudeep Holla <sudeep.holla@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Andy Gross <andy.gross@...aro.org>,
David Brown <david.brown@...aro.org>
Subject: Re: [PATCH V8 1/6] PM / Domains: Add support to select
performance-state of domains
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
Powered by blists - more mailing lists