From: Jiri Kosina jkosina@suse.cz
drm_fb_helper_damage() is acquiring spinlock_t (helper->damage_lock), while it can be called from contexts where raw_spinlock_t is held (e.g. console_owner lock obtained on vprintk_emit() codepath).
As the critical sections protected by damage_lock are super-tiny, let's fix this by converting it to raw_spinlock_t in order not to violate PREEMPT_RT-imposed lock nesting rules.
This fixes the splat below.
============================= [ BUG: Invalid wait context ] 5.17.0-rc4-00002-gd567f5db412e #1 Not tainted ----------------------------- swapper/0/0 is trying to lock: ffff8c5687cc4158 (&helper->damage_lock){....}-{3:3}, at: drm_fb_helper_damage.isra.22+0x4a/0xf0 other info that might help us debug this: context-{2:2} 3 locks held by swapper/0/0: #0: ffffffffad776520 (console_lock){+.+.}-{0:0}, at: vprintk_emit+0xb8/0x2a0 #1: ffffffffad696120 (console_owner){-...}-{0:0}, at: console_unlock+0x17f/0x550 #2: ffffffffad926a58 (printing_lock){....}-{3:3}, at: vt_console_print+0x7d/0x3e0 stack backtrace: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.17.0-rc4-00002-gd567f5db412e #1 bed1d5e19e0e7e8c9d97fd8afa1322f7f47a4f38 Hardware name: LENOVO 20UJS2B905/20UJS2B905, BIOS R1CET63W(1.32 ) 04/09/2021 Call Trace: <IRQ> dump_stack_lvl+0x58/0x71 __lock_acquire+0x165b/0x1780 ? secondary_startup_64_no_verify+0xd5/0xdb lock_acquire+0x278/0x300 ? drm_fb_helper_damage.isra.22+0x4a/0xf0 ? save_trace+0x3e/0x340 ? __bfs+0x10f/0x240 _raw_spin_lock_irqsave+0x48/0x60 ? drm_fb_helper_damage.isra.22+0x4a/0xf0 drm_fb_helper_damage.isra.22+0x4a/0xf0 soft_cursor+0x194/0x240 bit_cursor+0x386/0x630 ? get_color+0x29/0x120 ? bit_putcs+0x4b0/0x4b0 ? console_unlock+0x17f/0x550 hide_cursor+0x2f/0x90 vt_console_print+0x3c5/0x3e0 ? console_unlock+0x17f/0x550 console_unlock+0x515/0x550 vprintk_emit+0x1c8/0x2a0 _printk+0x52/0x6e ? sched_clock_tick+0x3d/0x60 collect_cpu_info_amd+0x93/0xd0 collect_cpu_info_local+0x23/0x30 flush_smp_call_function_queue+0x137/0x220 __sysvec_call_function_single+0x43/0x1c0 sysvec_call_function_single+0x43/0x80 </IRQ> <TASK> asm_sysvec_call_function_single+0x12/0x20 RIP: 0010:cpuidle_enter_state+0x111/0x4b0 Code: 7c ff 45 84 ff 74 17 9c 58 0f 1f 44 00 00 f6 c4 02 0f 85 71 03 00 00 31 ff e8 bb 21 86 ff e8 76 2f 8e ff fb 66 0f 1f 44 00 00 <45> 85 f6 0f 88 12 01 00 00 49 63 d6 4c 2b 24 24 48 8d 04 52 48 8d RSP: 0018:ffffffffad603e48 EFLAGS: 00000206 RAX: 00000000000127c3 RBX: 0000000000000003 RCX: 0000000000000000 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffac32617a RBP: ffff8c5687ba4400 R08: 0000000000000001 R09: 0000000000000001 R10: ffffffffad603e10 R11: 0000000000000000 R12: 00000000685eb4a0 R13: ffffffffad918f80 R14: 0000000000000003 R15: 0000000000000000 ? cpuidle_enter_state+0x10a/0x4b0 ? cpuidle_enter_state+0x10a/0x4b0 cpuidle_enter+0x29/0x40 do_idle+0x24d/0x2c0 cpu_startup_entry+0x19/0x20 start_kernel+0x9c2/0x9e9 secondary_startup_64_no_verify+0xd5/0xdb </TASK>
Signed-off-by: Jiri Kosina jkosina@suse.cz --- drivers/gpu/drm/drm_fb_helper.c | 14 +++++++------- include/drm/drm_fb_helper.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index ed43b987d306..7c4ab6e6f865 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -436,11 +436,11 @@ static void drm_fb_helper_damage_work(struct work_struct *work) unsigned long flags; int ret;
- spin_lock_irqsave(&helper->damage_lock, flags); + raw_spin_lock_irqsave(&helper->damage_lock, flags); clip_copy = *clip; clip->x1 = clip->y1 = ~0; clip->x2 = clip->y2 = 0; - spin_unlock_irqrestore(&helper->damage_lock, flags); + raw_spin_unlock_irqrestore(&helper->damage_lock, flags);
/* Call damage handlers only if necessary */ if (!(clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)) @@ -465,12 +465,12 @@ static void drm_fb_helper_damage_work(struct work_struct *work) * Restore damage clip rectangle on errors. The next run * of the damage worker will perform the update. */ - spin_lock_irqsave(&helper->damage_lock, flags); + raw_spin_lock_irqsave(&helper->damage_lock, flags); clip->x1 = min_t(u32, clip->x1, clip_copy.x1); clip->y1 = min_t(u32, clip->y1, clip_copy.y1); clip->x2 = max_t(u32, clip->x2, clip_copy.x2); clip->y2 = max_t(u32, clip->y2, clip_copy.y2); - spin_unlock_irqrestore(&helper->damage_lock, flags); + raw_spin_unlock_irqrestore(&helper->damage_lock, flags); }
/** @@ -486,7 +486,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, const struct drm_fb_helper_funcs *funcs) { INIT_LIST_HEAD(&helper->kernel_fb_list); - spin_lock_init(&helper->damage_lock); + raw_spin_lock_init(&helper->damage_lock); INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker); INIT_WORK(&helper->damage_work, drm_fb_helper_damage_work); helper->damage_clip.x1 = helper->damage_clip.y1 = ~0; @@ -670,12 +670,12 @@ static void drm_fb_helper_damage(struct fb_info *info, u32 x, u32 y, if (!drm_fbdev_use_shadow_fb(helper)) return;
- spin_lock_irqsave(&helper->damage_lock, flags); + raw_spin_lock_irqsave(&helper->damage_lock, flags); clip->x1 = min_t(u32, clip->x1, x); clip->y1 = min_t(u32, clip->y1, y); clip->x2 = max_t(u32, clip->x2, x + width); clip->y2 = max_t(u32, clip->y2, y + height); - spin_unlock_irqrestore(&helper->damage_lock, flags); + raw_spin_unlock_irqrestore(&helper->damage_lock, flags);
schedule_work(&helper->damage_work); } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 3af4624368d8..91178958896e 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -131,7 +131,7 @@ struct drm_fb_helper { struct fb_info *fbdev; u32 pseudo_palette[17]; struct drm_clip_rect damage_clip; - spinlock_t damage_lock; + raw_spinlock_t damage_lock; struct work_struct damage_work; struct work_struct resume_work;
On 2022-02-15 16:43:08 [+0100], Jiri Kosina wrote:
From: Jiri Kosina jkosina@suse.cz
drm_fb_helper_damage() is acquiring spinlock_t (helper->damage_lock), while it can be called from contexts where raw_spinlock_t is held (e.g. console_owner lock obtained on vprintk_emit() codepath).
As the critical sections protected by damage_lock are super-tiny, let's fix this by converting it to raw_spinlock_t in order not to violate PREEMPT_RT-imposed lock nesting rules.
This fixes the splat below.
============================= [ BUG: Invalid wait context ] 5.17.0-rc4-00002-gd567f5db412e #1 Not tainted
rc4. Is this also the case in the RT tree which includes John's printk changes?
swapper/0/0 is trying to lock: ffff8c5687cc4158 (&helper->damage_lock){....}-{3:3}, at: drm_fb_helper_damage.isra.22+0x4a/0xf0 other info that might help us debug this: context-{2:2} 3 locks held by swapper/0/0: #0: ffffffffad776520 (console_lock){+.+.}-{0:0}, at: vprintk_emit+0xb8/0x2a0 #1: ffffffffad696120 (console_owner){-...}-{0:0}, at: console_unlock+0x17f/0x550 #2: ffffffffad926a58 (printing_lock){....}-{3:3}, at: vt_console_print+0x7d/0x3e0 stack backtrace: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.17.0-rc4-00002-gd567f5db412e #1 bed1d5e19e0e7e8c9d97fd8afa1322f7f47a4f38 Hardware name: LENOVO 20UJS2B905/20UJS2B905, BIOS R1CET63W(1.32 ) 04/09/2021 Call Trace:
<IRQ> dump_stack_lvl+0x58/0x71 __lock_acquire+0x165b/0x1780 ? secondary_startup_64_no_verify+0xd5/0xdb lock_acquire+0x278/0x300 ? drm_fb_helper_damage.isra.22+0x4a/0xf0 ? save_trace+0x3e/0x340 ? __bfs+0x10f/0x240 _raw_spin_lock_irqsave+0x48/0x60 ? drm_fb_helper_damage.isra.22+0x4a/0xf0 drm_fb_helper_damage.isra.22+0x4a/0xf0 soft_cursor+0x194/0x240 bit_cursor+0x386/0x630 ? get_color+0x29/0x120 ? bit_putcs+0x4b0/0x4b0 ? console_unlock+0x17f/0x550 hide_cursor+0x2f/0x90 vt_console_print+0x3c5/0x3e0 ? console_unlock+0x17f/0x550 console_unlock+0x515/0x550 vprintk_emit+0x1c8/0x2a0 _printk+0x52/0x6e ? sched_clock_tick+0x3d/0x60 collect_cpu_info_amd+0x93/0xd0 collect_cpu_info_local+0x23/0x30 flush_smp_call_function_queue+0x137/0x220 __sysvec_call_function_single+0x43/0x1c0 sysvec_call_function_single+0x43/0x80 </IRQ> <TASK> asm_sysvec_call_function_single+0x12/0x20 RIP: 0010:cpuidle_enter_state+0x111/0x4b0 Code: 7c ff 45 84 ff 74 17 9c 58 0f 1f 44 00 00 f6 c4 02 0f 85 71 03 00 00 31 ff e8 bb 21 86 ff e8 76 2f 8e ff fb 66 0f 1f 44 00 00 <45> 85 f6 0f 88 12 01 00 00 49 63 d6 4c 2b 24 24 48 8d 04 52 48 8d RSP: 0018:ffffffffad603e48 EFLAGS: 00000206 RAX: 00000000000127c3 RBX: 0000000000000003 RCX: 0000000000000000 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffac32617a RBP: ffff8c5687ba4400 R08: 0000000000000001 R09: 0000000000000001 R10: ffffffffad603e10 R11: 0000000000000000 R12: 00000000685eb4a0 R13: ffffffffad918f80 R14: 0000000000000003 R15: 0000000000000000 ? cpuidle_enter_state+0x10a/0x4b0 ? cpuidle_enter_state+0x10a/0x4b0 cpuidle_enter+0x29/0x40 do_idle+0x24d/0x2c0 cpu_startup_entry+0x19/0x20 start_kernel+0x9c2/0x9e9 secondary_startup_64_no_verify+0xd5/0xdb </TASK>
Signed-off-by: Jiri Kosina jkosina@suse.cz
drivers/gpu/drm/drm_fb_helper.c | 14 +++++++------- include/drm/drm_fb_helper.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index ed43b987d306..7c4ab6e6f865 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -436,11 +436,11 @@ static void drm_fb_helper_damage_work(struct work_struct *work) unsigned long flags; int ret;
- spin_lock_irqsave(&helper->damage_lock, flags);
- raw_spin_lock_irqsave(&helper->damage_lock, flags); clip_copy = *clip; clip->x1 = clip->y1 = ~0; clip->x2 = clip->y2 = 0;
- spin_unlock_irqrestore(&helper->damage_lock, flags);
raw_spin_unlock_irqrestore(&helper->damage_lock, flags);
/* Call damage handlers only if necessary */ if (!(clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2))
@@ -465,12 +465,12 @@ static void drm_fb_helper_damage_work(struct work_struct *work) * Restore damage clip rectangle on errors. The next run * of the damage worker will perform the update. */
- spin_lock_irqsave(&helper->damage_lock, flags);
- raw_spin_lock_irqsave(&helper->damage_lock, flags); clip->x1 = min_t(u32, clip->x1, clip_copy.x1); clip->y1 = min_t(u32, clip->y1, clip_copy.y1); clip->x2 = max_t(u32, clip->x2, clip_copy.x2); clip->y2 = max_t(u32, clip->y2, clip_copy.y2);
- spin_unlock_irqrestore(&helper->damage_lock, flags);
- raw_spin_unlock_irqrestore(&helper->damage_lock, flags);
}
/** @@ -486,7 +486,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, const struct drm_fb_helper_funcs *funcs) { INIT_LIST_HEAD(&helper->kernel_fb_list);
- spin_lock_init(&helper->damage_lock);
- raw_spin_lock_init(&helper->damage_lock); INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker); INIT_WORK(&helper->damage_work, drm_fb_helper_damage_work); helper->damage_clip.x1 = helper->damage_clip.y1 = ~0;
@@ -670,12 +670,12 @@ static void drm_fb_helper_damage(struct fb_info *info, u32 x, u32 y, if (!drm_fbdev_use_shadow_fb(helper)) return;
- spin_lock_irqsave(&helper->damage_lock, flags);
- raw_spin_lock_irqsave(&helper->damage_lock, flags); clip->x1 = min_t(u32, clip->x1, x); clip->y1 = min_t(u32, clip->y1, y); clip->x2 = max_t(u32, clip->x2, x + width); clip->y2 = max_t(u32, clip->y2, y + height);
- spin_unlock_irqrestore(&helper->damage_lock, flags);
raw_spin_unlock_irqrestore(&helper->damage_lock, flags);
schedule_work(&helper->damage_work);
} diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 3af4624368d8..91178958896e 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -131,7 +131,7 @@ struct drm_fb_helper { struct fb_info *fbdev; u32 pseudo_palette[17]; struct drm_clip_rect damage_clip;
- spinlock_t damage_lock;
- raw_spinlock_t damage_lock; struct work_struct damage_work; struct work_struct resume_work;
Sebastian
On 2022-02-15, Sebastian Siewior bigeasy@linutronix.de wrote:
From: Jiri Kosina jkosina@suse.cz
drm_fb_helper_damage() is acquiring spinlock_t (helper->damage_lock), while it can be called from contexts where raw_spinlock_t is held (e.g. console_owner lock obtained on vprintk_emit() codepath).
As the critical sections protected by damage_lock are super-tiny, let's fix this by converting it to raw_spinlock_t in order not to violate PREEMPT_RT-imposed lock nesting rules.
This fixes the splat below.
============================= [ BUG: Invalid wait context ] 5.17.0-rc4-00002-gd567f5db412e #1 Not tainted
rc4. Is this also the case in the RT tree which includes John's printk changes?
In the RT tree, the fbcon's write() callback is only called in preemptible() contexts. So this is only a mainline issue.
The series I recently posted to LKML [0] should also address this issue (if/when it gets accepted).
John
[0] https://lore.kernel.org/lkml/20220207194323.273637-1-john.ogness@linutronix....
On Tue, 15 Feb 2022, John Ogness wrote:
drm_fb_helper_damage() is acquiring spinlock_t (helper->damage_lock), while it can be called from contexts where raw_spinlock_t is held (e.g. console_owner lock obtained on vprintk_emit() codepath).
As the critical sections protected by damage_lock are super-tiny, let's fix this by converting it to raw_spinlock_t in order not to violate PREEMPT_RT-imposed lock nesting rules.
This fixes the splat below.
============================= [ BUG: Invalid wait context ] 5.17.0-rc4-00002-gd567f5db412e #1 Not tainted
rc4. Is this also the case in the RT tree which includes John's printk changes?
In the RT tree, the fbcon's write() callback is only called in preemptible() contexts. So this is only a mainline issue.
The series I recently posted to LKML [0] should also address this issue (if/when it gets accepted).
John
[0] https://lore.kernel.org/lkml/20220207194323.273637-1-john.ogness@linutronix....
Thanks for confirmation, seems like this problem is indeed cured by your series.
I still believe though that we shouldn't let 5.17 released with this warning being emitted into dmesg, so I'd suggest going with my patch for mainline, and revert it in your series on top of it.
Thanks,
On 2022-02-15 20:59:24 [+0100], Jiri Kosina wrote:
Thanks for confirmation, seems like this problem is indeed cured by your series.
Oki.
I still believe though that we shouldn't let 5.17 released with this warning being emitted into dmesg, so I'd suggest going with my patch for mainline, and revert it in your series on top of it.
No. That warning is only visible with CONFIG_PROVE_RAW_LOCK_NESTING with the following paragraph in its help: | NOTE: There are known nesting problems. So if you enable this | option expect lockdep splats until these problems have been fully | addressed which is work in progress. This config switch allows to | identify and analyze these problems. It will be removed and the | check permanently enabled once the main issues have been fixed.
This warning in this call chain should affect every console driver which acquires a lock.
Thanks,
Sebastian
dri-devel@lists.freedesktop.org