fbdev: Debug knob to register without holding console_lock

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>
---
 drivers/video/fbdev/core/fbmem.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d8883ede..4e73b6f6b1c0 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	return 0;
 }
 
+static bool lockless_register_fb;
+module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400);
+MODULE_PARM_DESC(lockless_register_fb,
+	"Lockless framebuffer registration for debugging [default=off]");
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i, ret;

@@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	registered_fb[i] = fb_info;
 
 	event.info = fb_info;
-	console_lock();
+	if (!lockless_register_fb)
+		console_lock();
 	if (!lock_fb_info(fb_info)) {
-		console_unlock();
+		if (!lockless_register_fb)
+			console_unlock();
 		return -ENODEV;
 	}
 
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
 	unlock_fb_info(fb_info);
-	console_unlock();
+	if (!lockless_register_fb)
+		console_unlock();
 	return 0;
 }
 

-- 
1.8.3.1

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

BR,
-R

9 more quoted lines

On 25/08/15 22:24, Rob Clark wrote:

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

 Tomi

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen [off-list ref] wrote:

On 25/08/15 22:24, Rob Clark wrote:

quoted

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

I hadn't had a chance to look at it further yet..  I think Daniel
claimed it worked for him, but he was probably on intel-next, where I
was on drm-next at the time which seemed to be having some unrelated
i915 issues (when I was trying to debug atomic fb-helper patches).  So
can't really say that the issue I had was actually related to this
patch.  I'll try again later this week or next, when hopefully i915 in
drm-next is in better shape..

BR,
-R

 Tomi

On 01/09/15 17:34, Rob Clark wrote:

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 25/08/15 22:24, Rob Clark wrote:

quoted

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

7 more quoted lines

Oh, I didn't mean this patch, but the whole console lock in general.
I've also banged my head to a wall because of it =).

 Tomi

On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen [off-list ref] wrote:

On 01/09/15 17:34, Rob Clark wrote:

quoted

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 25/08/15 22:24, Rob Clark wrote:

quoted

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..

13 more quoted lines

oh, not sure.. every time I've started looking closer at
console/console_lock I run away screaming..  I guess if it were
possible to push the lock down further so only drivers that needed the
lock (presumably serial/net/etc) could take it, that would be nice..
but not sure I am that brave..

BR,
-R

 Tomi

On Tue, Sep 01, 2015 at 11:12:11AM -0400, Rob Clark wrote:

On Tue, Sep 1, 2015 at 10:41 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 01/09/15 17:34, Rob Clark wrote:

quoted

On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 25/08/15 22:24, Rob Clark wrote:

quoted

On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter [off-list ref] wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
Signed-off-by: Daniel Vetter <redacted>

This one was causing me some problems, if I tried to enable
lockless_register_fb.  It *looks* like it should work, so I'm not
quite sure what the deal is.  But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..

I'll have to debug further on a system where I can see more than the

20 more quoted lines

console_lock is pretty much unfixable without rewriting half of fbdev.
Which I don't expect to ever happen. For the curious look at all the
commits changing locking in fbdev over the past few years.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On 01/09/15 17:34, Rob Clark wrote:

I hadn't had a chance to look at it further yet..  I think Daniel
claimed it worked for him, but he was probably on intel-next, where I
was on drm-next at the time which seemed to be having some unrelated
i915 issues (when I was trying to debug atomic fb-helper patches).  So
can't really say that the issue I had was actually related to this
patch.  I'll try again later this week or next, when hopefully i915 in
drm-next is in better shape..

Rob, did you have a chance to test this?

 Tomi

On 25/08/15 16:45, Daniel Vetter wrote:

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

 Tomi

On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:

On 25/08/15 16:45, Daniel Vetter wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

Patches 1-3 have all already landed in drm, and patch 4 is free standing.
Would be great if you can pull it in.
 

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

Hm, where in fbdev do you hold the console_lock outside of
registering/unregistering an fbdev (because of fbcon)? There's of course
general trouble with console_lock deadlocks and fun like that, but ime
that all got a lot more manageable since I added lockdep annotations to
console_lock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On 08/12/15 10:19, Daniel Vetter wrote:

On Mon, Dec 07, 2015 at 07:32:42PM +0200, Tomi Valkeinen wrote:

quoted

On 25/08/15 16:45, Daniel Vetter wrote:

quoted

When the usual fbcon legacy options are enabled we have
->register_framebuffer
  ->fb notifier chain calls into fbcon
    ->fbcon sets up console on new fbi
      ->fbi->set_par
        ->drm_fb_helper_set_par exercises full kms api

And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).

Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

Patches 1-3 have all already landed in drm, and patch 4 is free standing.
Would be great if you can pull it in.

Ok, I'll apply for 4.5.

quoted

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

Hm, where in fbdev do you hold the console_lock outside of
registering/unregistering an fbdev (because of fbcon)? There's of course
general trouble with console_lock deadlocks and fun like that, but ime
that all got a lot more manageable since I added lockdep annotations to
console_lock.

I don't know... I just have a vague recollection that I was having
trouble with the lock with... crashes during blanking, perhaps. I really
can't remember, so possibly things are better now, or I just remember wrong.

 Tomi