* [PATCH 1/4] PM / Domains: Use the base device for driver_deferred_probe_check_state()
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:39 ` Viresh Kumar
2019-04-25 9:04 ` [PATCH 2/4] PM / Domains: Drop unused in-parameter to some genpd functions Ulf Hansson
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 9:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Rob Herring, Ulf Hansson, Rajendra Nayak, Kevin Hilman,
Stephen Boyd, Viresh Kumar, Daniel Lezcano, linux-kernel,
Lina Iyer, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, linux-arm-kernel
When genpd fails to attach a device to one of its multiple PM domains, we
end up calling driver_deferred_probe_check_state() for the recently
allocated virtual device. This is wrong, as it's the base device that is
being probed. Fix this, by passing along the base device to
__genpd_dev_pm_attach() and use that instead.
Fixes: e01afc325025 ("PM / Domains: Stop deferring probe at the end of initcall")
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 8362dfe187f5..8aca1c9b4406 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2405,8 +2405,8 @@ static void genpd_dev_pm_sync(struct device *dev)
genpd_queue_power_off_work(pd);
}
-static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
- bool power_on)
+static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
+ unsigned int index, bool power_on)
{
struct of_phandle_args pd_args;
struct generic_pm_domain *pd;
@@ -2424,7 +2424,7 @@ static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
mutex_unlock(&gpd_list_lock);
dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
__func__, PTR_ERR(pd));
- return driver_deferred_probe_check_state(dev);
+ return driver_deferred_probe_check_state(base_dev);
}
dev_dbg(dev, "adding to PM domain %s\n", pd->name);
@@ -2480,7 +2480,7 @@ int genpd_dev_pm_attach(struct device *dev)
"#power-domain-cells") != 1)
return 0;
- return __genpd_dev_pm_attach(dev, 0, true);
+ return __genpd_dev_pm_attach(dev, dev, 0, true);
}
EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
@@ -2533,7 +2533,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
}
/* Try to attach the device to the PM domain at the specified index. */
- ret = __genpd_dev_pm_attach(virt_dev, index, false);
+ ret = __genpd_dev_pm_attach(virt_dev, dev, index, false);
if (ret < 1) {
device_unregister(virt_dev);
return ret ? ERR_PTR(ret) : NULL;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/4] PM / Domains: Use the base device for driver_deferred_probe_check_state()
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:39 ` Viresh Kumar
0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2019-04-25 9:39 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rob Herring, Rajendra Nayak, linux-pm, Stephen Boyd,
Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman, Lina Iyer,
linux-kernel, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, linux-arm-kernel
On 25-04-19, 11:04, Ulf Hansson wrote:
> When genpd fails to attach a device to one of its multiple PM domains, we
> end up calling driver_deferred_probe_check_state() for the recently
> allocated virtual device. This is wrong, as it's the base device that is
> being probed. Fix this, by passing along the base device to
> __genpd_dev_pm_attach() and use that instead.
>
> Fixes: e01afc325025 ("PM / Domains: Stop deferring probe at the end of initcall")
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 8362dfe187f5..8aca1c9b4406 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2405,8 +2405,8 @@ static void genpd_dev_pm_sync(struct device *dev)
> genpd_queue_power_off_work(pd);
> }
>
> -static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
> - bool power_on)
> +static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> + unsigned int index, bool power_on)
> {
> struct of_phandle_args pd_args;
> struct generic_pm_domain *pd;
> @@ -2424,7 +2424,7 @@ static int __genpd_dev_pm_attach(struct device *dev, unsigned int index,
> mutex_unlock(&gpd_list_lock);
> dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
> __func__, PTR_ERR(pd));
> - return driver_deferred_probe_check_state(dev);
> + return driver_deferred_probe_check_state(base_dev);
> }
>
> dev_dbg(dev, "adding to PM domain %s\n", pd->name);
> @@ -2480,7 +2480,7 @@ int genpd_dev_pm_attach(struct device *dev)
> "#power-domain-cells") != 1)
> return 0;
>
> - return __genpd_dev_pm_attach(dev, 0, true);
> + return __genpd_dev_pm_attach(dev, dev, 0, true);
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> @@ -2533,7 +2533,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> }
>
> /* Try to attach the device to the PM domain at the specified index. */
> - ret = __genpd_dev_pm_attach(virt_dev, index, false);
> + ret = __genpd_dev_pm_attach(virt_dev, dev, index, false);
> if (ret < 1) {
> device_unregister(virt_dev);
> return ret ? ERR_PTR(ret) : NULL;
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/4] PM / Domains: Drop unused in-parameter to some genpd functions
2019-04-25 9:04 [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd 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:40 ` Viresh Kumar
2019-04-25 9:04 ` [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock Ulf Hansson
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 9:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Rajendra Nayak, Kevin Hilman, Stephen Boyd,
Viresh Kumar, Daniel Lezcano, linux-kernel, Lina Iyer,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
Both genpd_alloc_dev_data() and genpd_add_device(), whom are internal genpd
functions, allows a struct gpd_timing_data *td to be passed as an
in-parameter. However, as NULL is always passed, let's just drop the
in-parameter altogether.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 8aca1c9b4406..93298b7db408 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1396,8 +1396,7 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
#endif /* CONFIG_PM_SLEEP */
-static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
- struct gpd_timing_data *td)
+static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
{
struct generic_pm_domain_data *gpd_data;
int ret;
@@ -1412,9 +1411,6 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
goto err_put;
}
- if (td)
- gpd_data->td = *td;
-
gpd_data->base.dev = dev;
gpd_data->td.constraint_changed = true;
gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS;
@@ -1504,8 +1500,7 @@ static void genpd_clear_cpumask(struct generic_pm_domain *genpd,
genpd_update_cpumask(genpd, dev, false);
}
-static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
- struct gpd_timing_data *td)
+static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
{
struct generic_pm_domain_data *gpd_data;
int ret;
@@ -1515,7 +1510,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
return -EINVAL;
- gpd_data = genpd_alloc_dev_data(dev, td);
+ gpd_data = genpd_alloc_dev_data(dev);
if (IS_ERR(gpd_data))
return PTR_ERR(gpd_data);
@@ -1553,7 +1548,7 @@ int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
int ret;
mutex_lock(&gpd_list_lock);
- ret = genpd_add_device(genpd, dev, NULL);
+ ret = genpd_add_device(genpd, dev);
mutex_unlock(&gpd_list_lock);
return ret;
@@ -2259,7 +2254,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
goto out;
}
- ret = genpd_add_device(genpd, dev, NULL);
+ ret = genpd_add_device(genpd, dev);
out:
mutex_unlock(&gpd_list_lock);
@@ -2429,7 +2424,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
dev_dbg(dev, "adding to PM domain %s\n", pd->name);
- ret = genpd_add_device(pd, dev, NULL);
+ ret = genpd_add_device(pd, dev);
mutex_unlock(&gpd_list_lock);
if (ret < 0) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/4] PM / Domains: Drop unused in-parameter to some genpd functions
2019-04-25 9:04 ` [PATCH 2/4] PM / Domains: Drop unused in-parameter to some genpd functions Ulf Hansson
@ 2019-04-25 9:40 ` Viresh Kumar
0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2019-04-25 9:40 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, linux-pm, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Kevin Hilman, Lina Iyer, linux-kernel,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
On 25-04-19, 11:04, Ulf Hansson wrote:
> Both genpd_alloc_dev_data() and genpd_add_device(), whom are internal genpd
> functions, allows a struct gpd_timing_data *td to be passed as an
> in-parameter. However, as NULL is always passed, let's just drop the
> in-parameter altogether.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 8aca1c9b4406..93298b7db408 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1396,8 +1396,7 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
>
> #endif /* CONFIG_PM_SLEEP */
>
> -static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
> - struct gpd_timing_data *td)
> +static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev)
> {
> struct generic_pm_domain_data *gpd_data;
> int ret;
> @@ -1412,9 +1411,6 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
> goto err_put;
> }
>
> - if (td)
> - gpd_data->td = *td;
> -
> gpd_data->base.dev = dev;
> gpd_data->td.constraint_changed = true;
> gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS;
> @@ -1504,8 +1500,7 @@ static void genpd_clear_cpumask(struct generic_pm_domain *genpd,
> genpd_update_cpumask(genpd, dev, false);
> }
>
> -static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> - struct gpd_timing_data *td)
> +static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
> {
> struct generic_pm_domain_data *gpd_data;
> int ret;
> @@ -1515,7 +1510,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
> return -EINVAL;
>
> - gpd_data = genpd_alloc_dev_data(dev, td);
> + gpd_data = genpd_alloc_dev_data(dev);
> if (IS_ERR(gpd_data))
> return PTR_ERR(gpd_data);
>
> @@ -1553,7 +1548,7 @@ int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
> int ret;
>
> mutex_lock(&gpd_list_lock);
> - ret = genpd_add_device(genpd, dev, NULL);
> + ret = genpd_add_device(genpd, dev);
> mutex_unlock(&gpd_list_lock);
>
> return ret;
> @@ -2259,7 +2254,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
> goto out;
> }
>
> - ret = genpd_add_device(genpd, dev, NULL);
> + ret = genpd_add_device(genpd, dev);
>
> out:
> mutex_unlock(&gpd_list_lock);
> @@ -2429,7 +2424,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>
> dev_dbg(dev, "adding to PM domain %s\n", pd->name);
>
> - ret = genpd_add_device(pd, dev, NULL);
> + ret = genpd_add_device(pd, dev);
> mutex_unlock(&gpd_list_lock);
>
> if (ret < 0) {
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock
2019-04-25 9:04 [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd 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 ` [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:45 ` 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:56 ` [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Rafael J. Wysocki
4 siblings, 2 replies; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 9:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Rajendra Nayak, Kevin Hilman, Stephen Boyd,
Viresh Kumar, Daniel Lezcano, linux-kernel, Lina Iyer,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
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);
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);
dev_pm_domain_set(dev, NULL);
list_del_init(&pdd->list_node);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock
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:45 ` Viresh Kumar
2019-04-25 10:14 ` Ulf Hansson
2019-04-25 10:17 ` Viresh Kumar
1 sibling, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2019-04-25 9:45 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, linux-pm, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Kevin Hilman, Lina Iyer, linux-kernel,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock
2019-04-25 9:45 ` Viresh Kumar
@ 2019-04-25 10:14 ` Ulf Hansson
0 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 10:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rajendra Nayak, Linux PM, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Kevin Hilman, Lina Iyer,
Linux Kernel Mailing List, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, Linux ARM
On Thu, 25 Apr 2019 at 11:45, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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.
This is being done in the new version of genpd_set|clear_cpumask(). Like below.
"if (cpu >= 0)"...
>
> > 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.
See above.
>
> > dev_pm_domain_set(dev, NULL);
> >
> > list_del_init(&pdd->list_node);
> > --
> > 2.17.1
>
> --
> viresh
Thanks for reviewing!
Kind regards
Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] PM / Domains: Search for the CPU device outside the genpd lock
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:45 ` Viresh Kumar
@ 2019-04-25 10:17 ` Viresh Kumar
1 sibling, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2019-04-25 10:17 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, linux-pm, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Kevin Hilman, Lina Iyer, linux-kernel,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
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);
> 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);
> dev_pm_domain_set(dev, NULL);
>
> list_del_init(&pdd->list_node);
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/4] PM / Domains: Allow to attach a CPU via genpd_dev_pm_attach_by_id|name()
2019-04-25 9:04 [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Ulf Hansson
` (2 preceding siblings ...)
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:47 ` Viresh Kumar
2019-04-25 9:56 ` [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Rafael J. Wysocki
4 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 9:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Rajendra Nayak, Kevin Hilman, Stephen Boyd,
Viresh Kumar, Daniel Lezcano, linux-kernel, Lina Iyer,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
Attaching a device via genpd_dev_pm_attach_by_id|name() makes genpd to
allocate a virtual device that it attaches instead. This leads to a problem
in case the base device belongs to a CPU. More precisely, it means
genpd_get_cpu() compares against the virtual device, thus it fails to find
a matching CPU device.
Address this limitation, by passing the base device to genpd_get_cpu()
rather than the virtual device. Moreover, to deal with detach correctly
from genpd_remove_device(), let's store the CPU number in the struct
generic_pm_domain_data, as to be able to clear the corresponding bit in the
cpumask for the genpd.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 20 ++++++++++----------
include/linux/pm_domain.h | 1 +
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index da1c99178943..3d899e8abd58 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1499,10 +1499,11 @@ static int genpd_get_cpu(struct generic_pm_domain *genpd, struct device *dev)
return -1;
}
-static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
+static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+ struct device *base_dev)
{
struct generic_pm_domain_data *gpd_data;
- int ret, cpu;
+ int ret;
dev_dbg(dev, "%s()\n", __func__);
@@ -1513,7 +1514,7 @@ 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);
+ gpd_data->cpu = genpd_get_cpu(genpd, base_dev);
ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0;
if (ret)
@@ -1521,7 +1522,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
genpd_lock(genpd);
- genpd_set_cpumask(genpd, cpu);
+ genpd_set_cpumask(genpd, gpd_data->cpu);
dev_pm_domain_set(dev, &genpd->domain);
genpd->device_count++;
@@ -1549,7 +1550,7 @@ int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
int ret;
mutex_lock(&gpd_list_lock);
- ret = genpd_add_device(genpd, dev);
+ ret = genpd_add_device(genpd, dev, dev);
mutex_unlock(&gpd_list_lock);
return ret;
@@ -1561,14 +1562,13 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
{
struct generic_pm_domain_data *gpd_data;
struct pm_domain_data *pdd;
- int cpu, ret = 0;
+ int 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);
@@ -1580,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, cpu);
+ genpd_clear_cpumask(genpd, gpd_data->cpu);
dev_pm_domain_set(dev, NULL);
list_del_init(&pdd->list_node);
@@ -2256,7 +2256,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
goto out;
}
- ret = genpd_add_device(genpd, dev);
+ ret = genpd_add_device(genpd, dev, dev);
out:
mutex_unlock(&gpd_list_lock);
@@ -2426,7 +2426,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
dev_dbg(dev, "adding to PM domain %s\n", pd->name);
- ret = genpd_add_device(pd, dev);
+ ret = genpd_add_device(pd, dev, base_dev);
mutex_unlock(&gpd_list_lock);
if (ret < 0) {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index bc82e74560ee..0e8e356bed6a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -175,6 +175,7 @@ struct generic_pm_domain_data {
struct pm_domain_data base;
struct gpd_timing_data td;
struct notifier_block nb;
+ int cpu;
unsigned int performance_state;
void *data;
};
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/4] PM / Domains: Allow to attach a CPU via genpd_dev_pm_attach_by_id|name()
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:47 ` Viresh Kumar
0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2019-04-25 9:47 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, linux-pm, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Kevin Hilman, Lina Iyer, linux-kernel,
Tony Lindgren, Niklas Cassel, Raju P . L . S . S . S . N,
linux-arm-kernel
On 25-04-19, 11:04, Ulf Hansson wrote:
> Attaching a device via genpd_dev_pm_attach_by_id|name() makes genpd to
> allocate a virtual device that it attaches instead. This leads to a problem
> in case the base device belongs to a CPU. More precisely, it means
> genpd_get_cpu() compares against the virtual device, thus it fails to find
> a matching CPU device.
>
> Address this limitation, by passing the base device to genpd_get_cpu()
> rather than the virtual device. Moreover, to deal with detach correctly
> from genpd_remove_device(), let's store the CPU number in the struct
> generic_pm_domain_data, as to be able to clear the corresponding bit in the
> cpumask for the genpd.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 20 ++++++++++----------
> include/linux/pm_domain.h | 1 +
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index da1c99178943..3d899e8abd58 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1499,10 +1499,11 @@ static int genpd_get_cpu(struct generic_pm_domain *genpd, struct device *dev)
> return -1;
> }
>
> -static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
> +static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> + struct device *base_dev)
> {
> struct generic_pm_domain_data *gpd_data;
> - int ret, cpu;
> + int ret;
>
> dev_dbg(dev, "%s()\n", __func__);
>
> @@ -1513,7 +1514,7 @@ 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);
> + gpd_data->cpu = genpd_get_cpu(genpd, base_dev);
>
> ret = genpd->attach_dev ? genpd->attach_dev(genpd, dev) : 0;
> if (ret)
> @@ -1521,7 +1522,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
>
> genpd_lock(genpd);
>
> - genpd_set_cpumask(genpd, cpu);
> + genpd_set_cpumask(genpd, gpd_data->cpu);
> dev_pm_domain_set(dev, &genpd->domain);
>
> genpd->device_count++;
> @@ -1549,7 +1550,7 @@ int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
> int ret;
>
> mutex_lock(&gpd_list_lock);
> - ret = genpd_add_device(genpd, dev);
> + ret = genpd_add_device(genpd, dev, dev);
> mutex_unlock(&gpd_list_lock);
>
> return ret;
> @@ -1561,14 +1562,13 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
> {
> struct generic_pm_domain_data *gpd_data;
> struct pm_domain_data *pdd;
> - int cpu, ret = 0;
> + int 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);
>
> @@ -1580,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, cpu);
> + genpd_clear_cpumask(genpd, gpd_data->cpu);
> dev_pm_domain_set(dev, NULL);
>
> list_del_init(&pdd->list_node);
> @@ -2256,7 +2256,7 @@ int of_genpd_add_device(struct of_phandle_args *genpdspec, struct device *dev)
> goto out;
> }
>
> - ret = genpd_add_device(genpd, dev);
> + ret = genpd_add_device(genpd, dev, dev);
>
> out:
> mutex_unlock(&gpd_list_lock);
> @@ -2426,7 +2426,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>
> dev_dbg(dev, "adding to PM domain %s\n", pd->name);
>
> - ret = genpd_add_device(pd, dev);
> + ret = genpd_add_device(pd, dev, base_dev);
> mutex_unlock(&gpd_list_lock);
>
> if (ret < 0) {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index bc82e74560ee..0e8e356bed6a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -175,6 +175,7 @@ struct generic_pm_domain_data {
> struct pm_domain_data base;
> struct gpd_timing_data td;
> struct notifier_block nb;
> + int cpu;
> unsigned int performance_state;
> void *data;
> };
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 9:04 [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Ulf Hansson
` (3 preceding siblings ...)
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:56 ` Rafael J. Wysocki
2019-04-25 10:11 ` Ulf Hansson
4 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-04-25 9:56 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, Linux PM, Stephen Boyd, Viresh Kumar,
Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman, Lina Iyer,
Linux Kernel Mailing List, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, Linux ARM
On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Recently genpd was extended to cope with devices belonging to CPUs. However,
> attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> because of the virtual device that genpd allocates in this path.
>
> In this series, this limitation is addressed, together with a few other related
> fixes/cleanups.
>
> Ulf Hansson (4):
> PM / Domains: Use the base device for
> driver_deferred_probe_check_state()
> PM / Domains: Drop unused in-parameter to some genpd functions
> PM / Domains: Search for the CPU device outside the genpd lock
> PM / Domains: Allow to attach a CPU via
> genpd_dev_pm_attach_by_id|name()
>
> drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> include/linux/pm_domain.h | 1 +
> 2 files changed, 36 insertions(+), 38 deletions(-)
Are there any dependencies between this and the series you've recently posted?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 9:56 ` [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd Rafael J. Wysocki
@ 2019-04-25 10:11 ` Ulf Hansson
2019-04-25 10:14 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 10:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rajendra Nayak, Linux PM, Stephen Boyd, Viresh Kumar,
Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman, Lina Iyer,
Linux Kernel Mailing List, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, Linux ARM
On Thu, 25 Apr 2019 at 11:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Recently genpd was extended to cope with devices belonging to CPUs. However,
> > attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> > because of the virtual device that genpd allocates in this path.
> >
> > In this series, this limitation is addressed, together with a few other related
> > fixes/cleanups.
> >
> > Ulf Hansson (4):
> > PM / Domains: Use the base device for
> > driver_deferred_probe_check_state()
> > PM / Domains: Drop unused in-parameter to some genpd functions
> > PM / Domains: Search for the CPU device outside the genpd lock
> > PM / Domains: Allow to attach a CPU via
> > genpd_dev_pm_attach_by_id|name()
> >
> > drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> > include/linux/pm_domain.h | 1 +
> > 2 files changed, 36 insertions(+), 38 deletions(-)
>
> Are there any dependencies between this and the series you've recently posted?
Yep. I should have stated that, sorry. This should be applied on top.
Kind regards
Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 10:11 ` Ulf Hansson
@ 2019-04-25 10:14 ` Rafael J. Wysocki
2019-04-25 10:17 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-04-25 10:14 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, Niklas Cassel, Linux PM, Stephen Boyd,
Viresh Kumar, Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman,
Lina Iyer, Linux Kernel Mailing List, Tony Lindgren,
Rafael J. Wysocki, Raju P . L . S . S . S . N, Linux ARM
On Thu, Apr 25, 2019 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 25 Apr 2019 at 11:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > Recently genpd was extended to cope with devices belonging to CPUs. However,
> > > attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> > > because of the virtual device that genpd allocates in this path.
> > >
> > > In this series, this limitation is addressed, together with a few other related
> > > fixes/cleanups.
> > >
> > > Ulf Hansson (4):
> > > PM / Domains: Use the base device for
> > > driver_deferred_probe_check_state()
> > > PM / Domains: Drop unused in-parameter to some genpd functions
> > > PM / Domains: Search for the CPU device outside the genpd lock
> > > PM / Domains: Allow to attach a CPU via
> > > genpd_dev_pm_attach_by_id|name()
> > >
> > > drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> > > include/linux/pm_domain.h | 1 +
> > > 2 files changed, 36 insertions(+), 38 deletions(-)
> >
> > Are there any dependencies between this and the series you've recently posted?
>
> Yep. I should have stated that, sorry. This should be applied on top.
On top of which series?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 10:14 ` Rafael J. Wysocki
@ 2019-04-25 10:17 ` Ulf Hansson
2019-04-25 10:32 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2019-04-25 10:17 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rajendra Nayak, Linux PM, Stephen Boyd, Viresh Kumar,
Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman, Lina Iyer,
Linux Kernel Mailing List, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, Linux ARM
On Thu, 25 Apr 2019 at 12:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 25, 2019 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 25 Apr 2019 at 11:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > Recently genpd was extended to cope with devices belonging to CPUs. However,
> > > > attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> > > > because of the virtual device that genpd allocates in this path.
> > > >
> > > > In this series, this limitation is addressed, together with a few other related
> > > > fixes/cleanups.
> > > >
> > > > Ulf Hansson (4):
> > > > PM / Domains: Use the base device for
> > > > driver_deferred_probe_check_state()
> > > > PM / Domains: Drop unused in-parameter to some genpd functions
> > > > PM / Domains: Search for the CPU device outside the genpd lock
> > > > PM / Domains: Allow to attach a CPU via
> > > > genpd_dev_pm_attach_by_id|name()
> > > >
> > > > drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> > > > include/linux/pm_domain.h | 1 +
> > > > 2 files changed, 36 insertions(+), 38 deletions(-)
> > >
> > > Are there any dependencies between this and the series you've recently posted?
> >
> > Yep. I should have stated that, sorry. This should be applied on top.
>
> On top of which series?
[PATCH 0/3] PM / Domains: Improve support for multi PM domains
and
[PATCH v13 0/4] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM)
Kind regards
Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 10:17 ` Ulf Hansson
@ 2019-04-25 10:32 ` Rafael J. Wysocki
2019-05-01 10:42 ` Rafael J. Wysocki
0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-04-25 10:32 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rajendra Nayak, Niklas Cassel, Linux PM, Stephen Boyd,
Viresh Kumar, Daniel Lezcano, Rafael J . Wysocki, Kevin Hilman,
Lina Iyer, Linux Kernel Mailing List, Tony Lindgren,
Rafael J. Wysocki, Raju P . L . S . S . S . N, Linux ARM
On Thu, Apr 25, 2019 at 12:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 25 Apr 2019 at 12:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Apr 25, 2019 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 25 Apr 2019 at 11:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > Recently genpd was extended to cope with devices belonging to CPUs. However,
> > > > > attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> > > > > because of the virtual device that genpd allocates in this path.
> > > > >
> > > > > In this series, this limitation is addressed, together with a few other related
> > > > > fixes/cleanups.
> > > > >
> > > > > Ulf Hansson (4):
> > > > > PM / Domains: Use the base device for
> > > > > driver_deferred_probe_check_state()
> > > > > PM / Domains: Drop unused in-parameter to some genpd functions
> > > > > PM / Domains: Search for the CPU device outside the genpd lock
> > > > > PM / Domains: Allow to attach a CPU via
> > > > > genpd_dev_pm_attach_by_id|name()
> > > > >
> > > > > drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> > > > > include/linux/pm_domain.h | 1 +
> > > > > 2 files changed, 36 insertions(+), 38 deletions(-)
> > > >
> > > > Are there any dependencies between this and the series you've recently posted?
> > >
> > > Yep. I should have stated that, sorry. This should be applied on top.
> >
> > On top of which series?
>
> [PATCH 0/3] PM / Domains: Improve support for multi PM domains
>
> and
>
> [PATCH v13 0/4] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM)
OK, thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] PM / Domains: Improve support for CPUs in genpd
2019-04-25 10:32 ` Rafael J. Wysocki
@ 2019-05-01 10:42 ` Rafael J. Wysocki
0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-05-01 10:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rajendra Nayak, Linux PM, Stephen Boyd, Viresh Kumar,
Daniel Lezcano, Kevin Hilman, Lina Iyer,
Linux Kernel Mailing List, Tony Lindgren, Niklas Cassel,
Raju P . L . S . S . S . N, Linux ARM
On Thursday, April 25, 2019 12:32:24 PM CEST Rafael J. Wysocki wrote:
> On Thu, Apr 25, 2019 at 12:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 25 Apr 2019 at 12:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Apr 25, 2019 at 12:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Thu, 25 Apr 2019 at 11:56, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Thu, Apr 25, 2019 at 11:04 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > Recently genpd was extended to cope with devices belonging to CPUs. However,
> > > > > > attaching CPU devices via genpd_dev_pm_attach_by_id|name() doesn't work,
> > > > > > because of the virtual device that genpd allocates in this path.
> > > > > >
> > > > > > In this series, this limitation is addressed, together with a few other related
> > > > > > fixes/cleanups.
> > > > > >
> > > > > > Ulf Hansson (4):
> > > > > > PM / Domains: Use the base device for
> > > > > > driver_deferred_probe_check_state()
> > > > > > PM / Domains: Drop unused in-parameter to some genpd functions
> > > > > > PM / Domains: Search for the CPU device outside the genpd lock
> > > > > > PM / Domains: Allow to attach a CPU via
> > > > > > genpd_dev_pm_attach_by_id|name()
> > > > > >
> > > > > > drivers/base/power/domain.c | 73 ++++++++++++++++++-------------------
> > > > > > include/linux/pm_domain.h | 1 +
> > > > > > 2 files changed, 36 insertions(+), 38 deletions(-)
> > > > >
> > > > > Are there any dependencies between this and the series you've recently posted?
> > > >
> > > > Yep. I should have stated that, sorry. This should be applied on top.
> > >
> > > On top of which series?
> >
> > [PATCH 0/3] PM / Domains: Improve support for multi PM domains
> >
> > and
> >
> > [PATCH v13 0/4] PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM)
>
> OK, thanks!
>
This series has been applied, thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 17+ messages in thread