From: Viresh Kumar <viresh.kumar@linaro.org> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>, linux-pm@vger.kernel.org, Daniel Lezcano <daniel.lezcano@linaro.org>, "Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>, Stephen Boyd <sboyd@kernel.org>, Tony Lindgren <tony@atomide.com>, Kevin Hilman <khilman@kernel.org>, Lina Iyer <ilina@codeaurora.org>, Rajendra Nayak <rnayak@codeaurora.org>, Niklas Cassel <niklas.cassel@linaro.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock Date: Thu, 25 Apr 2019 15:15:02 +0530 [thread overview] Message-ID: <20190425094502.6wprvxdz44e5g63q@vireshk-i7> (raw) In-Reply-To: <20190425090413.10700-4-ulf.hansson@linaro.org> On 25-04-19, 11:04, Ulf Hansson wrote: > While attaching/detaching a device to a PM domain (genpd) that has the > GENPD_FLAG_CPU_DOMAIN set, genpd iterates the cpu_possible_mask to check > whether the device corresponds to a CPU. This iteration is done while > holding the genpd's lock, which is unnecessary. Let's avoid the locking, > by restructuring the corresponding code a bit. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 52 +++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 25 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 93298b7db408..da1c99178943 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1450,8 +1450,8 @@ static void genpd_free_dev_data(struct device *dev, > dev_pm_put_subsys_data(dev); > } > > -static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > - int cpu, bool set, unsigned int depth) > +static void genpd_update_cpumask(struct generic_pm_domain *genpd, > + int cpu, bool set, unsigned int depth) > { > struct gpd_link *link; > > @@ -1462,7 +1462,7 @@ static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > struct generic_pm_domain *master = link->master; > > genpd_lock_nested(master, depth + 1); > - __genpd_update_cpumask(master, cpu, set, depth + 1); > + genpd_update_cpumask(master, cpu, set, depth + 1); > genpd_unlock(master); > } > > @@ -1472,38 +1472,37 @@ static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > cpumask_clear_cpu(cpu, genpd->cpus); > } > > -static void genpd_update_cpumask(struct generic_pm_domain *genpd, > - struct device *dev, bool set) > +static void genpd_set_cpumask(struct generic_pm_domain *genpd, int cpu) > +{ > + if (cpu >= 0) > + genpd_update_cpumask(genpd, cpu, true, 0); > +} > + > +static void genpd_clear_cpumask(struct generic_pm_domain *genpd, int cpu) > +{ > + if (cpu >= 0) > + genpd_update_cpumask(genpd, cpu, false, 0); > +} > + > +static int genpd_get_cpu(struct generic_pm_domain *genpd, struct device *dev) > { > int cpu; > > if (!genpd_is_cpu_domain(genpd)) > - return; > + return -1; > > for_each_possible_cpu(cpu) { > - if (get_cpu_device(cpu) == dev) { > - __genpd_update_cpumask(genpd, cpu, set, 0); > - return; > - } > + if (get_cpu_device(cpu) == dev) > + return cpu; > } > -} > > -static void genpd_set_cpumask(struct generic_pm_domain *genpd, > - struct device *dev) > -{ > - genpd_update_cpumask(genpd, dev, true); > -} > - > -static void genpd_clear_cpumask(struct generic_pm_domain *genpd, > - struct device *dev) > -{ > - genpd_update_cpumask(genpd, dev, false); > + return -1; > } > > static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev) > { > struct generic_pm_domain_data *gpd_data; > - int ret; > + int ret, cpu; > > dev_dbg(dev, "%s()\n", __func__); > > @@ -1514,13 +1513,15 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev) > if (IS_ERR(gpd_data)) > return PTR_ERR(gpd_data); > > + cpu = genpd_get_cpu(genpd, dev); > + > ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; > if (ret) > goto out; > > genpd_lock(genpd); > > - genpd_set_cpumask(genpd, dev); > + genpd_set_cpumask(genpd, cpu); Should we check if "cpu" is valid here ? As that was done earlier. > dev_pm_domain_set(dev, &genpd->domain); > > genpd->device_count++; > @@ -1560,13 +1561,14 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > { > struct generic_pm_domain_data *gpd_data; > struct pm_domain_data *pdd; > - int ret = 0; > + int cpu, ret = 0; > > dev_dbg(dev, "%s()\n", __func__); > > pdd = dev->power.subsys_data->domain_data; > gpd_data = to_gpd_data(pdd); > dev_pm_qos_remove_notifier(dev, &gpd_data->nb); > + cpu = genpd_get_cpu(genpd, dev); > > genpd_lock(genpd); > > @@ -1578,7 +1580,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > genpd->device_count--; > genpd->max_off_time_changed = true; > > - genpd_clear_cpumask(genpd, dev); > + genpd_clear_cpumask(genpd, cpu); Same here. > dev_pm_domain_set(dev, NULL); > > list_del_init(&pdd->list_node); > -- > 2.17.1 -- viresh
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Rajendra Nayak <rnayak@codeaurora.org>, linux-pm@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, "Rafael J . Wysocki" <rjw@rjwysocki.net>, Kevin Hilman <khilman@kernel.org>, Lina Iyer <ilina@codeaurora.org>, linux-kernel@vger.kernel.org, Tony Lindgren <tony@atomide.com>, Niklas Cassel <niklas.cassel@linaro.org>, "Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock Date: Thu, 25 Apr 2019 15:15:02 +0530 [thread overview] Message-ID: <20190425094502.6wprvxdz44e5g63q@vireshk-i7> (raw) In-Reply-To: <20190425090413.10700-4-ulf.hansson@linaro.org> On 25-04-19, 11:04, Ulf Hansson wrote: > While attaching/detaching a device to a PM domain (genpd) that has the > GENPD_FLAG_CPU_DOMAIN set, genpd iterates the cpu_possible_mask to check > whether the device corresponds to a CPU. This iteration is done while > holding the genpd's lock, which is unnecessary. Let's avoid the locking, > by restructuring the corresponding code a bit. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 52 +++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 25 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 93298b7db408..da1c99178943 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1450,8 +1450,8 @@ static void genpd_free_dev_data(struct device *dev, > dev_pm_put_subsys_data(dev); > } > > -static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > - int cpu, bool set, unsigned int depth) > +static void genpd_update_cpumask(struct generic_pm_domain *genpd, > + int cpu, bool set, unsigned int depth) > { > struct gpd_link *link; > > @@ -1462,7 +1462,7 @@ static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > struct generic_pm_domain *master = link->master; > > genpd_lock_nested(master, depth + 1); > - __genpd_update_cpumask(master, cpu, set, depth + 1); > + genpd_update_cpumask(master, cpu, set, depth + 1); > genpd_unlock(master); > } > > @@ -1472,38 +1472,37 @@ static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > cpumask_clear_cpu(cpu, genpd->cpus); > } > > -static void genpd_update_cpumask(struct generic_pm_domain *genpd, > - struct device *dev, bool set) > +static void genpd_set_cpumask(struct generic_pm_domain *genpd, int cpu) > +{ > + if (cpu >= 0) > + genpd_update_cpumask(genpd, cpu, true, 0); > +} > + > +static void genpd_clear_cpumask(struct generic_pm_domain *genpd, int cpu) > +{ > + if (cpu >= 0) > + genpd_update_cpumask(genpd, cpu, false, 0); > +} > + > +static int genpd_get_cpu(struct generic_pm_domain *genpd, struct device *dev) > { > int cpu; > > if (!genpd_is_cpu_domain(genpd)) > - return; > + return -1; > > for_each_possible_cpu(cpu) { > - if (get_cpu_device(cpu) == dev) { > - __genpd_update_cpumask(genpd, cpu, set, 0); > - return; > - } > + if (get_cpu_device(cpu) == dev) > + return cpu; > } > -} > > -static void genpd_set_cpumask(struct generic_pm_domain *genpd, > - struct device *dev) > -{ > - genpd_update_cpumask(genpd, dev, true); > -} > - > -static void genpd_clear_cpumask(struct generic_pm_domain *genpd, > - struct device *dev) > -{ > - genpd_update_cpumask(genpd, dev, false); > + return -1; > } > > static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev) > { > struct generic_pm_domain_data *gpd_data; > - int ret; > + int ret, cpu; > > dev_dbg(dev, "%s()\n", __func__); > > @@ -1514,13 +1513,15 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev) > if (IS_ERR(gpd_data)) > return PTR_ERR(gpd_data); > > + cpu = genpd_get_cpu(genpd, dev); > + > ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0; > if (ret) > goto out; > > genpd_lock(genpd); > > - genpd_set_cpumask(genpd, dev); > + genpd_set_cpumask(genpd, cpu); Should we check if "cpu" is valid here ? As that was done earlier. > dev_pm_domain_set(dev, &genpd->domain); > > genpd->device_count++; > @@ -1560,13 +1561,14 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > { > struct generic_pm_domain_data *gpd_data; > struct pm_domain_data *pdd; > - int ret = 0; > + int cpu, ret = 0; > > dev_dbg(dev, "%s()\n", __func__); > > pdd = dev->power.subsys_data->domain_data; > gpd_data = to_gpd_data(pdd); > dev_pm_qos_remove_notifier(dev, &gpd_data->nb); > + cpu = genpd_get_cpu(genpd, dev); > > genpd_lock(genpd); > > @@ -1578,7 +1580,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > genpd->device_count--; > genpd->max_off_time_changed = true; > > - genpd_clear_cpumask(genpd, dev); > + genpd_clear_cpumask(genpd, cpu); Same here. > dev_pm_domain_set(dev, NULL); > > list_del_init(&pdd->list_node); > -- > 2.17.1 -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-04-25 9:45 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-25 9:04 [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Ulf Hansson 2019-04-25 9:04 ` Ulf Hansson 2019-04-25 9:04 ` [PATCH 1/4] PM / Domains: Use the base device for driver_deferred_probe_check_state() Ulf Hansson 2019-04-25 9:04 ` Ulf Hansson 2019-04-25 9:39 ` Viresh Kumar 2019-04-25 9:39 ` Viresh Kumar 2019-04-25 9:04 ` [PATCH 2/4] PM / Domains: Drop unused in-parameter to some genpd functions Ulf Hansson 2019-04-25 9:04 ` Ulf Hansson 2019-04-25 9:40 ` Viresh Kumar 2019-04-25 9:40 ` Viresh Kumar 2019-04-25 9:04 ` [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock Ulf Hansson 2019-04-25 9:04 ` Ulf Hansson 2019-04-25 9:45 ` Viresh Kumar [this message] 2019-04-25 9:45 ` Viresh Kumar 2019-04-25 10:14 ` Ulf Hansson 2019-04-25 10:14 ` Ulf Hansson 2019-04-25 10:17 ` Viresh Kumar 2019-04-25 10:17 ` Viresh Kumar 2019-04-25 9:04 ` [PATCH 4/4] PM / Domains: Allow to attach a CPU via genpd_dev_pm_attach_by_id|name() Ulf Hansson 2019-04-25 9:04 ` Ulf Hansson 2019-04-25 9:47 ` Viresh Kumar 2019-04-25 9:47 ` Viresh Kumar 2019-04-25 9:56 ` [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Rafael J. Wysocki 2019-04-25 9:56 ` Rafael J. Wysocki 2019-04-25 10:11 ` Ulf Hansson 2019-04-25 10:11 ` Ulf Hansson 2019-04-25 10:11 ` Ulf Hansson 2019-04-25 10:14 ` Rafael J. Wysocki 2019-04-25 10:14 ` Rafael J. Wysocki 2019-04-25 10:17 ` Ulf Hansson 2019-04-25 10:17 ` Ulf Hansson 2019-04-25 10:32 ` Rafael J. Wysocki 2019-04-25 10:32 ` Rafael J. Wysocki 2019-05-01 10:42 ` Rafael J. Wysocki 2019-05-01 10:42 ` Rafael J. Wysocki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190425094502.6wprvxdz44e5g63q@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=daniel.lezcano@linaro.org \ --cc=ilina@codeaurora.org \ --cc=khilman@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=niklas.cassel@linaro.org \ --cc=rjw@rjwysocki.net \ --cc=rnayak@codeaurora.org \ --cc=rplsssn@codeaurora.org \ --cc=sboyd@kernel.org \ --cc=tony@atomide.com \ --cc=ulf.hansson@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.