Hi Rafael,
V1 only contained a single patch, i.e. 2/4 from this series. And then there were few suggestions to incorporate.
All are marked for stable 4.3, as this got changed recently.
I have tested it on exynos (Yeah, I was able to reproduce the lockdep with few config changes).
Viresh Kumar (4): PM / OPP: Propagate error properly from dev_pm_opp_set_sharing_cpus() PM / OPP: Protect updates to list_dev with mutex PM / OPP: Hold dev_opp_list_lock for writers PM / OPP: Add opp_rcu_lockdep_assert() to _find_device_opp()
drivers/base/power/opp/core.c | 21 +++++++++++++++++---- drivers/base/power/opp/cpu.c | 10 +++++----- drivers/base/power/opp/opp.h | 3 +++ 3 files changed, 25 insertions(+), 9 deletions(-)
We are returning 0 even in case of errors, fix it.
Cc: 4.3 stable@vger.kernel.org # 4.3 Fixes: 8d4d4e98acd6 ("PM / OPP: Add helpers for initializing CPU OPPs") Reported-by: Dan Carpenter dan.carpenter@oracle.com Reviewed-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index c27a1cdffec9..2139c9d4c447 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -156,7 +156,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) out_rcu_read_unlock: rcu_read_unlock();
- return 0; + return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
dev_opp_list_lock is used everywhere to protect device and OPP lists, but dev_pm_opp_set_sharing_cpus() is missed somehow. And instead we used rcu-lock, which wouldn't help here as we are adding a new list_dev.
This also fixes a problem where we have called kzalloc(..., GFP_KERNEL) from within rcu-lock, which isn't allowed as kzalloc can sleep when called with GFP_KERNEL.
With CONFIG_DEBUG_ATOMIC_SLEEP set, we get following lockdep-splat:
include/linux/rcupdate.h:578 Illegal context switch in RCU read-side critical section!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0 5 locks held by swapper/0/1: #0: (&dev->mutex){......}, at: [<c02f68f4>] __driver_attach+0x48/0x98 #1: (&dev->mutex){......}, at: [<c02f6904>] __driver_attach+0x58/0x98 #2: (cpu_hotplug.lock){++++++}, at: [<c00249d0>] get_online_cpus+0x40/0xb0 #3: (subsys mutex#5){+.+.+.}, at: [<c02f4f8c>] subsys_interface_register+0x44/0xdc #4: (rcu_read_lock){......}, at: [<c0305c80>] dev_pm_opp_set_sharing_cpus+0x0/0x1e4
stack backtrace: CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W 4.3.0-rc7-00047-g81f5932958a8 #59 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c0016874>] (unwind_backtrace) from [<c001355c>] (show_stack+0x10/0x14) [<c001355c>] (show_stack) from [<c022553c>] (dump_stack+0x94/0xbc) [<c022553c>] (dump_stack) from [<c004904c>] (___might_sleep+0x24c/0x298) [<c004904c>] (___might_sleep) from [<c00f07e4>] (kmem_cache_alloc+0xe8/0x164) [<c00f07e4>] (kmem_cache_alloc) from [<c0305354>] (_add_list_dev+0x30/0x58) [<c0305354>] (_add_list_dev) from [<c0305d50>] (dev_pm_opp_set_sharing_cpus+0xd0/0x1e4) [<c0305d50>] (dev_pm_opp_set_sharing_cpus) from [<c040eda4>] (cpufreq_init+0x4cc/0x62c) [<c040eda4>] (cpufreq_init) from [<c040a964>] (cpufreq_online+0xbc/0x73c) [<c040a964>] (cpufreq_online) from [<c02f4fe0>] (subsys_interface_register+0x98/0xdc) [<c02f4fe0>] (subsys_interface_register) from [<c040a640>] (cpufreq_register_driver+0x110/0x17c) [<c040a640>] (cpufreq_register_driver) from [<c040ef64>] (dt_cpufreq_probe+0x60/0x8c) [<c040ef64>] (dt_cpufreq_probe) from [<c02f8084>] (platform_drv_probe+0x44/0xa4) [<c02f8084>] (platform_drv_probe) from [<c02f67c0>] (driver_probe_device+0x208/0x2f4) [<c02f67c0>] (driver_probe_device) from [<c02f6940>] (__driver_attach+0x94/0x98) [<c02f6940>] (__driver_attach) from [<c02f4c1c>] (bus_for_each_dev+0x68/0x9c)
Cc: 4.3 stable@vger.kernel.org # 4.3 Reported-by: Michael Turquette mturquette@baylibre.com Reviewed-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/core.c | 2 +- drivers/base/power/opp/cpu.c | 8 ++++---- drivers/base/power/opp/opp.h | 3 +++ 3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 8fa522f41226..a93a433c9ac5 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -29,7 +29,7 @@ */ static LIST_HEAD(dev_opp_list); /* Lock to allow exclusive modification to the device and opp lists */ -static DEFINE_MUTEX(dev_opp_list_lock); +DEFINE_MUTEX(dev_opp_list_lock);
#define opp_rcu_lockdep_assert() \ do { \ diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c index 2139c9d4c447..7b445e88a0d5 100644 --- a/drivers/base/power/opp/cpu.c +++ b/drivers/base/power/opp/cpu.c @@ -127,12 +127,12 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) struct device *dev; int cpu, ret = 0;
- rcu_read_lock(); + mutex_lock(&dev_opp_list_lock);
dev_opp = _find_device_opp(cpu_dev); if (IS_ERR(dev_opp)) { ret = -EINVAL; - goto out_rcu_read_unlock; + goto unlock; }
for_each_cpu(cpu, cpumask) { @@ -153,8 +153,8 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask) continue; } } -out_rcu_read_unlock: - rcu_read_unlock(); +unlock: + mutex_unlock(&dev_opp_list_lock);
return ret; } diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index dcb38f78dae4..7366b2aa8997 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -21,6 +21,9 @@ #include <linux/rculist.h> #include <linux/rcupdate.h>
+/* Lock to allow exclusive modification to the device and opp lists */ +extern struct mutex dev_opp_list_lock; + /* * Internal data structure organization with the OPP layer library is as * follows:
Writers need to update OPP device and their list with dev_opp_list_lock mutex held, which was missed at few places. Fix it.
Cc: 4.3 stable@vger.kernel.org # 4.3 Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/core.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index a93a433c9ac5..7a395f5ba5f9 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1175,13 +1175,17 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) struct device_opp *dev_opp; int ret = 0, count = 0;
+ mutex_lock(&dev_opp_list_lock); + dev_opp = _managed_opp(opp_np); if (dev_opp) { /* OPPs are already managed */ if (!_add_list_dev(dev, dev_opp)) ret = -ENOMEM; + mutex_unlock(&dev_opp_list_lock); return ret; } + mutex_unlock(&dev_opp_list_lock);
/* We have opp-list node now, iterate over it and add OPPs */ for_each_available_child_of_node(opp_np, np) { @@ -1199,15 +1203,20 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) if (WARN_ON(!count)) return -ENOENT;
+ mutex_lock(&dev_opp_list_lock); + dev_opp = _find_device_opp(dev); if (WARN_ON(IS_ERR(dev_opp))) { ret = PTR_ERR(dev_opp); + mutex_unlock(&dev_opp_list_lock); goto free_table; }
dev_opp->np = opp_np; dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared");
+ mutex_unlock(&dev_opp_list_lock); + return 0;
free_table:
_find_device_opp() should be called with rcu-read lock or dev_opp_list_lock held. Add the opp_rcu_lockdep_assert() check to make sure caller have taken appropriate locks.
Fix comment over the routine as well.
Cc: 4.3 stable@vger.kernel.org # 4.3 Suggested-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 7a395f5ba5f9..5e4b1d82b2f8 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -81,14 +81,18 @@ static struct device_opp *_managed_opp(const struct device_node *np) * Return: pointer to 'struct device_opp' if found, otherwise -ENODEV or * -EINVAL based on type of error. * - * Locking: This function must be called under rcu_read_lock(). device_opp - * is a RCU protected pointer. This means that device_opp is valid as long - * as we are under RCU lock. + * Locking: For readers, this function must be called under rcu_read_lock(). + * device_opp is a RCU protected pointer, which means that device_opp is valid + * as long as we are under RCU lock. + * + * For Writers, this function must be called with dev_opp_list_lock held. */ struct device_opp *_find_device_opp(struct device *dev) { struct device_opp *dev_opp;
+ opp_rcu_lockdep_assert(); + if (IS_ERR_OR_NULL(dev)) { pr_err("%s: Invalid parameters\n", __func__); return ERR_PTR(-EINVAL);
On Thursday, November 05, 2015 02:21:17 PM Viresh Kumar wrote:
Hi Rafael,
V1 only contained a single patch, i.e. 2/4 from this series. And then there were few suggestions to incorporate.
All are marked for stable 4.3, as this got changed recently.
I have tested it on exynos (Yeah, I was able to reproduce the lockdep with few config changes).
Viresh Kumar (4): PM / OPP: Propagate error properly from dev_pm_opp_set_sharing_cpus() PM / OPP: Protect updates to list_dev with mutex PM / OPP: Hold dev_opp_list_lock for writers PM / OPP: Add opp_rcu_lockdep_assert() to _find_device_opp()
drivers/base/power/opp/core.c | 21 +++++++++++++++++---- drivers/base/power/opp/cpu.c | 10 +++++----- drivers/base/power/opp/opp.h | 3 +++ 3 files changed, 25 insertions(+), 9 deletions(-)
Applied, but I removed the stable tag from the last patch as it really is not a bug fix.
Thanks, Rafael
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Viresh Kumar