diff mbox series

[v3,14/17] fbcon: Move console_lock for register/unlink/unregister

Message ID 20220405210335.3434130-15-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series fbcon cleanups | expand

Commit Message

Daniel Vetter April 5, 2022, 9:03 p.m. UTC 
Ideally console_lock becomes an implementation detail of fbcon.c and
doesn't show up anywhere in fbmem.c. We're still pretty far from that,
but at least the register/unregister code is there now.

With this the do_fb_ioctl() handler is the only code in fbmem.c still
calling console_lock().

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Zheyu Ma <zheyuma97@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
---
 drivers/video/fbdev/core/fbcon.c | 33 ++++++++++++++++++++++++++------
 drivers/video/fbdev/core/fbmem.c | 23 ++--------------------
 2 files changed, 29 insertions(+), 27 deletions(-)

Comments

Nathan Chancellor April 11, 2022, 10:27 p.m. UTC | #1 
Hi Daniel,

On Tue, Apr 05, 2022 at 11:03:32PM +0200, Daniel Vetter wrote:
> Ideally console_lock becomes an implementation detail of fbcon.c and
> doesn't show up anywhere in fbmem.c. We're still pretty far from that,
> but at least the register/unregister code is there now.
> 
> With this the do_fb_ioctl() handler is the only code in fbmem.c still
> calling console_lock().
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Zheyu Ma <zheyuma97@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Zhen Lei <thunder.leizhen@huawei.com>
> Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>

