Re: [PATCH] PM / OPP: Protect updates to list_dev with mutex

[Michael Turquette <mturquette@baylibre.com>]

Quoting Viresh Kumar (2015-10-30 19:14:09)
> On 30-10-15, 10:06, Stephen Boyd wrote:
> > On 10/30, Viresh Kumar wrote:
> > > 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.
> > 
> > Care to share the splat here?
> 
> I don't know what is wrong (or right) with my exynos 5250 board, but I
> didn't got any splat here even with the right config options (yes I
> should have mentioned that earlier). I have seen this at other times
> as well, while we were running after some cpufreq traces..
> 
> But, the case in hand is pretty straight forward and Mike T. did get a
> splat as that's what he told me. We are calling a sleep-able function
> from rcu_lock and that's obviously wrong.

Splat:

[    2.461883] ===============================
[    2.466278] [ INFO: suspicious RCU usage. ]
[    2.470703] 4.3.0-rc7-00004-g7f16d90-dirty #11 Not tainted
[    2.476501] -------------------------------
[    2.480895] include/linux/rcupdate.h:578 Illegal context switch in RCU read-side critical section!
[    2.490325]
[    2.490325] other info that might help us debug this:
[    2.490325]
[    2.498779]
[    2.498779] rcu_scheduler_active = 1, debug_locks = 1
[    2.505645] 4 locks held by swapper/0/1:
[    2.509796]  #0:  (&dev->mutex){......}, at: [<c0678e1c>] __device_attach+0x20/0x10c
[    2.518066]  #1:  (cpu_hotplug.lock){++++++}, at: [<c024c204>] get_online_cpus+0x40/0xb0
[    2.526672]  #2:  (subsys mutex#5){+.+.+.}, at: [<c0677900>] subsys_interface_register+0x44/0xdc
[    2.535980]  #3:  (rcu_read_lock){......}, at: [<c0687bc8>] set_cpus_sharing_opps+0x0/0x1d4
[    2.544860]
[    2.544860] stack backtrace:
[    2.549499] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.3.0-rc7-00004-g7f16d90-dirty #11
[    2.558013] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    2.564361] [<c0219c34>] (unwind_backtrace) from [<c0215228>] (show_stack+0x10/0x14)
[    2.572570] [<c0215228>] (show_stack) from [<c04c2af0>] (dump_stack+0x98/0xc0)
[    2.580200] [<c04c2af0>] (dump_stack) from [<c0270a84>] (___might_sleep+0x24c/0x298)
[    2.588378] [<c0270a84>] (___might_sleep) from [<c0328ccc>] (kmem_cache_alloc+0xe8/0x164)
[    2.597015] [<c0328ccc>] (kmem_cache_alloc) from [<c068706c>] (_add_list_dev+0x20/0x48)
[    2.605468] [<c068706c>] (_add_list_dev) from [<c0687c98>] (set_cpus_sharing_opps+0xd0/0x1d4)
[    2.614471] [<c0687c98>] (set_cpus_sharing_opps) from [<c086b9b4>] (cpufreq_init+0x4cc/0x62c)
[    2.623474] [<c086b9b4>] (cpufreq_init) from [<c0867780>] (cpufreq_online+0xc8/0x704)
[    2.631713] [<c0867780>] (cpufreq_online) from [<c0677954>] (subsys_interface_register+0x98/0xdc)
[    2.641082] [<c0677954>] (subsys_interface_register) from [<c08682b4>] (cpufreq_register_driver+0x110/0x17c)
[    2.651458] [<c08682b4>] (cpufreq_register_driver) from [<c086bb74>] (dt_cpufreq_probe+0x60/0x8c)
[    2.660827] [<c086bb74>] (dt_cpufreq_probe) from [<c067a9f8>] (platform_drv_probe+0x44/0xa4)
[    2.669708] [<c067a9f8>] (platform_drv_probe) from [<c0679134>] (driver_probe_device+0x208/0x2f4)
[    2.679077] [<c0679134>] (driver_probe_device) from [<c0677630>] (bus_for_each_drv+0x60/0x94)
[    2.688079] [<c0677630>] (bus_for_each_drv) from [<c0678ea4>] (__device_attach+0xa8/0x10c)
[    2.696777] [<c0678ea4>] (__device_attach) from [<c0678584>] (bus_probe_device+0x88/0x90)
[    2.705413] [<c0678584>] (bus_probe_device) from [<c0676934>] (device_add+0x3e8/0x574)
[    2.713775] [<c0676934>] (device_add) from [<c067a70c>] (platform_device_add+0xb4/0x20c)
[    2.722320] [<c067a70c>] (platform_device_add) from [<c067af70>] (platform_device_register_full+0xc4/0xe8)
[    2.732482] [<c067af70>] (platform_device_register_full) from [<c0eaccf0>] (omap2_common_pm_late_init+0x108/0x114)
[    2.743377] [<c0eaccf0>] (omap2_common_pm_late_init) from [<c0ea9f18>] (omap_common_late_init+0xc/0x14)
[    2.753295] [<c0ea9f18>] (omap_common_late_init) from [<c0eaa3d8>] (omap4430_init_late+0x8/0x14)
[    2.762542] [<c0eaa3d8>] (omap4430_init_late) from [<c0e9e8cc>] (init_machine_late+0x20/0x94)
[    2.771545] [<c0e9e8cc>] (init_machine_late) from [<c020ad50>] (do_one_initcall+0x8c/0x1d8)
[    2.780395] [<c020ad50>] (do_one_initcall) from [<c0e9ce08>] (kernel_init_freeable+0x158/0x1f8)
[    2.789581] [<c0e9ce08>] (kernel_init_freeable) from [<c0a4ac04>] (kernel_init+0xc/0xe8)
[    2.798126] [<c0a4ac04>] (kernel_init) from [<c0210dd0>] (ret_from_fork+0x14/0x24)

Regards,
Mike

> 
> > > diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c
> > > index 7654c5606307..91f15b2e25ee 100644
> > > --- a/drivers/base/power/opp/cpu.c
> > > +++ b/drivers/base/power/opp/cpu.c
> > > @@ -124,12 +124,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);
> > 
> > So does _find_device_opp() need to be called with rcu_read_lock()
> > held or not? The comment above the function makes it sound like
> > we need RCU, but we don't do that here anymore.
> 
> That is more for the readers, as this function is going to return a
> pointer to the device OPP, and to make sure it isn't freed behind
> their back, they need to take the RCU lock.
> 
> There are other writer code paths as well, like add-opp, where we just
> take the mutex as there can't be anything stronger than that :)
> 
> -- 
> viresh