| Message ID | 20230526073539.339203-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bf7da55fcdd1478839b21697cf0534be1f149a49 |
| Headers | show |
| Series | notifier: Initialize new struct srcu_usage field | expand |
Il 26/05/23 09:35, Chen-Yu Tsai ha scritto: > In commit 95433f726301 ("srcu: Begin offloading srcu_struct fields to > srcu_update"), a new struct srcu_usage field was added, but was not > properly initialized. This led to a "spinlock bad magic" BUG when > the SRCU notifier was ever used. This was observed in the MediaTek > CCI devfreq driver on next-20230525. Trimmed stack trace as follows: > > BUG: spinlock bad magic on CPU#4, swapper/0/1 > lock: 0xffffff80ff529ac0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0 > Call trace: > spin_bug+0xa4/0xe8 > do_raw_spin_lock+0xec/0x120 > _raw_spin_lock_irqsave+0x78/0xb8 > synchronize_srcu+0x3c/0x168 > srcu_notifier_chain_unregister+0x5c/0xa0 > cpufreq_unregister_notifier+0x94/0xe0 > devfreq_passive_event_handler+0x7c/0x3e0 > devfreq_remove_device+0x48/0xe8 > > Add __SRCU_USAGE_INIT() to SRCU_NOTIFIER_INIT() so that srcu_usage gets > initialized properly. > > Fixes: 95433f726301 ("srcu: Begin offloading srcu_struct fields to srcu_update") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Fri, May 26, 2023 at 03:35:37PM +0800, Chen-Yu Tsai wrote: > In commit 95433f726301 ("srcu: Begin offloading srcu_struct fields to > srcu_update"), a new struct srcu_usage field was added, but was not > properly initialized. This led to a "spinlock bad magic" BUG when > the SRCU notifier was ever used. This was observed in the MediaTek > CCI devfreq driver on next-20230525. Trimmed stack trace as follows: > > BUG: spinlock bad magic on CPU#4, swapper/0/1 > lock: 0xffffff80ff529ac0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0 > Call trace: > spin_bug+0xa4/0xe8 > do_raw_spin_lock+0xec/0x120 > _raw_spin_lock_irqsave+0x78/0xb8 > synchronize_srcu+0x3c/0x168 > srcu_notifier_chain_unregister+0x5c/0xa0 > cpufreq_unregister_notifier+0x94/0xe0 > devfreq_passive_event_handler+0x7c/0x3e0 > devfreq_remove_device+0x48/0xe8 > > Add __SRCU_USAGE_INIT() to SRCU_NOTIFIER_INIT() so that srcu_usage gets > initialized properly. > > Fixes: 95433f726301 ("srcu: Begin offloading srcu_struct fields to srcu_update") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Good catch, thank you! Reviewed-by: Paul E. McKenney <paulmck@kernel.org> > --- > > Also, the original patch subject said "srcu_update", however the data > structure ended up as "srcu_usage". Maybe that could be updated? > > include/linux/notifier.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/notifier.h b/include/linux/notifier.h > index 2aba75145144..86544707236a 100644 > --- a/include/linux/notifier.h > +++ b/include/linux/notifier.h > @@ -106,12 +106,22 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh); > #define RAW_NOTIFIER_INIT(name) { \ > .head = NULL } > > +#ifdef CONFIG_TREE_SRCU > #define SRCU_NOTIFIER_INIT(name, pcpu) \ > { \ > .mutex = __MUTEX_INITIALIZER(name.mutex), \ > .head = NULL, \ > + .srcuu = __SRCU_USAGE_INIT(name.srcuu), \ > .srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \ > } > +#else > +#define SRCU_NOTIFIER_INIT(name, pcpu) \ > + { \ > + .mutex = __MUTEX_INITIALIZER(name.mutex), \ > + .head = NULL, \ > + .srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \ > + } > +#endif > > #define ATOMIC_NOTIFIER_HEAD(name) \ > struct atomic_notifier_head name = \ > -- > 2.41.0.rc0.172.g3f132b7071-goog >
diff --git a/include/linux/notifier.h b/include/linux/notifier.h index 2aba75145144..86544707236a 100644 --- a/include/linux/notifier.h +++ b/include/linux/notifier.h @@ -106,12 +106,22 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh); #define RAW_NOTIFIER_INIT(name) { \ .head = NULL } +#ifdef CONFIG_TREE_SRCU #define SRCU_NOTIFIER_INIT(name, pcpu) \ { \ .mutex = __MUTEX_INITIALIZER(name.mutex), \ .head = NULL, \ + .srcuu = __SRCU_USAGE_INIT(name.srcuu), \ .srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \ } +#else +#define SRCU_NOTIFIER_INIT(name, pcpu) \ + { \ + .mutex = __MUTEX_INITIALIZER(name.mutex), \ + .head = NULL, \ + .srcu = __SRCU_STRUCT_INIT(name.srcu, name.srcuu, pcpu), \ + } +#endif #define ATOMIC_NOTIFIER_HEAD(name) \ struct atomic_notifier_head name = \
In commit 95433f726301 ("srcu: Begin offloading srcu_struct fields to srcu_update"), a new struct srcu_usage field was added, but was not properly initialized. This led to a "spinlock bad magic" BUG when the SRCU notifier was ever used. This was observed in the MediaTek CCI devfreq driver on next-20230525. Trimmed stack trace as follows: BUG: spinlock bad magic on CPU#4, swapper/0/1 lock: 0xffffff80ff529ac0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0 Call trace: spin_bug+0xa4/0xe8 do_raw_spin_lock+0xec/0x120 _raw_spin_lock_irqsave+0x78/0xb8 synchronize_srcu+0x3c/0x168 srcu_notifier_chain_unregister+0x5c/0xa0 cpufreq_unregister_notifier+0x94/0xe0 devfreq_passive_event_handler+0x7c/0x3e0 devfreq_remove_device+0x48/0xe8 Add __SRCU_USAGE_INIT() to SRCU_NOTIFIER_INIT() so that srcu_usage gets initialized properly. Fixes: 95433f726301 ("srcu: Begin offloading srcu_struct fields to srcu_update") Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Also, the original patch subject said "srcu_update", however the data structure ended up as "srcu_usage". Maybe that could be updated? include/linux/notifier.h | 10 ++++++++++ 1 file changed, 10 insertions(+)