This patch as commit 6e7da3af008b ("fbcon: Move console_lock for
register/unlink/unregister") in next-20220411 causes a lock up on my
test Intel desktop; I get no output on my display and I cannot ssh into
the machine. Bisect log below. If back out of this patch and the ones
that follow in this series on top of next-20220411, everything works
properly (i.e. 'git diff 6e7da3af008b^..efc3acbc105a | git apply -R').

What information would be helpful for debugging this? The system has an
i7-11700 in it and it is booting under UEFI, so I assume it should be
using EFI_FB. I am happy to offer any debugging information or test any
patches.

# bad: [d12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14] Add linux-next specific files for 20220411
# good: [8b57b3046107b50ebecb65537a172ef3d6cec673] Merge tag 'tty-5.18-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
git bisect start 'd12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14' '8b57b3046107b50ebecb65537a172ef3d6cec673'
# bad: [8f0b3ef070bd35e80da1caa85824fc344fb6b82e] Merge branch 'for-linux-next' of git://anongit.freedesktop.org/drm/drm-misc
git bisect bad 8f0b3ef070bd35e80da1caa85824fc344fb6b82e
# good: [69ae2d6587df089ec1c21bddddc7852df403d3ce] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
git bisect good 69ae2d6587df089ec1c21bddddc7852df403d3ce
# good: [43ffc7321ed6ce13e5424fa687576f4442c25f9d] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
git bisect good 43ffc7321ed6ce13e5424fa687576f4442c25f9d
# good: [d44c2642c40b6c11e4a0afc76d491287c074376c] drm/gem: Delete gem array fencing helpers
git bisect good d44c2642c40b6c11e4a0afc76d491287c074376c
# good: [3223e922ccf8b5c3dd0b05faeaba407655ee0774] orinoco: Prepare cleanup of powerpc's asm/prom.h
git bisect good 3223e922ccf8b5c3dd0b05faeaba407655ee0774
# good: [689333136327b6cd618df85d83d79f2aa620d585] fbcon: Move fbcon_bmove(_rec) functions
git bisect good 689333136327b6cd618df85d83d79f2aa620d585
# good: [840db0076b05a1dd4c2ded8d08f8481e0297d09f] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good 840db0076b05a1dd4c2ded8d08f8481e0297d09f
# bad: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister
git bisect bad 6e7da3af008b72520f5318507f455f344b27f022
# good: [6b2060cf9138a2cd5f3468a949d3869abed049ef] fb: Delete fb_info->queue
git bisect good 6b2060cf9138a2cd5f3468a949d3869abed049ef
# good: [d443d93864726ad68c0a741d1e7b03934a9af143] fbcon: move more common code into fb_open()
git bisect good d443d93864726ad68c0a741d1e7b03934a9af143
# good: [43553559121ca90965b572cf8a1d6d0fd618b449] fbcon: Consistently protect deferred_takeover with console_lock()
git bisect good 43553559121ca90965b572cf8a1d6d0fd618b449
# first bad commit: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister

Cheers,
Nathan
Daniel Vetter April 13, 2022, 8:13 a.m. UTC | #2 
On Mon, Apr 11, 2022 at 03:27:47PM -0700, Nathan Chancellor wrote:
> Hi Daniel,
> 
> On Tue, Apr 05, 2022 at 11:03:32PM +0200, Daniel Vetter wrote:
> > Ideally console_lock becomes an implementation detail of fbcon.c and
> > doesn't show up anywhere in fbmem.c. We're still pretty far from that,
> > but at least the register/unregister code is there now.
> > 
> > With this the do_fb_ioctl() handler is the only code in fbmem.c still
> > calling console_lock().
> > 
> > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Du Cheng <ducheng2@gmail.com>
> > Cc: Claudio Suarez <cssk@net-c.es>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Zheyu Ma <zheyuma97@gmail.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Zhen Lei <thunder.leizhen@huawei.com>
> > Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> 
> This patch as commit 6e7da3af008b ("fbcon: Move console_lock for
> register/unlink/unregister") in next-20220411 causes a lock up on my
> test Intel desktop; I get no output on my display and I cannot ssh into
> the machine. Bisect log below. If back out of this patch and the ones
> that follow in this series on top of next-20220411, everything works
> properly (i.e. 'git diff 6e7da3af008b^..efc3acbc105a | git apply -R').
> 
> What information would be helpful for debugging this? The system has an
> i7-11700 in it and it is booting under UEFI, so I assume it should be
> using EFI_FB. I am happy to offer any debugging information or test any
> patches.

I think I got it, typing a patch now for you to test. Thanks a lot for the
bug report and bisect!
-Daniel

> 
> # bad: [d12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14] Add linux-next specific files for 20220411
> # good: [8b57b3046107b50ebecb65537a172ef3d6cec673] Merge tag 'tty-5.18-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
> git bisect start 'd12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14' '8b57b3046107b50ebecb65537a172ef3d6cec673'
> # bad: [8f0b3ef070bd35e80da1caa85824fc344fb6b82e] Merge branch 'for-linux-next' of git://anongit.freedesktop.org/drm/drm-misc
> git bisect bad 8f0b3ef070bd35e80da1caa85824fc344fb6b82e
> # good: [69ae2d6587df089ec1c21bddddc7852df403d3ce] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
> git bisect good 69ae2d6587df089ec1c21bddddc7852df403d3ce
> # good: [43ffc7321ed6ce13e5424fa687576f4442c25f9d] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
> git bisect good 43ffc7321ed6ce13e5424fa687576f4442c25f9d
> # good: [d44c2642c40b6c11e4a0afc76d491287c074376c] drm/gem: Delete gem array fencing helpers
> git bisect good d44c2642c40b6c11e4a0afc76d491287c074376c
> # good: [3223e922ccf8b5c3dd0b05faeaba407655ee0774] orinoco: Prepare cleanup of powerpc's asm/prom.h
> git bisect good 3223e922ccf8b5c3dd0b05faeaba407655ee0774
> # good: [689333136327b6cd618df85d83d79f2aa620d585] fbcon: Move fbcon_bmove(_rec) functions
> git bisect good 689333136327b6cd618df85d83d79f2aa620d585
> # good: [840db0076b05a1dd4c2ded8d08f8481e0297d09f] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> git bisect good 840db0076b05a1dd4c2ded8d08f8481e0297d09f
> # bad: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister
> git bisect bad 6e7da3af008b72520f5318507f455f344b27f022
> # good: [6b2060cf9138a2cd5f3468a949d3869abed049ef] fb: Delete fb_info->queue
> git bisect good 6b2060cf9138a2cd5f3468a949d3869abed049ef
> # good: [d443d93864726ad68c0a741d1e7b03934a9af143] fbcon: move more common code into fb_open()
> git bisect good d443d93864726ad68c0a741d1e7b03934a9af143
> # good: [43553559121ca90965b572cf8a1d6d0fd618b449] fbcon: Consistently protect deferred_takeover with console_lock()
> git bisect good 43553559121ca90965b572cf8a1d6d0fd618b449
> # first bad commit: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister
> 
> Cheers,
> Nathan
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 4f9752ee9189..abb419a091c6 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2756,10 +2756,12 @@  void fbcon_fb_unbind(struct fb_info *info)
 	int i, new_idx = -1;
 	int idx = info->node;
 
-	WARN_CONSOLE_UNLOCKED();
+	console_lock();
 
-	if (!fbcon_has_console_bind)
+	if (!fbcon_has_console_bind) {
+		console_unlock();
 		return;
+	}
 
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		if (con2fb_map[i] != idx &&
@@ -2794,6 +2796,8 @@  void fbcon_fb_unbind(struct fb_info *info)
 		}
 		fbcon_unbind();
 	}
+
+	console_unlock();
 }
 
 /* called with console_lock held */
