On Fri, Aug 10, 2018 at 10:42 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
>
> On 09-08-18 15:29, Daniel Vetter wrote:
>>
>> On Thu, Aug 9, 2018 at 1:42 PM, Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
