| Message ID | 20180809114215.22028-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fbcon: Do not takeover the console from atomic context | expand |
On Thu, Aug 9, 2018 at 1:42 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Taking over the console involves allocating mem with GFP_KERNEL, talking > to drm drivers, etc. So this should not be done from an atomic context. > > But the console-output trigger deferred console takeover may happen from an > atomic context, which leads to "BUG: sleeping function called from invalid > context" errors. > > This commit fixes these errors by doing the deferred takeover from a > workqueue when the notifier runs from an atomic context. > > Note this uses in_atomic, as checkpatch points out this should not be > done from driver code. But the console subsys is not really normal driver > code, specifically it plays some tricks where it disables some locking > when logging an oops, or when logging a lockdep bug when lockdep debugging > is turned on, so we need to make an exception here. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Add a comment fbcon.c and the commit message about why we need to use > in_atomic here, no functional changes > --- > drivers/video/fbdev/core/fbcon.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index ef8b2d0b7071..31f518f8dde7 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -3592,7 +3592,20 @@ static int fbcon_init_device(void) > } > > #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER > +static void fbcon_register_existing_fbs(struct work_struct *work) > +{ > + int i; > + > + console_lock(); > + > + for_each_registered_fb(i) > + fbcon_fb_registered(registered_fb[i]); > + > + console_unlock(); > +} > + > static struct notifier_block fbcon_output_nb; > +static DECLARE_WORK(fbcon_deferred_takeover_work, fbcon_register_existing_fbs); > > static int fbcon_output_notifier(struct notifier_block *nb, > unsigned long action, void *data) > @@ -3607,8 +3620,20 @@ static int fbcon_output_notifier(struct notifier_block *nb, > deferred_takeover = false; > logo_shown = FBCON_LOGO_DONTSHOW; > > - for_each_registered_fb(i) > - fbcon_fb_registered(registered_fb[i]); > + /* > + * Normally all console output happens in a sleeping context, but > + * during oopses the kernel goes into a special mode where the > + * console code may not sleep. We check for this using in_atomic > + * as the note about in_atomic in preempt.h mentions, in_atomic > + * does not detect a spinlock being held when non-preemptible, so > + * we also check for irqs_disabled which covers this case. > + */ If this is only for oopses, then opps_in_progress is what you're looking for. At least that's what we've switched to in drm_fb_helper.c instead of a cargo-culted pile of in_atimc/irq/kgdb checks. And since the box will die right afterwards, you might as well just bail out directly and not bother with scheduling the worker? Again, that's what we've been doing in the drm fbdev emulation code. -Daniel > + if (in_atomic() || irqs_disabled()) { > + schedule_work(&fbcon_deferred_takeover_work); > + } else { > + for_each_registered_fb(i) > + fbcon_fb_registered(registered_fb[i]); > + } > > return NOTIFY_OK; > } > -- > 2.18.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 09-08-18 15:29, Daniel Vetter wrote: > On Thu, Aug 9, 2018 at 1:42 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Taking over the console involves allocating mem with GFP_KERNEL, talking >> to drm drivers, etc. So this should not be done from an atomic context. >> >> But the console-output trigger deferred console takeover may happen from an >> atomic context, which leads to "BUG: sleeping function called from invalid >> context" errors. >> >> This commit fixes these errors by doing the deferred takeover from a >> workqueue when the notifier runs from an atomic context. >> >> Note this uses in_atomic, as checkpatch points out this should not be >> done from driver code. But the console subsys is not really normal driver >> code, specifically it plays some tricks where it disables some locking >> when logging an oops, or when logging a lockdep bug when lockdep debugging >> is turned on, so we need to make an exception here. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Add a comment fbcon.c and the commit message about why we need to use >> in_atomic here, no functional changes >> --- >> drivers/video/fbdev/core/fbcon.c | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c >> index ef8b2d0b7071..31f518f8dde7 100644 >> --- a/drivers/video/fbdev/core/fbcon.c >> +++ b/drivers/video/fbdev/core/fbcon.c >> @@ -3592,7 +3592,20 @@ static int fbcon_init_device(void) >> } >> >> #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER >> +static void fbcon_register_existing_fbs(struct work_struct *work) >> +{ >> + int i; >> + >> + console_lock(); >> + >> + for_each_registered_fb(i) >> + fbcon_fb_registered(registered_fb[i]); >> + >> + console_unlock(); >> +} >> + >> static struct notifier_block fbcon_output_nb; >> +static DECLARE_WORK(fbcon_deferred_takeover_work, fbcon_register_existing_fbs); >> >> static int fbcon_output_notifier(struct notifier_block *nb, >> unsigned long action, void *data) >> @@ -3607,8 +3620,20 @@ static int fbcon_output_notifier(struct notifier_block *nb, >> deferred_takeover = false; >> logo_shown = FBCON_LOGO_DONTSHOW; >> >> - for_each_registered_fb(i) >> - fbcon_fb_registered(registered_fb[i]); >> + /* >> + * Normally all console output happens in a sleeping context, but >> + * during oopses the kernel goes into a special mode where the >> + * console code may not sleep. We check for this using in_atomic >> + * as the note about in_atomic in preempt.h mentions, in_atomic >> + * does not detect a spinlock being held when non-preemptible, so >> + * we also check for irqs_disabled which covers this case. >> + */ > > If this is only for oopses, then opps_in_progress is what you're > looking for. Unfortunately that is not good enough as mentioned in the v2 commit message, console output may also happen in atomic context when the lockdep code has found an issue (from print_circular_bug). I've tried replacing: if (in_atomic() || irqs_disabled()) { With: if (oops_in_progress) { On a system where I know the lockdep code will report a problem wrt the bttv driver and here is what happened: [ 7.937690] fbcon: Taking over console [ 7.937691] BUG: sleeping function called from invalid context at mm/slab.h:421 [ 7.937691] in_atomic(): 1, irqs_disabled(): 1, pid: 561, name: systemd-udevd [ 7.937692] INFO: lockdep is turned off. [ 7.937692] irq event stamp: 196513 [ 7.937692] hardirqs last enabled at (196513): [<ffffffffbba4314b>] _raw_spin_unlock_irqrestore+0x4b/0x60 [ 7.937692] hardirqs last disabled at (196512): [<ffffffffbba43962>] _raw_spin_lock_irqsave+0x22/0x81 [ 7.937693] softirqs last enabled at (196504): [<ffffffffbbe0038c>] __do_softirq+0x38c/0x4f7 [ 7.937693] softirqs last disabled at (196401): [<ffffffffbb0c19be>] irq_exit+0x10e/0x120 [ 7.937694] CPU: 1 PID: 561 Comm: systemd-udevd Not tainted 4.18.0-0.rc8.git1.1.hdg1.fc29.x86_64 #1 [ 7.937694] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016 [ 7.937694] Call Trace: [ 7.937694] dump_stack+0x85/0xc0 [ 7.937695] ___might_sleep.cold.72+0xac/0xbc [ 7.937695] kmem_cache_alloc_trace+0x202/0x2f0 [ 7.937695] ? fbcon_startup+0xae/0x300 [ 7.937695] fbcon_startup+0xae/0x300 [ 7.937696] do_take_over_console+0x6d/0x180 [ 7.937696] do_fbcon_takeover+0x58/0xb0 [ 7.937696] fbcon_output_notifier.cold.35+0x18/0x44 [ 7.937697] notifier_call_chain+0x39/0x90 [ 7.937697] vt_console_print+0x363/0x420 [ 7.937697] console_unlock+0x422/0x610 [ 7.937697] vprintk_emit+0x268/0x540 [ 7.937698] ? kernel_text_address+0xe5/0xf0 [ 7.937698] printk+0x58/0x6f [ 7.937698] print_circular_bug_header.cold.56+0x17/0x9c [ 7.937698] print_circular_bug.isra.38+0x7c/0xb0 [ 7.937699] __lock_acquire+0x139a/0x16c0 [ 7.937699] lock_acquire+0x9e/0x1b0 [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] [ 7.937700] __mutex_lock+0x88/0xa10 [ 7.937700] ? find_ref_lock+0x21/0x40 [videodev] [ 7.937700] ? bttv_gpio_bits+0x22/0x60 [bttv] [ 7.937700] ? native_sched_clock+0x3e/0xa0 [ 7.937701] ? mark_held_locks+0x57/0x80 [ 7.937701] ? _raw_spin_unlock_irqrestore+0x4b/0x60 [ 7.937701] ? find_ref_lock+0x21/0x40 [videodev] [ 7.937701] find_ref_lock+0x21/0x40 [videodev] [ 7.937702] v4l2_ctrl_find+0xa/0x20 [videodev] [ 7.937702] audio_mute+0x38/0x120 [bttv] [ 7.937702] bttv_s_ctrl+0x2d5/0x3b0 [bttv] [ 7.937702] __v4l2_ctrl_handler_setup+0xdf/0x130 [videodev] [ 7.937703] v4l2_ctrl_handler_setup+0x27/0x40 [videodev] [ 7.937703] bttv_probe.cold.45+0x8f4/0xca7 [bttv] [ 7.937703] local_pci_probe+0x41/0x90 [ 7.937704] pci_device_probe+0x118/0x1a0 [ 7.937704] driver_probe_device+0x2da/0x450 [ 7.937704] __driver_attach+0xe1/0x110 [ 7.937704] ? driver_probe_device+0x450/0x450 [ 7.937705] ? driver_probe_device+0x450/0x450 [ 7.937705] bus_for_each_dev+0x79/0xc0 [ 7.937705] ? deferred_probe_initcall+0x30/0x30 [ 7.937705] bus_add_driver+0x155/0x230 [ 7.937706] ? 0xffffffffc07a4000 [ 7.937706] driver_register+0x6b/0xb0 [ 7.937706] ? 0xffffffffc07a4000 [ 7.937706] bttv_init_module+0xcd/0xe3 [bttv] [ 7.937706] do_one_initcall+0x5d/0x362 [ 7.937707] ? rcu_read_lock_sched_held+0x6b/0x80 [ 7.937707] ? kmem_cache_alloc_trace+0x293/0x2f0 [ 7.937707] ? do_init_module+0x22/0x210 [ 7.937707] do_init_module+0x5a/0x210 [ 7.937708] load_module+0x21e6/0x2610 [ 7.937708] ? ima_post_read_file+0x10c/0x119 [ 7.937708] ? __do_sys_finit_module+0xad/0x110 [ 7.937708] __do_sys_finit_module+0xad/0x110 [ 7.937709] do_syscall_64+0x60/0x1f0 [ 7.937709] entry_SYSCALL_64_after_hwframe+0x49/0xbe Follow by a printing of the actual lockdep issue. This is what I wrote this patch for in the first place and checking for oops_in_progress does not fix this case. So I believe we need to keep using the: if (in_atomic() || irqs_disabled()) { Check, Bartlomiej can you apply v2 please, or let me know if you want any other changes? > At least that's what we've switched to in drm_fb_helper.c > instead of a cargo-culted pile of in_atimc/irq/kgdb checks. And since > the box will die right afterwards, you might as well just bail out > directly and not bother with scheduling the worker? Well with a lockdep bug the box won't die, depending on the actual locking issue it may be quite harmless. Regards, Hans > Again, that's what > we've been doing in the drm fbdev emulation code. > -Daniel > >> + if (in_atomic() || irqs_disabled()) { >> + schedule_work(&fbcon_deferred_takeover_work); >> + } else { >> + for_each_registered_fb(i) >> + fbcon_fb_registered(registered_fb[i]); >> + } >> >> return NOTIFY_OK; >> } >> -- >> 2.18.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
On Fri, Aug 10, 2018 at 10:42 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 09-08-18 15:29, Daniel Vetter wrote: >> >> On Thu, Aug 9, 2018 at 1:42 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Taking over the console involves allocating mem with GFP_KERNEL, talking >>> to drm drivers, etc. So this should not be done from an atomic context. >>> >>> But the console-output trigger deferred console takeover may happen from >>> an >>> atomic context, which leads to "BUG: sleeping function called from >>> invalid >>> context" errors. >>> >>> This commit fixes these errors by doing the deferred takeover from a >>> workqueue when the notifier runs from an atomic context. >>> >>> Note this uses in_atomic, as checkpatch points out this should not be >>> done from driver code. But the console subsys is not really normal driver >>> code, specifically it plays some tricks where it disables some locking >>> when logging an oops, or when logging a lockdep bug when lockdep >>> debugging >>> is turned on, so we need to make an exception here. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Changes in v2: >>> -Add a comment fbcon.c and the commit message about why we need to use >>> in_atomic here, no functional changes >>> --- >>> drivers/video/fbdev/core/fbcon.c | 29 +++++++++++++++++++++++++++-- >>> 1 file changed, 27 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/core/fbcon.c >>> b/drivers/video/fbdev/core/fbcon.c >>> index ef8b2d0b7071..31f518f8dde7 100644 >>> --- a/drivers/video/fbdev/core/fbcon.c >>> +++ b/drivers/video/fbdev/core/fbcon.c >>> @@ -3592,7 +3592,20 @@ static int fbcon_init_device(void) >>> } >>> >>> #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER >>> +static void fbcon_register_existing_fbs(struct work_struct *work) >>> +{ >>> + int i; >>> + >>> + console_lock(); >>> + >>> + for_each_registered_fb(i) >>> + fbcon_fb_registered(registered_fb[i]); >>> + >>> + console_unlock(); >>> +} >>> + >>> static struct notifier_block fbcon_output_nb; >>> +static DECLARE_WORK(fbcon_deferred_takeover_work, >>> fbcon_register_existing_fbs); >>> >>> static int fbcon_output_notifier(struct notifier_block *nb, >>> unsigned long action, void *data) >>> @@ -3607,8 +3620,20 @@ static int fbcon_output_notifier(struct >>> notifier_block *nb, >>> deferred_takeover = false; >>> logo_shown = FBCON_LOGO_DONTSHOW; >>> >>> - for_each_registered_fb(i) >>> - fbcon_fb_registered(registered_fb[i]); >>> + /* >>> + * Normally all console output happens in a sleeping context, but >>> + * during oopses the kernel goes into a special mode where the >>> + * console code may not sleep. We check for this using in_atomic >>> + * as the note about in_atomic in preempt.h mentions, in_atomic >>> + * does not detect a spinlock being held when non-preemptible, so >>> + * we also check for irqs_disabled which covers this case. >>> + */ >> >> >> If this is only for oopses, then opps_in_progress is what you're >> looking for. > > > Unfortunately that is not good enough as mentioned in the v2 commit > message, console output may also happen in atomic context when the > lockdep code has found an issue (from print_circular_bug). > > I've tried replacing: > > if (in_atomic() || irqs_disabled()) { > > With: > > if (oops_in_progress) { You can't use in_atomic() either, because it can't detect atomic regions on non-preemptible kernels. Unconditionally punting to a worker is the only solution here I think. See e.g the entire history around how we call the ->dirty callback in the drm fbdev emulation. The only generic approach that actually works is drm_fb_helper_dirty unconditionally offloading everything to a worker. -Daniel > On a system where I know the lockdep code will report a problem > wrt the bttv driver and here is what happened: > > [ 7.937690] fbcon: Taking over console > [ 7.937691] BUG: sleeping function called from invalid context at > mm/slab.h:421 > [ 7.937691] in_atomic(): 1, irqs_disabled(): 1, pid: 561, name: > systemd-udevd > [ 7.937692] INFO: lockdep is turned off. > [ 7.937692] irq event stamp: 196513 > [ 7.937692] hardirqs last enabled at (196513): [<ffffffffbba4314b>] > _raw_spin_unlock_irqrestore+0x4b/0x60 > [ 7.937692] hardirqs last disabled at (196512): [<ffffffffbba43962>] > _raw_spin_lock_irqsave+0x22/0x81 > [ 7.937693] softirqs last enabled at (196504): [<ffffffffbbe0038c>] > __do_softirq+0x38c/0x4f7 > [ 7.937693] softirqs last disabled at (196401): [<ffffffffbb0c19be>] > irq_exit+0x10e/0x120 > [ 7.937694] CPU: 1 PID: 561 Comm: systemd-udevd Not tainted > 4.18.0-0.rc8.git1.1.hdg1.fc29.x86_64 #1 > [ 7.937694] Hardware name: To Be Filled By O.E.M. To Be Filled By > O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016 > [ 7.937694] Call Trace: > [ 7.937694] dump_stack+0x85/0xc0 > [ 7.937695] ___might_sleep.cold.72+0xac/0xbc > [ 7.937695] kmem_cache_alloc_trace+0x202/0x2f0 > [ 7.937695] ? fbcon_startup+0xae/0x300 > [ 7.937695] fbcon_startup+0xae/0x300 > [ 7.937696] do_take_over_console+0x6d/0x180 > [ 7.937696] do_fbcon_takeover+0x58/0xb0 > [ 7.937696] fbcon_output_notifier.cold.35+0x18/0x44 > [ 7.937697] notifier_call_chain+0x39/0x90 > [ 7.937697] vt_console_print+0x363/0x420 > [ 7.937697] console_unlock+0x422/0x610 > [ 7.937697] vprintk_emit+0x268/0x540 > [ 7.937698] ? kernel_text_address+0xe5/0xf0 > [ 7.937698] printk+0x58/0x6f > [ 7.937698] print_circular_bug_header.cold.56+0x17/0x9c > [ 7.937698] print_circular_bug.isra.38+0x7c/0xb0 > [ 7.937699] __lock_acquire+0x139a/0x16c0 > [ 7.937699] lock_acquire+0x9e/0x1b0 > [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] > [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] > [ 7.937700] __mutex_lock+0x88/0xa10 > [ 7.937700] ? find_ref_lock+0x21/0x40 [videodev] > [ 7.937700] ? bttv_gpio_bits+0x22/0x60 [bttv] > [ 7.937700] ? native_sched_clock+0x3e/0xa0 > [ 7.937701] ? mark_held_locks+0x57/0x80 > [ 7.937701] ? _raw_spin_unlock_irqrestore+0x4b/0x60 > [ 7.937701] ? find_ref_lock+0x21/0x40 [videodev] > [ 7.937701] find_ref_lock+0x21/0x40 [videodev] > [ 7.937702] v4l2_ctrl_find+0xa/0x20 [videodev] > [ 7.937702] audio_mute+0x38/0x120 [bttv] > [ 7.937702] bttv_s_ctrl+0x2d5/0x3b0 [bttv] > [ 7.937702] __v4l2_ctrl_handler_setup+0xdf/0x130 [videodev] > [ 7.937703] v4l2_ctrl_handler_setup+0x27/0x40 [videodev] > [ 7.937703] bttv_probe.cold.45+0x8f4/0xca7 [bttv] > [ 7.937703] local_pci_probe+0x41/0x90 > [ 7.937704] pci_device_probe+0x118/0x1a0 > [ 7.937704] driver_probe_device+0x2da/0x450 > [ 7.937704] __driver_attach+0xe1/0x110 > [ 7.937704] ? driver_probe_device+0x450/0x450 > [ 7.937705] ? driver_probe_device+0x450/0x450 > [ 7.937705] bus_for_each_dev+0x79/0xc0 > [ 7.937705] ? deferred_probe_initcall+0x30/0x30 > [ 7.937705] bus_add_driver+0x155/0x230 > [ 7.937706] ? 0xffffffffc07a4000 > [ 7.937706] driver_register+0x6b/0xb0 > [ 7.937706] ? 0xffffffffc07a4000 > [ 7.937706] bttv_init_module+0xcd/0xe3 [bttv] > [ 7.937706] do_one_initcall+0x5d/0x362 > [ 7.937707] ? rcu_read_lock_sched_held+0x6b/0x80 > [ 7.937707] ? kmem_cache_alloc_trace+0x293/0x2f0 > [ 7.937707] ? do_init_module+0x22/0x210 > [ 7.937707] do_init_module+0x5a/0x210 > [ 7.937708] load_module+0x21e6/0x2610 > [ 7.937708] ? ima_post_read_file+0x10c/0x119 > [ 7.937708] ? __do_sys_finit_module+0xad/0x110 > [ 7.937708] __do_sys_finit_module+0xad/0x110 > [ 7.937709] do_syscall_64+0x60/0x1f0 > [ 7.937709] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Follow by a printing of the actual lockdep issue. > > This is what I wrote this patch for in the first place > and checking for oops_in_progress does not fix this case. > > So I believe we need to keep using the: > > if (in_atomic() || irqs_disabled()) { > > Check, Bartlomiej can you apply v2 please, or let me know if > you want any other changes? > >> At least that's what we've switched to in drm_fb_helper.c >> instead of a cargo-culted pile of in_atimc/irq/kgdb checks. And since >> the box will die right afterwards, you might as well just bail out >> directly and not bother with scheduling the worker? > > > Well with a lockdep bug the box won't die, depending on the actual > locking issue it may be quite harmless. > > Regards, > > Hans > > > > >> Again, that's what >> we've been doing in the drm fbdev emulation code. >> -Daniel >> >>> + if (in_atomic() || irqs_disabled()) { >>> + schedule_work(&fbcon_deferred_takeover_work); >>> + } else { >>> + for_each_registered_fb(i) >>> + fbcon_fb_registered(registered_fb[i]); >>> + } >>> >>> return NOTIFY_OK; >>> } >>> -- >>> 2.18.0 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> >> >
HI, On 10-08-18 10:50, Daniel Vetter wrote: > On Fri, Aug 10, 2018 at 10:42 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 09-08-18 15:29, Daniel Vetter wrote: >>> >>> On Thu, Aug 9, 2018 at 1:42 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Taking over the console involves allocating mem with GFP_KERNEL, talking >>>> to drm drivers, etc. So this should not be done from an atomic context. >>>> >>>> But the console-output trigger deferred console takeover may happen from >>>> an >>>> atomic context, which leads to "BUG: sleeping function called from >>>> invalid >>>> context" errors. >>>> >>>> This commit fixes these errors by doing the deferred takeover from a >>>> workqueue when the notifier runs from an atomic context. >>>> >>>> Note this uses in_atomic, as checkpatch points out this should not be >>>> done from driver code. But the console subsys is not really normal driver >>>> code, specifically it plays some tricks where it disables some locking >>>> when logging an oops, or when logging a lockdep bug when lockdep >>>> debugging >>>> is turned on, so we need to make an exception here. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Changes in v2: >>>> -Add a comment fbcon.c and the commit message about why we need to use >>>> in_atomic here, no functional changes >>>> --- >>>> drivers/video/fbdev/core/fbcon.c | 29 +++++++++++++++++++++++++++-- >>>> 1 file changed, 27 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/video/fbdev/core/fbcon.c >>>> b/drivers/video/fbdev/core/fbcon.c >>>> index ef8b2d0b7071..31f518f8dde7 100644 >>>> --- a/drivers/video/fbdev/core/fbcon.c >>>> +++ b/drivers/video/fbdev/core/fbcon.c >>>> @@ -3592,7 +3592,20 @@ static int fbcon_init_device(void) >>>> } >>>> >>>> #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER >>>> +static void fbcon_register_existing_fbs(struct work_struct *work) >>>> +{ >>>> + int i; >>>> + >>>> + console_lock(); >>>> + >>>> + for_each_registered_fb(i) >>>> + fbcon_fb_registered(registered_fb[i]); >>>> + >>>> + console_unlock(); >>>> +} >>>> + >>>> static struct notifier_block fbcon_output_nb; >>>> +static DECLARE_WORK(fbcon_deferred_takeover_work, >>>> fbcon_register_existing_fbs); >>>> >>>> static int fbcon_output_notifier(struct notifier_block *nb, >>>> unsigned long action, void *data) >>>> @@ -3607,8 +3620,20 @@ static int fbcon_output_notifier(struct >>>> notifier_block *nb, >>>> deferred_takeover = false; >>>> logo_shown = FBCON_LOGO_DONTSHOW; >>>> >>>> - for_each_registered_fb(i) >>>> - fbcon_fb_registered(registered_fb[i]); >>>> + /* >>>> + * Normally all console output happens in a sleeping context, but >>>> + * during oopses the kernel goes into a special mode where the >>>> + * console code may not sleep. We check for this using in_atomic >>>> + * as the note about in_atomic in preempt.h mentions, in_atomic >>>> + * does not detect a spinlock being held when non-preemptible, so >>>> + * we also check for irqs_disabled which covers this case. >>>> + */ >>> >>> >>> If this is only for oopses, then opps_in_progress is what you're >>> looking for. >> >> >> Unfortunately that is not good enough as mentioned in the v2 commit >> message, console output may also happen in atomic context when the >> lockdep code has found an issue (from print_circular_bug). >> >> I've tried replacing: >> >> if (in_atomic() || irqs_disabled()) { >> >> With: >> >> if (oops_in_progress) { > > You can't use in_atomic() either, because it can't detect atomic > regions on non-preemptible kernels. Unconditionally punting to a > worker is the only solution here I think. > > See e.g the entire history around how we call the ->dirty callback in > the drm fbdev emulation. The only generic approach that actually works > is drm_fb_helper_dirty unconditionally offloading everything to a > worker. Ok, I don't see any downsides to doing the takeover in a worker unconditionally, so I will prepare a v3 doing this. Regards, Hans > -Daniel > >> On a system where I know the lockdep code will report a problem >> wrt the bttv driver and here is what happened: >> >> [ 7.937690] fbcon: Taking over console >> [ 7.937691] BUG: sleeping function called from invalid context at >> mm/slab.h:421 >> [ 7.937691] in_atomic(): 1, irqs_disabled(): 1, pid: 561, name: >> systemd-udevd >> [ 7.937692] INFO: lockdep is turned off. >> [ 7.937692] irq event stamp: 196513 >> [ 7.937692] hardirqs last enabled at (196513): [<ffffffffbba4314b>] >> _raw_spin_unlock_irqrestore+0x4b/0x60 >> [ 7.937692] hardirqs last disabled at (196512): [<ffffffffbba43962>] >> _raw_spin_lock_irqsave+0x22/0x81 >> [ 7.937693] softirqs last enabled at (196504): [<ffffffffbbe0038c>] >> __do_softirq+0x38c/0x4f7 >> [ 7.937693] softirqs last disabled at (196401): [<ffffffffbb0c19be>] >> irq_exit+0x10e/0x120 >> [ 7.937694] CPU: 1 PID: 561 Comm: systemd-udevd Not tainted >> 4.18.0-0.rc8.git1.1.hdg1.fc29.x86_64 #1 >> [ 7.937694] Hardware name: To Be Filled By O.E.M. To Be Filled By >> O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016 >> [ 7.937694] Call Trace: >> [ 7.937694] dump_stack+0x85/0xc0 >> [ 7.937695] ___might_sleep.cold.72+0xac/0xbc >> [ 7.937695] kmem_cache_alloc_trace+0x202/0x2f0 >> [ 7.937695] ? fbcon_startup+0xae/0x300 >> [ 7.937695] fbcon_startup+0xae/0x300 >> [ 7.937696] do_take_over_console+0x6d/0x180 >> [ 7.937696] do_fbcon_takeover+0x58/0xb0 >> [ 7.937696] fbcon_output_notifier.cold.35+0x18/0x44 >> [ 7.937697] notifier_call_chain+0x39/0x90 >> [ 7.937697] vt_console_print+0x363/0x420 >> [ 7.937697] console_unlock+0x422/0x610 >> [ 7.937697] vprintk_emit+0x268/0x540 >> [ 7.937698] ? kernel_text_address+0xe5/0xf0 >> [ 7.937698] printk+0x58/0x6f >> [ 7.937698] print_circular_bug_header.cold.56+0x17/0x9c >> [ 7.937698] print_circular_bug.isra.38+0x7c/0xb0 >> [ 7.937699] __lock_acquire+0x139a/0x16c0 >> [ 7.937699] lock_acquire+0x9e/0x1b0 >> [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] >> [ 7.937699] ? find_ref_lock+0x21/0x40 [videodev] >> [ 7.937700] __mutex_lock+0x88/0xa10 >> [ 7.937700] ? find_ref_lock+0x21/0x40 [videodev] >> [ 7.937700] ? bttv_gpio_bits+0x22/0x60 [bttv] >> [ 7.937700] ? native_sched_clock+0x3e/0xa0 >> [ 7.937701] ? mark_held_locks+0x57/0x80 >> [ 7.937701] ? _raw_spin_unlock_irqrestore+0x4b/0x60 >> [ 7.937701] ? find_ref_lock+0x21/0x40 [videodev] >> [ 7.937701] find_ref_lock+0x21/0x40 [videodev] >> [ 7.937702] v4l2_ctrl_find+0xa/0x20 [videodev] >> [ 7.937702] audio_mute+0x38/0x120 [bttv] >> [ 7.937702] bttv_s_ctrl+0x2d5/0x3b0 [bttv] >> [ 7.937702] __v4l2_ctrl_handler_setup+0xdf/0x130 [videodev] >> [ 7.937703] v4l2_ctrl_handler_setup+0x27/0x40 [videodev] >> [ 7.937703] bttv_probe.cold.45+0x8f4/0xca7 [bttv] >> [ 7.937703] local_pci_probe+0x41/0x90 >> [ 7.937704] pci_device_probe+0x118/0x1a0 >> [ 7.937704] driver_probe_device+0x2da/0x450 >> [ 7.937704] __driver_attach+0xe1/0x110 >> [ 7.937704] ? driver_probe_device+0x450/0x450 >> [ 7.937705] ? driver_probe_device+0x450/0x450 >> [ 7.937705] bus_for_each_dev+0x79/0xc0 >> [ 7.937705] ? deferred_probe_initcall+0x30/0x30 >> [ 7.937705] bus_add_driver+0x155/0x230 >> [ 7.937706] ? 0xffffffffc07a4000 >> [ 7.937706] driver_register+0x6b/0xb0 >> [ 7.937706] ? 0xffffffffc07a4000 >> [ 7.937706] bttv_init_module+0xcd/0xe3 [bttv] >> [ 7.937706] do_one_initcall+0x5d/0x362 >> [ 7.937707] ? rcu_read_lock_sched_held+0x6b/0x80 >> [ 7.937707] ? kmem_cache_alloc_trace+0x293/0x2f0 >> [ 7.937707] ? do_init_module+0x22/0x210 >> [ 7.937707] do_init_module+0x5a/0x210 >> [ 7.937708] load_module+0x21e6/0x2610 >> [ 7.937708] ? ima_post_read_file+0x10c/0x119 >> [ 7.937708] ? __do_sys_finit_module+0xad/0x110 >> [ 7.937708] __do_sys_finit_module+0xad/0x110 >> [ 7.937709] do_syscall_64+0x60/0x1f0 >> [ 7.937709] entry_SYSCALL_64_after_hwframe+0x49/0xbe >> >> Follow by a printing of the actual lockdep issue. >> >> This is what I wrote this patch for in the first place >> and checking for oops_in_progress does not fix this case. >> >> So I believe we need to keep using the: >> >> if (in_atomic() || irqs_disabled()) { >> >> Check, Bartlomiej can you apply v2 please, or let me know if >> you want any other changes? >> >>> At least that's what we've switched to in drm_fb_helper.c >>> instead of a cargo-culted pile of in_atimc/irq/kgdb checks. And since >>> the box will die right afterwards, you might as well just bail out >>> directly and not bother with scheduling the worker? >> >> >> Well with a lockdep bug the box won't die, depending on the actual >> locking issue it may be quite harmless. >> >> Regards, >> >> Hans >> >> >> >> >>> Again, that's what >>> we've been doing in the drm fbdev emulation code. >>> -Daniel >>> >>>> + if (in_atomic() || irqs_disabled()) { >>>> + schedule_work(&fbcon_deferred_takeover_work); >>>> + } else { >>>> + for_each_registered_fb(i) >>>> + fbcon_fb_registered(registered_fb[i]); >>>> + } >>>> >>>> return NOTIFY_OK; >>>> } >>>> -- >>>> 2.18.0 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> >>> >>> >> > > >
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index ef8b2d0b7071..31f518f8dde7 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -3592,7 +3592,20 @@ static int fbcon_init_device(void) } #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER +static void fbcon_register_existing_fbs(struct work_struct *work) +{ + int i; + + console_lock(); + + for_each_registered_fb(i) + fbcon_fb_registered(registered_fb[i]); + + console_unlock(); +} + static struct notifier_block fbcon_output_nb; +static DECLARE_WORK(fbcon_deferred_takeover_work, fbcon_register_existing_fbs); static int fbcon_output_notifier(struct notifier_block *nb, unsigned long action, void *data) @@ -3607,8 +3620,20 @@ static int fbcon_output_notifier(struct notifier_block *nb, deferred_takeover = false; logo_shown = FBCON_LOGO_DONTSHOW; - for_each_registered_fb(i) - fbcon_fb_registered(registered_fb[i]); + /* + * Normally all console output happens in a sleeping context, but + * during oopses the kernel goes into a special mode where the + * console code may not sleep. We check for this using in_atomic + * as the note about in_atomic in preempt.h mentions, in_atomic + * does not detect a spinlock being held when non-preemptible, so + * we also check for irqs_disabled which covers this case. + */ + if (in_atomic() || irqs_disabled()) { + schedule_work(&fbcon_deferred_takeover_work); + } else { + for_each_registered_fb(i) + fbcon_fb_registered(registered_fb[i]); + } return NOTIFY_OK; }
Taking over the console involves allocating mem with GFP_KERNEL, talking to drm drivers, etc. So this should not be done from an atomic context. But the console-output trigger deferred console takeover may happen from an atomic context, which leads to "BUG: sleeping function called from invalid context" errors. This commit fixes these errors by doing the deferred takeover from a workqueue when the notifier runs from an atomic context. Note this uses in_atomic, as checkpatch points out this should not be done from driver code. But the console subsys is not really normal driver code, specifically it plays some tricks where it disables some locking when logging an oops, or when logging a lockdep bug when lockdep debugging is turned on, so we need to make an exception here. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Add a comment fbcon.c and the commit message about why we need to use in_atomic here, no functional changes --- drivers/video/fbdev/core/fbcon.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)