@@ -2801,10 +2805,12 @@  void fbcon_fb_unregistered(struct fb_info *info)
 {
 	int i, idx;
 
-	WARN_CONSOLE_UNLOCKED();
+	console_lock();
 
-	if (deferred_takeover)
+	if (deferred_takeover) {
+		console_unlock();
 		return;
+	}
 
 	idx = info->node;
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
@@ -2833,6 +2839,7 @@  void fbcon_fb_unregistered(struct fb_info *info)
 
 	if (!num_registered_fb)
 		do_unregister_con_driver(&fb_con);
+	console_unlock();
 }
 
 void fbcon_remap_all(struct fb_info *info)
@@ -2890,19 +2897,27 @@  static inline void fbcon_select_primary(struct fb_info *info)
 }
 #endif /* CONFIG_FRAMEBUFFER_DETECT_PRIMARY */
 
+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]");
+
 /* called with console_lock held */
 int fbcon_fb_registered(struct fb_info *info)
 {
 	int ret = 0, i, idx;
 
-	WARN_CONSOLE_UNLOCKED();
+	if (!lockless_register_fb)
+		console_lock();
+	else
+		atomic_inc(&ignore_console_lock_warning);
 
 	idx = info->node;
 	fbcon_select_primary(info);
 
 	if (deferred_takeover) {
 		pr_info("fbcon: Deferring console take-over\n");
-		return 0;
+		goto out;
 	}
 
 	if (info_idx == -1) {
@@ -2922,6 +2937,12 @@  int fbcon_fb_registered(struct fb_info *info)
 		}
 	}
 
+out:
+	if (!lockless_register_fb)
+		console_unlock();
+	else
+		atomic_dec(&ignore_console_lock_warning);
+
 	return ret;
 }
 
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0e68d9456bc2..bdd00d381bbc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1590,14 +1590,9 @@  static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	}
 }
 
-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;
+	int i;
 	struct fb_videomode mode;
 
 	if (fb_check_foreignness(fb_info))
@@ -1666,17 +1661,7 @@  static int do_register_framebuffer(struct fb_info *fb_info)
 	}
 #endif
 
-	if (!lockless_register_fb)
-		console_lock();
-	else
-		atomic_inc(&ignore_console_lock_warning);
-	ret = fbcon_fb_registered(fb_info);
-
-	if (!lockless_register_fb)
-		console_unlock();
-	else
-		atomic_dec(&ignore_console_lock_warning);
-	return ret;
+	return fbcon_fb_registered(fb_info);
 }
 
 static void unbind_console(struct fb_info *fb_info)
@@ -1686,9 +1671,7 @@  static void unbind_console(struct fb_info *fb_info)
 	if (WARN_ON(i < 0 || i >= FB_MAX || registered_fb[i] != fb_info))
 		return;
 
-	console_lock();
 	fbcon_fb_unbind(fb_info);
-	console_unlock();
 }
 
 static void unlink_framebuffer(struct fb_info *fb_info)
@@ -1731,9 +1714,7 @@  static void do_unregister_framebuffer(struct fb_info *fb_info)
 		fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
 	}
 #endif
-	console_lock();
 	fbcon_fb_unregistered(fb_info);
-	console_unlock();
 
 	/* this may free fb info */
 	put_fb_info(fb_info);