linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
@ 2018-06-28  9:03 ` Hans de Goede
  2018-06-28  9:03   ` [PATCH v5 1/3] printk: Export is_console_locked Hans de Goede
                     ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Hans de Goede @ 2018-06-28  9:03 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky
  Cc: Hans de Goede, dri-devel, linux-fbdev, Steven Rostedt, linux-kernel

Hi All,

Here is v5 of my patch-set, to delay fbcon taking over the console (and
binding to fbdev devices) until there actually is some text output to the
console. This is intended for use with the "quiet" cmdline option, in
combination with a bootloader which leaves the vendor's logo /
EFI bootgraphics put up by the firmware intact on the EFI framebuffer.

The end goal here is a boot where the firmware shows its boot graphics
and these stay in place for a couple of seconds until the GUI loads and
the GUI then smoothly takes over the framebuffer without any distruptions.

Bartlomiej, I believe that this patch-set is ready for merging now and
the first patch has Petr's ack, so can we get this merged now?

Also please create an inmutable (or topic) branch for this, so that the
drm people can merge it into drm-tip for additional testing.

Changelog:

Changes in v5:
-Improve commit message, explain why dummy_con.c is used to detect the
 first printout, instead of handling this entirely inside the fbcon code
 (no code changes, only updated the commit message)

Changes in v4:
-Keep the comments about which fbcon functions need locks in place

Changes in v3:
-Export is_console_locke() for use in modules (as fbcon may be built as a .ko)
-Use WARN_CONSOLE_UNLOCKED() in several places in the fbcon code to assert
 proper locking (requested by Daniel)
-Unregister the fbcon-dummycon-output-notifier on fbcon_exit() (req. by Daniel)
-Document the fbcon=nodefer commandline option (req. by Emil)

Changes in v2:
-Check the whole string when checking for erases in putcs, instead of just
 the first char
-Make dummycon_blank return 1, so that a redraw gets triggered and any text
 rendered while blanked gets output so that it can trigger a deferred
 takeover if one is pending

Regards,

Hans


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v5 1/3] printk: Export is_console_locked
  2018-06-28  9:03 ` [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover Hans de Goede
@ 2018-06-28  9:03   ` Hans de Goede
  2018-06-28  9:03   ` [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable Hans de Goede
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 28+ messages in thread
From: Hans de Goede @ 2018-06-28  9:03 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky
  Cc: Hans de Goede, dri-devel, linux-fbdev, Steven Rostedt, linux-kernel

This is a preparation patch for adding a number of WARN_CONSOLE_UNLOCKED()
calls to the fbcon code, which may be built as a module (event though
usually it is not).

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-New patch in v3 of this patchset
---
 kernel/printk/printk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..3f041e7cbfc9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2243,6 +2243,7 @@ int is_console_locked(void)
 {
 	return console_locked;
 }
+EXPORT_SYMBOL(is_console_locked);
 
 /*
  * Check if we have any console that is capable of printing while cpu is
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-06-28  9:03 ` [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover Hans de Goede
  2018-06-28  9:03   ` [PATCH v5 1/3] printk: Export is_console_locked Hans de Goede
@ 2018-06-28  9:03   ` Hans de Goede
  2018-07-11 14:46     ` Thomas Zimmermann
  2018-06-28  9:03   ` [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover Hans de Goede
  2018-06-28 13:50   ` [PATCH v5 0/3] " Bartlomiej Zolnierkiewicz
  3 siblings, 1 reply; 28+ messages in thread
From: Hans de Goede @ 2018-06-28  9:03 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky
  Cc: Hans de Goede, dri-devel, linux-fbdev, Steven Rostedt, linux-kernel

Replace comments about places where the console lock should be held with
calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-New patch in v3 of this patchset

Changes in v4:
-Keep the comments about which fbcon functions need locks in place
---
 drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c910e74d46ff..cd8d52a967aa 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
 	struct fb_info *oldinfo = NULL;
  	int found, err = 0;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	if (oldidx == newidx)
 		return 0;
 
@@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
 {
 	int i, new_idx = -1, ret = 0;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	if (!fbcon_has_console_bind)
 		return 0;
 
@@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
 {
 	int i, idx;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	idx = info->node;
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		if (con2fb_map[i] == idx)
@@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
 static void fbcon_remap_all(int idx)
 {
 	int i;
+
+	WARN_CONSOLE_UNLOCKED();
+
 	for (i = first_fb_vc; i <= last_fb_vc; i++)
 		set_con2fb_map(i, idx, 0);
 
@@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
 {
 	int ret = 0, i, idx;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	idx = info->node;
 	fbcon_select_primary(info);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover
  2018-06-28  9:03 ` [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover Hans de Goede
  2018-06-28  9:03   ` [PATCH v5 1/3] printk: Export is_console_locked Hans de Goede
  2018-06-28  9:03   ` [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable Hans de Goede
@ 2018-06-28  9:03   ` Hans de Goede
  2018-07-03 16:13     ` Sergey Senozhatsky
  2018-06-28 13:50   ` [PATCH v5 0/3] " Bartlomiej Zolnierkiewicz
  3 siblings, 1 reply; 28+ messages in thread
From: Hans de Goede @ 2018-06-28  9:03 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky
  Cc: Hans de Goede, dri-devel, linux-fbdev, Steven Rostedt, linux-kernel

Currently fbcon claims fbdevs as soon as they are registered and takes over
the console as soon as the first fbdev gets registered.

This behavior is undesirable in cases where a smooth graphical bootup is
desired, in such cases we typically want the contents of the framebuffer
(typically a vendor logo) to stay in place as is.

The current solution for this problem (on embedded systems) is to not
enable fbcon.

This commit adds a new FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER config option,
which when enabled defers fbcon taking over the console from the dummy
console until the first text is displayed on the console. Together with the
"quiet" kernel commandline option, this allows fbcon to still be used
together with a smooth graphical bootup, having it take over the console as
soon as e.g. an error message is logged.

Note the choice to detect the first console output in the dummycon driver,
rather then handling this entirely inside the fbcon code, was made after
2 failed attempts to handle this entirely inside the fbcon code. The fbcon
code is woven quite tightly into the console code, making this to only
feasible option.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Check the whole string when checking for erases in putcs, instead of just
 the first char
-Make dummycon_blank return 1, so that a redraw gets triggered and any text
 rendered while blanked gets output so that it can trigger a deferred
 takeover if one is pending

Changes in v3:
-Call WARN_CONSOLE_UNLOCKED() from fbcon_output_notifier()
-Unregister the notifier on fbcon_exit()
-Document the fbcon=nodefer commandline option in Documentation/fb/fbcon.txt

Changes in v5:
-Improve commit message, explain why dummy_con.c is used to detect the
 first printout, instead of handling this entirely inside the fbcon code
---
 Documentation/fb/fbcon.txt       |  7 ++++
 drivers/video/console/Kconfig    | 11 +++++
 drivers/video/console/dummycon.c | 67 +++++++++++++++++++++++++----
 drivers/video/fbdev/core/fbcon.c | 72 ++++++++++++++++++++++++++++++++
 include/linux/console.h          |  5 +++
 5 files changed, 154 insertions(+), 8 deletions(-)

diff --git a/Documentation/fb/fbcon.txt b/Documentation/fb/fbcon.txt
index 79c22d096bbc..d4d642e1ce9c 100644
--- a/Documentation/fb/fbcon.txt
+++ b/Documentation/fb/fbcon.txt
@@ -155,6 +155,13 @@ C. Boot options
 	used by text. By default, this area will be black. The 'color' value
 	is an integer number that depends on the framebuffer driver being used.
 
+6. fbcon=nodefer
+
+	If the kernel is compiled with deferred fbcon takeover support, normally
+	the framebuffer contents, left in place by the firmware/bootloader, will
+	be preserved until there actually is some text is output to the console.
+	This option causes fbcon to bind immediately to the fbdev device.
+
 C. Attaching, Detaching and Unloading
 
 Before going on how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index 4110ba7d7ca9..e91edef98633 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -150,6 +150,17 @@ config FRAMEBUFFER_CONSOLE_ROTATION
          such that other users of the framebuffer will remain normally
          oriented.
 
+config FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+	bool "Framebuffer Console Deferred Takeover"
+	depends on FRAMEBUFFER_CONSOLE=y && DUMMY_CONSOLE=y
+	help
+	  If enabled this defers the framebuffer console taking over the
+	  console from the dummy console until the first text is displayed on
+	  the console. This is useful in combination with the "quiet" kernel
+	  commandline option to keep the framebuffer contents initially put up
+	  by the firmware in place, rather then replacing the contents with a
+	  black screen as soon as fbcon loads.
+
 config STI_CONSOLE
         bool "STI text console"
 	depends on PARISC && HAS_IOMEM
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index f2eafe2ed980..45ad925ad5f8 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -26,6 +26,65 @@
 #define DUMMY_ROWS	CONFIG_DUMMY_CONSOLE_ROWS
 #endif
 
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+/* These are both protected by the console_lock */
+static RAW_NOTIFIER_HEAD(dummycon_output_nh);
+static bool dummycon_putc_called;
+
+void dummycon_register_output_notifier(struct notifier_block *nb)
+{
+	raw_notifier_chain_register(&dummycon_output_nh, nb);
+
+	if (dummycon_putc_called)
+		nb->notifier_call(nb, 0, NULL);
+}
+
+void dummycon_unregister_output_notifier(struct notifier_block *nb)
+{
+	raw_notifier_chain_unregister(&dummycon_output_nh, nb);
+}
+
+static void dummycon_putc(struct vc_data *vc, int c, int ypos, int xpos)
+{
+	dummycon_putc_called = true;
+	raw_notifier_call_chain(&dummycon_output_nh, 0, NULL);
+}
+
+static void dummycon_putcs(struct vc_data *vc, const unsigned short *s,
+			   int count, int ypos, int xpos)
+{
+	int i;
+
+	if (!dummycon_putc_called) {
+		/* Ignore erases */
+		for (i = 0 ; i < count; i++) {
+			if (s[i] != vc->vc_video_erase_char)
+				break;
+		}
+		if (i == count)
+			return;
+
+		dummycon_putc_called = true;
+	}
+
+	raw_notifier_call_chain(&dummycon_output_nh, 0, NULL);
+}
+
+static int dummycon_blank(struct vc_data *vc, int blank, int mode_switch)
+{
+	/* Redraw, so that we get putc(s) for output done while blanked */
+	return 1;
+}
+#else
+static void dummycon_putc(struct vc_data *vc, int c, int ypos, int xpos) { }
+static void dummycon_putcs(struct vc_data *vc, const unsigned short *s,
+			   int count, int ypos, int xpos) { }
+static int dummycon_blank(struct vc_data *vc, int blank, int mode_switch)
+{
+	return 0;
+}
+#endif
+
 static const char *dummycon_startup(void)
 {
     return "dummy device";
@@ -44,9 +103,6 @@ static void dummycon_init(struct vc_data *vc, int init)
 static void dummycon_deinit(struct vc_data *vc) { }
 static void dummycon_clear(struct vc_data *vc, int sy, int sx, int height,
 			   int width) { }
-static void dummycon_putc(struct vc_data *vc, int c, int ypos, int xpos) { }
-static void dummycon_putcs(struct vc_data *vc, const unsigned short *s,
-			   int count, int ypos, int xpos) { }
 static void dummycon_cursor(struct vc_data *vc, int mode) { }
 
 static bool dummycon_scroll(struct vc_data *vc, unsigned int top,
@@ -61,11 +117,6 @@ static int dummycon_switch(struct vc_data *vc)
 	return 0;
 }
 
-static int dummycon_blank(struct vc_data *vc, int blank, int mode_switch)
-{
-	return 0;
-}
-
 static int dummycon_font_set(struct vc_data *vc, struct console_font *font,
 			     unsigned int flags)
 {
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index cd8d52a967aa..5fb156bdcf4e 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -129,6 +129,12 @@ static inline void fbcon_map_override(void)
 }
 #endif /* CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY */
 
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+static bool deferred_takeover = true;
+#else
+#define deferred_takeover false
+#endif
+
 /* font data */
 static char fontname[40];
 
@@ -499,6 +505,12 @@ static int __init fb_console_setup(char *this_opt)
 				margin_color = simple_strtoul(options, &options, 0);
 			continue;
 		}
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+		if (!strcmp(options, "nodefer")) {
+			deferred_takeover = false;
+			continue;
+		}
+#endif
 	}
 	return 1;
 }
@@ -3100,6 +3112,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
 
 	WARN_CONSOLE_UNLOCKED();
 
+	if (deferred_takeover)
+		return 0;
+
 	idx = info->node;
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		if (con2fb_map[i] == idx)
@@ -3140,6 +3155,13 @@ static void fbcon_remap_all(int idx)
 
 	WARN_CONSOLE_UNLOCKED();
 
+	if (deferred_takeover) {
+		for (i = first_fb_vc; i <= last_fb_vc; i++)
+			con2fb_map_boot[i] = idx;
+		fbcon_map_override();
+		return;
+	}
+
 	for (i = first_fb_vc; i <= last_fb_vc; i++)
 		set_con2fb_map(i, idx, 0);
 
@@ -3191,6 +3213,11 @@ static int fbcon_fb_registered(struct fb_info *info)
 	idx = info->node;
 	fbcon_select_primary(info);
 
+	if (deferred_takeover) {
+		pr_info("fbcon: Deferring console take-over\n");
+		return 0;
+	}
+
 	if (info_idx == -1) {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] == idx) {
@@ -3566,8 +3593,46 @@ static int fbcon_init_device(void)
 	return 0;
 }
 
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+static struct notifier_block fbcon_output_nb;
+
+static int fbcon_output_notifier(struct notifier_block *nb,
+				 unsigned long action, void *data)
+{
+	int i;
+
+	WARN_CONSOLE_UNLOCKED();
+
+	pr_info("fbcon: Taking over console\n");
+
+	dummycon_unregister_output_notifier(&fbcon_output_nb);
+	deferred_takeover = false;
+	logo_shown = FBCON_LOGO_DONTSHOW;
+
+	for (i = 0; i < FB_MAX; i++) {
+		if (registered_fb[i])
+			fbcon_fb_registered(registered_fb[i]);
+	}
+
+	return NOTIFY_OK;
+}
+
+static void fbcon_register_output_notifier(void)
+{
+	fbcon_output_nb.notifier_call = fbcon_output_notifier;
+	dummycon_register_output_notifier(&fbcon_output_nb);
+}
+#else
+static inline void fbcon_register_output_notifier(void) {}
+#endif
+
 static void fbcon_start(void)
 {
+	if (deferred_takeover) {
+		fbcon_register_output_notifier();
+		return;
+	}
+
 	if (num_registered_fb) {
 		int i;
 
@@ -3594,6 +3659,13 @@ static void fbcon_exit(void)
 	if (fbcon_has_exited)
 		return;
 
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+	if (deferred_takeover) {
+		dummycon_unregister_output_notifier(&fbcon_output_nb);
+		deferred_takeover = false;
+	}
+#endif
+
 	kfree((void *)softback_buf);
 	softback_buf = 0UL;
 
diff --git a/include/linux/console.h b/include/linux/console.h
index dfd6b0e97855..f59f3dbca65c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -21,6 +21,7 @@ struct console_font_op;
 struct console_font;
 struct module;
 struct tty_struct;
+struct notifier_block;
 
 /*
  * this is what the terminal answers to a ESC-Z or csi0c query.
@@ -220,4 +221,8 @@ static inline bool vgacon_text_force(void) { return false; }
 
 extern void console_init(void);
 
+/* For deferred console takeover */
+void dummycon_register_output_notifier(struct notifier_block *nb);
+void dummycon_unregister_output_notifier(struct notifier_block *nb);
+
 #endif /* _LINUX_CONSOLE_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
  2018-06-28  9:03 ` [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover Hans de Goede
                     ` (2 preceding siblings ...)
  2018-06-28  9:03   ` [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover Hans de Goede
@ 2018-06-28 13:50   ` Bartlomiej Zolnierkiewicz
  2018-06-28 15:24     ` Hans de Goede
  2018-06-28 22:44     ` Gustavo Padovan
  3 siblings, 2 replies; 28+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2018-06-28 13:50 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Petr Mladek, Sergey Senozhatsky, dri-devel, linux-fbdev,
	Steven Rostedt, linux-kernel, Daniel Vetter

On Thursday, June 28, 2018 11:03:48 AM Hans de Goede wrote:
> Hi All,
> 
> Here is v5 of my patch-set, to delay fbcon taking over the console (and
> binding to fbdev devices) until there actually is some text output to the
> console. This is intended for use with the "quiet" cmdline option, in
> combination with a bootloader which leaves the vendor's logo /
> EFI bootgraphics put up by the firmware intact on the EFI framebuffer.
> 
> The end goal here is a boot where the firmware shows its boot graphics
> and these stay in place for a couple of seconds until the GUI loads and
> the GUI then smoothly takes over the framebuffer without any distruptions.
> 
> Bartlomiej, I believe that this patch-set is ready for merging now and
> the first patch has Petr's ack, so can we get this merged now?

Done, thanks for patches (also for reviews & acks).

> Also please create an inmutable (or topic) branch for this, so that the
> drm people can merge it into drm-tip for additional testing.

Daniel,

The following changes since commit 7daf201d7fe8334e2d2364d4e8ed3394ec9af819:

  Linux 4.18-rc2 (2018-06-24 20:54:29 +0800)

are available in the git repository at:

  https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-deferred-console-takeover

for you to fetch changes up to 83d83bebf40132e2d55ec58af666713cc76f9764:

  console/fbcon: Add support for deferred console takeover (2018-06-28 15:20:30 +0200)

----------------------------------------------------------------
Immutable branch between fbdev and drm for the v4.19 merge window
(contains the deferred console takeover feature)

----------------------------------------------------------------
Hans de Goede (3):
      printk: Export is_console_locked
      fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
      console/fbcon: Add support for deferred console takeover

 Documentation/fb/fbcon.txt       |  7 ++++
 drivers/video/console/Kconfig    | 11 ++++++
 drivers/video/console/dummycon.c | 67 ++++++++++++++++++++++++++++----
 drivers/video/fbdev/core/fbcon.c | 83 ++++++++++++++++++++++++++++++++++++++++
 include/linux/console.h          |  5 +++
 kernel/printk/printk.c           |  1 +
 6 files changed, 166 insertions(+), 8 deletions(-)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
  2018-06-28 13:50   ` [PATCH v5 0/3] " Bartlomiej Zolnierkiewicz
@ 2018-06-28 15:24     ` Hans de Goede
  2018-06-28 22:44     ` Gustavo Padovan
  1 sibling, 0 replies; 28+ messages in thread
From: Hans de Goede @ 2018-06-28 15:24 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Petr Mladek, Sergey Senozhatsky, dri-devel, linux-fbdev,
	Steven Rostedt, linux-kernel, Daniel Vetter

HI,

On 28-06-18 15:50, Bartlomiej Zolnierkiewicz wrote:
> On Thursday, June 28, 2018 11:03:48 AM Hans de Goede wrote:
>> Hi All,
>>
>> Here is v5 of my patch-set, to delay fbcon taking over the console (and
>> binding to fbdev devices) until there actually is some text output to the
>> console. This is intended for use with the "quiet" cmdline option, in
>> combination with a bootloader which leaves the vendor's logo /
>> EFI bootgraphics put up by the firmware intact on the EFI framebuffer.
>>
>> The end goal here is a boot where the firmware shows its boot graphics
>> and these stay in place for a couple of seconds until the GUI loads and
>> the GUI then smoothly takes over the framebuffer without any distruptions.
>>
>> Bartlomiej, I believe that this patch-set is ready for merging now and
>> the first patch has Petr's ack, so can we get this merged now?
> 
> Done, thanks for patches (also for reviews & acks).

You're welcome and thank you for merging these.

Regards,

Hans


> 
>> Also please create an inmutable (or topic) branch for this, so that the
>> drm people can merge it into drm-tip for additional testing.
> 
> Daniel,
> 
> The following changes since commit 7daf201d7fe8334e2d2364d4e8ed3394ec9af819:
> 
>    Linux 4.18-rc2 (2018-06-24 20:54:29 +0800)
> 
> are available in the git repository at:
> 
>    https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-deferred-console-takeover
> 
> for you to fetch changes up to 83d83bebf40132e2d55ec58af666713cc76f9764:
> 
>    console/fbcon: Add support for deferred console takeover (2018-06-28 15:20:30 +0200)
> 
> ----------------------------------------------------------------
> Immutable branch between fbdev and drm for the v4.19 merge window
> (contains the deferred console takeover feature)
> 
> ----------------------------------------------------------------
> Hans de Goede (3):
>        printk: Export is_console_locked
>        fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
>        console/fbcon: Add support for deferred console takeover
> 
>   Documentation/fb/fbcon.txt       |  7 ++++
>   drivers/video/console/Kconfig    | 11 ++++++
>   drivers/video/console/dummycon.c | 67 ++++++++++++++++++++++++++++----
>   drivers/video/fbdev/core/fbcon.c | 83 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/console.h          |  5 +++
>   kernel/printk/printk.c           |  1 +
>   6 files changed, 166 insertions(+), 8 deletions(-)
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
  2018-06-28 13:50   ` [PATCH v5 0/3] " Bartlomiej Zolnierkiewicz
  2018-06-28 15:24     ` Hans de Goede
@ 2018-06-28 22:44     ` Gustavo Padovan
  2018-06-29 10:08       ` Bartlomiej Zolnierkiewicz
  1 sibling, 1 reply; 28+ messages in thread
From: Gustavo Padovan @ 2018-06-28 22:44 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz, Hans de Goede
  Cc: Petr Mladek, linux-fbdev, Daniel Vetter, linux-kernel,
	Steven Rostedt, Sergey Senozhatsky, dri-devel

Hi Bartlomiej,

On Thu, 2018-06-28 at 15:50 +0200, Bartlomiej Zolnierkiewicz wrote:
> On Thursday, June 28, 2018 11:03:48 AM Hans de Goede wrote:
> > Hi All,
> > 
> > Here is v5 of my patch-set, to delay fbcon taking over the console
> > (and
> > binding to fbdev devices) until there actually is some text output
> > to the
> > console. This is intended for use with the "quiet" cmdline option,
> > in
> > combination with a bootloader which leaves the vendor's logo /
> > EFI bootgraphics put up by the firmware intact on the EFI
> > framebuffer.
> > 
> > The end goal here is a boot where the firmware shows its boot
> > graphics
> > and these stay in place for a couple of seconds until the GUI loads
> > and
> > the GUI then smoothly takes over the framebuffer without any
> > distruptions.
> > 
> > Bartlomiej, I believe that this patch-set is ready for merging now
> > and
> > the first patch has Petr's ack, so can we get this merged now?
> 
> Done, thanks for patches (also for reviews & acks).
> 
> > Also please create an inmutable (or topic) branch for this, so that
> > the
> > drm people can merge it into drm-tip for additional testing.
> 
> Daniel,
> 
> The following changes since commit
> 7daf201d7fe8334e2d2364d4e8ed3394ec9af819:
> 
>   Linux 4.18-rc2 (2018-06-24 20:54:29 +0800)
> 
> are available in the git repository at:
> 
>   https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-
> deferred-console-takeover
> 
> for you to fetch changes up to
> 83d83bebf40132e2d55ec58af666713cc76f9764:
> 
>   console/fbcon: Add support for deferred console takeover (2018-06-
> 28 15:20:30 +0200)
> 
> ----------------------------------------------------------------
> Immutable branch between fbdev and drm for the v4.19 merge window
> (contains the deferred console takeover feature)
> 
> ----------------------------------------------------------------
> Hans de Goede (3):
>       printk: Export is_console_locked
>       fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
>       console/fbcon: Add support for deferred console takeover
> 
>  Documentation/fb/fbcon.txt       |  7 ++++
>  drivers/video/console/Kconfig    | 11 ++++++
>  drivers/video/console/dummycon.c | 67 ++++++++++++++++++++++++++++
> ----
>  drivers/video/fbdev/core/fbcon.c | 83
> ++++++++++++++++++++++++++++++++++++++++
>  include/linux/console.h          |  5 +++
>  kernel/printk/printk.c           |  1 +
>  6 files changed, 166 insertions(+), 8 deletions(-)

Pulled into drm-misc-next, thanks

Gustavo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
  2018-06-28 22:44     ` Gustavo Padovan
@ 2018-06-29 10:08       ` Bartlomiej Zolnierkiewicz
  2018-06-29 12:51         ` Gustavo Padovan
  0 siblings, 1 reply; 28+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2018-06-29 10:08 UTC (permalink / raw)
  To: Gustavo Padovan
  Cc: Hans de Goede, Petr Mladek, linux-fbdev, Daniel Vetter,
	linux-kernel, Steven Rostedt, Sergey Senozhatsky, dri-devel


Hi Gustavo,

On Thursday, June 28, 2018 07:44:38 PM Gustavo Padovan wrote:
> Hi Bartlomiej,
> 
> On Thu, 2018-06-28 at 15:50 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday, June 28, 2018 11:03:48 AM Hans de Goede wrote:
> > > Hi All,
> > > 
> > > Here is v5 of my patch-set, to delay fbcon taking over the console
> > > (and
> > > binding to fbdev devices) until there actually is some text output
> > > to the
> > > console. This is intended for use with the "quiet" cmdline option,
> > > in
> > > combination with a bootloader which leaves the vendor's logo /
> > > EFI bootgraphics put up by the firmware intact on the EFI
> > > framebuffer.
> > > 
> > > The end goal here is a boot where the firmware shows its boot
> > > graphics
> > > and these stay in place for a couple of seconds until the GUI loads
> > > and
> > > the GUI then smoothly takes over the framebuffer without any
> > > distruptions.
> > > 
> > > Bartlomiej, I believe that this patch-set is ready for merging now
> > > and
> > > the first patch has Petr's ack, so can we get this merged now?
> > 
> > Done, thanks for patches (also for reviews & acks).
> > 
> > > Also please create an inmutable (or topic) branch for this, so that
> > > the
> > > drm people can merge it into drm-tip for additional testing.
> > 
> > Daniel,
> > 
> > The following changes since commit
> > 7daf201d7fe8334e2d2364d4e8ed3394ec9af819:
> > 
> >   Linux 4.18-rc2 (2018-06-24 20:54:29 +0800)
> > 
> > are available in the git repository at:
> > 
> >   https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-
> > deferred-console-takeover
> > 
> > for you to fetch changes up to
> > 83d83bebf40132e2d55ec58af666713cc76f9764:
> > 
> >   console/fbcon: Add support for deferred console takeover (2018-06-
> > 28 15:20:30 +0200)
> > 
> > ----------------------------------------------------------------
> > Immutable branch between fbdev and drm for the v4.19 merge window
> > (contains the deferred console takeover feature)
> > 
> > ----------------------------------------------------------------
> > Hans de Goede (3):
> >       printk: Export is_console_locked
> >       fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
> >       console/fbcon: Add support for deferred console takeover
> > 
> >  Documentation/fb/fbcon.txt       |  7 ++++
> >  drivers/video/console/Kconfig    | 11 ++++++
> >  drivers/video/console/dummycon.c | 67 ++++++++++++++++++++++++++++
> > ----
> >  drivers/video/fbdev/core/fbcon.c | 83
> > ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/console.h          |  5 +++
> >  kernel/printk/printk.c           |  1 +
> >  6 files changed, 166 insertions(+), 8 deletions(-)
> 
> Pulled into drm-misc-next, thanks

Please also apply fixup branch:

The following changes since commit 83d83bebf40132e2d55ec58af666713cc76f9764:

  console/fbcon: Add support for deferred console takeover (2018-06-28 15:20:30 +0200)

are available in the git repository at:

  https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-deferred-console-takeover-fixup

for you to fetch changes up to 334bb8972a131e604a741e9b284d8867190c723e:

  console: dummycon: export dummycon_[un]register_output_notifier (2018-06-29 11:46:19 +0200)

----------------------------------------------------------------
Immutable branch between fbdev and drm for the v4.19 merge window
(contains build fixup for the deferred console takeover feature)

----------------------------------------------------------------
Hans de Goede (1):
      console: dummycon: export dummycon_[un]register_output_notifier

 drivers/video/console/dummycon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index 45ad925..0254251 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -38,11 +38,13 @@ void dummycon_register_output_notifier(struct notifier_block *nb)
 	if (dummycon_putc_called)
 		nb->notifier_call(nb, 0, NULL);
 }
+EXPORT_SYMBOL_GPL(dummycon_register_output_notifier);
 
 void dummycon_unregister_output_notifier(struct notifier_block *nb)
 {
 	raw_notifier_chain_unregister(&dummycon_output_nh, nb);
 }
+EXPORT_SYMBOL_GPL(dummycon_unregister_output_notifier);
 
 static void dummycon_putc(struct vc_data *vc, int c, int ypos, int xpos)
 {


Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover
  2018-06-29 10:08       ` Bartlomiej Zolnierkiewicz
@ 2018-06-29 12:51         ` Gustavo Padovan
  0 siblings, 0 replies; 28+ messages in thread
From: Gustavo Padovan @ 2018-06-29 12:51 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Hans de Goede, Petr Mladek, linux-fbdev, Daniel Vetter,
	linux-kernel, Steven Rostedt, Sergey Senozhatsky, dri-devel

On Fri, 2018-06-29 at 12:08 +0200, Bartlomiej Zolnierkiewicz wrote:
> Hi Gustavo,
> 
> On Thursday, June 28, 2018 07:44:38 PM Gustavo Padovan wrote:
> > Hi Bartlomiej,
> > 
> > On Thu, 2018-06-28 at 15:50 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > On Thursday, June 28, 2018 11:03:48 AM Hans de Goede wrote:
> > > > Hi All,
> > > > 
> > > > Here is v5 of my patch-set, to delay fbcon taking over the
> > > > console
> > > > (and
> > > > binding to fbdev devices) until there actually is some text
> > > > output
> > > > to the
> > > > console. This is intended for use with the "quiet" cmdline
> > > > option,
> > > > in
> > > > combination with a bootloader which leaves the vendor's logo /
> > > > EFI bootgraphics put up by the firmware intact on the EFI
> > > > framebuffer.
> > > > 
> > > > The end goal here is a boot where the firmware shows its boot
> > > > graphics
> > > > and these stay in place for a couple of seconds until the GUI
> > > > loads
> > > > and
> > > > the GUI then smoothly takes over the framebuffer without any
> > > > distruptions.
> > > > 
> > > > Bartlomiej, I believe that this patch-set is ready for merging
> > > > now
> > > > and
> > > > the first patch has Petr's ack, so can we get this merged now?
> > > 
> > > Done, thanks for patches (also for reviews & acks).
> > > 
> > > > Also please create an inmutable (or topic) branch for this, so
> > > > that
> > > > the
> > > > drm people can merge it into drm-tip for additional testing.
> > > 
> > > Daniel,
> > > 
> > > The following changes since commit
> > > 7daf201d7fe8334e2d2364d4e8ed3394ec9af819:
> > > 
> > >   Linux 4.18-rc2 (2018-06-24 20:54:29 +0800)
> > > 
> > > are available in the git repository at:
> > > 
> > >   https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-
> > > deferred-console-takeover
> > > 
> > > for you to fetch changes up to
> > > 83d83bebf40132e2d55ec58af666713cc76f9764:
> > > 
> > >   console/fbcon: Add support for deferred console takeover (2018-
> > > 06-
> > > 28 15:20:30 +0200)
> > > 
> > > ----------------------------------------------------------------
> > > Immutable branch between fbdev and drm for the v4.19 merge window
> > > (contains the deferred console takeover feature)
> > > 
> > > ----------------------------------------------------------------
> > > Hans de Goede (3):
> > >       printk: Export is_console_locked
> > >       fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
> > >       console/fbcon: Add support for deferred console takeover
> > > 
> > >  Documentation/fb/fbcon.txt       |  7 ++++
> > >  drivers/video/console/Kconfig    | 11 ++++++
> > >  drivers/video/console/dummycon.c | 67
> > > ++++++++++++++++++++++++++++
> > > ----
> > >  drivers/video/fbdev/core/fbcon.c | 83
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/console.h          |  5 +++
> > >  kernel/printk/printk.c           |  1 +
> > >  6 files changed, 166 insertions(+), 8 deletions(-)
> > 
> > Pulled into drm-misc-next, thanks
> 
> Please also apply fixup branch:
> 
> The following changes since commit
> 83d83bebf40132e2d55ec58af666713cc76f9764:
> 
>   console/fbcon: Add support for deferred console takeover (2018-06-
> 28 15:20:30 +0200)
> 
> are available in the git repository at:
> 
>   https://github.com/bzolnier/linux.git tags/ib-fbdev-drm-v4.19-
> deferred-console-takeover-fixup
> 
> for you to fetch changes up to
> 334bb8972a131e604a741e9b284d8867190c723e:
> 
>   console: dummycon: export dummycon_[un]register_output_notifier
> (2018-06-29 11:46:19 +0200)
> 
> ----------------------------------------------------------------
> Immutable branch between fbdev and drm for the v4.19 merge window
> (contains build fixup for the deferred console takeover feature)
> 
> ----------------------------------------------------------------
> Hans de Goede (1):
>       console: dummycon: export dummycon_[un]register_output_notifier
> 
>  drivers/video/console/dummycon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/video/console/dummycon.c
> b/drivers/video/console/dummycon.c
> index 45ad925..0254251 100644
> --- a/drivers/video/console/dummycon.c
> +++ b/drivers/video/console/dummycon.c
> @@ -38,11 +38,13 @@ void dummycon_register_output_notifier(struct
> notifier_block *nb)
>  	if (dummycon_putc_called)
>  		nb->notifier_call(nb, 0, NULL);
>  }
> +EXPORT_SYMBOL_GPL(dummycon_register_output_notifier);
>  
>  void dummycon_unregister_output_notifier(struct notifier_block *nb)
>  {
>  	raw_notifier_chain_unregister(&dummycon_output_nh, nb);
>  }
> +EXPORT_SYMBOL_GPL(dummycon_unregister_output_notifier);
>  
>  static void dummycon_putc(struct vc_data *vc, int c, int ypos, int
> xpos)
>  {
> 

Pulled. Thanks.

Gustavo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover
  2018-06-28  9:03   ` [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover Hans de Goede
@ 2018-07-03 16:13     ` Sergey Senozhatsky
  2018-07-03 16:14       ` Steven Rostedt
  0 siblings, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2018-07-03 16:13 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky,
	dri-devel, linux-fbdev, Steven Rostedt, linux-kernel

On (06/28/18 11:03), Hans de Goede wrote:
[..]
>  
> +config FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> +	bool "Framebuffer Console Deferred Takeover"
> +	depends on FRAMEBUFFER_CONSOLE=y && DUMMY_CONSOLE=y

+	default n

> +	help
> +	  If enabled this defers the framebuffer console taking over the
> +	  console from the dummy console until the first text is displayed on
> +	  the console. This is useful in combination with the "quiet" kernel
> +	  commandline option to keep the framebuffer contents initially put up
> +	  by the firmware in place, rather then replacing the contents with a
> +	  black screen as soon as fbcon loads.

Can we please default this to 'n'?

	-ss

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover
  2018-07-03 16:13     ` Sergey Senozhatsky
@ 2018-07-03 16:14       ` Steven Rostedt
  2018-07-03 16:36         ` Sergey Senozhatsky
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Rostedt @ 2018-07-03 16:14 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans de Goede, Bartlomiej Zolnierkiewicz, Petr Mladek, dri-devel,
	linux-fbdev, linux-kernel

On Wed, 4 Jul 2018 01:13:27 +0900
Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> On (06/28/18 11:03), Hans de Goede wrote:
> [..]
> >  
> > +config FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > +	bool "Framebuffer Console Deferred Takeover"
> > +	depends on FRAMEBUFFER_CONSOLE=y && DUMMY_CONSOLE=y  
> 
> +	default n
> 
> > +	help
> > +	  If enabled this defers the framebuffer console taking over the
> > +	  console from the dummy console until the first text is displayed on
> > +	  the console. This is useful in combination with the "quiet" kernel
> > +	  commandline option to keep the framebuffer contents initially put up
> > +	  by the firmware in place, rather then replacing the contents with a
> > +	  black screen as soon as fbcon loads.  
> 
> Can we please default this to 'n'?
>

No explicit default means 'n'. You don't need to specify it.

-- Steve

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover
  2018-07-03 16:14       ` Steven Rostedt
@ 2018-07-03 16:36         ` Sergey Senozhatsky
  0 siblings, 0 replies; 28+ messages in thread
From: Sergey Senozhatsky @ 2018-07-03 16:36 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Sergey Senozhatsky, Hans de Goede, Bartlomiej Zolnierkiewicz,
	Petr Mladek, dri-devel, linux-fbdev, linux-kernel

On (07/03/18 12:14), Steven Rostedt wrote:
> > Can we please default this to 'n'?
> >
> 
> No explicit default means 'n'. You don't need to specify it.

Ah, OK. Thanks.

	-ss

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-06-28  9:03   ` [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable Hans de Goede
@ 2018-07-11 14:46     ` Thomas Zimmermann
  2018-07-11 14:52       ` Steven Rostedt
  2018-07-11 23:59       ` Sergey Senozhatsky
  0 siblings, 2 replies; 28+ messages in thread
From: Thomas Zimmermann @ 2018-07-11 14:46 UTC (permalink / raw)
  To: Hans de Goede, Bartlomiej Zolnierkiewicz, Petr Mladek,
	Sergey Senozhatsky
  Cc: linux-fbdev, Steven Rostedt, dri-devel, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 2570 bytes --]

Hi

Am 28.06.2018 um 11:03 schrieb Hans de Goede:
> Replace comments about places where the console lock should be held with
> calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.

Debugging fbcon sometimes requires to not take the console lock. This
patch breaks the debugging workaround provided by
'fb.lockless_register_fb'. The dmesg is now filled with warnings about
the missing lock.

Best regards
Thomas

> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> -New patch in v3 of this patchset
>
> Changes in v4:
> -Keep the comments about which fbcon functions need locks in place
> ---
>  drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index c910e74d46ff..cd8d52a967aa 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>  	struct fb_info *oldinfo = NULL;
>   	int found, err = 0;
>  
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	if (oldidx == newidx)
>  		return 0;
>  
> @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>  {
>  	int i, new_idx = -1, ret = 0;
>  
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	if (!fbcon_has_console_bind)
>  		return 0;
>  
> @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>  {
>  	int i, idx;
>  
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	idx = info->node;
>  	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>  		if (con2fb_map[i] == idx)
> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>  static void fbcon_remap_all(int idx)
>  {
>  	int i;
> +
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	for (i = first_fb_vc; i <= last_fb_vc; i++)
>  		set_con2fb_map(i, idx, 0);
>  
> @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
>  {
>  	int ret = 0, i, idx;
>  
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	idx = info->node;
>  	fbcon_select_primary(info);
>  

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 14:46     ` Thomas Zimmermann
@ 2018-07-11 14:52       ` Steven Rostedt
  2018-07-11 15:01         ` Hans de Goede
  2018-07-11 15:07         ` Thomas Zimmermann
  2018-07-11 23:59       ` Sergey Senozhatsky
  1 sibling, 2 replies; 28+ messages in thread
From: Steven Rostedt @ 2018-07-11 14:52 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: Hans de Goede, Bartlomiej Zolnierkiewicz, Petr Mladek,
	Sergey Senozhatsky, linux-fbdev, dri-devel, linux-kernel

On Wed, 11 Jul 2018 16:46:11 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
> > Replace comments about places where the console lock should be held with
> > calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.  
> 
> Debugging fbcon sometimes requires to not take the console lock. This
> patch breaks the debugging workaround provided by
> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
> the missing lock.
> 

What if you make lockless_register_fb visible to fbcon, and then we can
have a macro:

#define WARN_FB_CONSOLE_UNLOCKED() 			\
	do {						\
		if (unlikely(!lockless_register_fb))	\
			WARN_CONSOLE_UNLOCKED();	\
	} while (0)

And use that instead?

-- Steve


> Best regards
> Thomas
> 
> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Changes in v3:
> > -New patch in v3 of this patchset
> >
> > Changes in v4:
> > -Keep the comments about which fbcon functions need locks in place
> > ---
> >  drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index c910e74d46ff..cd8d52a967aa 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
> >  	struct fb_info *oldinfo = NULL;
> >   	int found, err = 0;
> >  
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> >  	if (oldidx == newidx)
> >  		return 0;
> >  
> > @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
> >  {
> >  	int i, new_idx = -1, ret = 0;
> >  
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> >  	if (!fbcon_has_console_bind)
> >  		return 0;
> >  
> > @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
> >  {
> >  	int i, idx;
> >  
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> >  	idx = info->node;
> >  	for (i = first_fb_vc; i <= last_fb_vc; i++) {
> >  		if (con2fb_map[i] == idx)
> > @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
> >  static void fbcon_remap_all(int idx)
> >  {
> >  	int i;
> > +
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> >  	for (i = first_fb_vc; i <= last_fb_vc; i++)
> >  		set_con2fb_map(i, idx, 0);
> >  
> > @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
> >  {
> >  	int ret = 0, i, idx;
> >  
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> >  	idx = info->node;
> >  	fbcon_select_primary(info);
> >    
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 14:52       ` Steven Rostedt
@ 2018-07-11 15:01         ` Hans de Goede
  2018-07-11 15:07           ` Daniel Vetter
  2018-07-11 15:07         ` Thomas Zimmermann
  1 sibling, 1 reply; 28+ messages in thread
From: Hans de Goede @ 2018-07-11 15:01 UTC (permalink / raw)
  To: Steven Rostedt, Thomas Zimmermann
  Cc: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky,
	linux-fbdev, dri-devel, linux-kernel

Hi,

On 11-07-18 16:52, Steven Rostedt wrote:
> On Wed, 11 Jul 2018 16:46:11 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Hi
>>
>> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
>>> Replace comments about places where the console lock should be held with
>>> calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
>>
>> Debugging fbcon sometimes requires to not take the console lock. This
>> patch breaks the debugging workaround provided by
>> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
>> the missing lock.
>>
> 
> What if you make lockless_register_fb visible to fbcon, and then we can
> have a macro:
> 
> #define WARN_FB_CONSOLE_UNLOCKED() 			\
> 	do {						\
> 		if (unlikely(!lockless_register_fb))	\
> 			WARN_CONSOLE_UNLOCKED();	\
> 	} while (0)
> 
> And use that instead?

Yes I'm already preparing a patch like that :)

Without the unlikely though, this is not used in hot paths.

Regards,

Hans



> 
> -- Steve
> 
> 
>> Best regards
>> Thomas
>>
>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v3:
>>> -New patch in v3 of this patchset
>>>
>>> Changes in v4:
>>> -Keep the comments about which fbcon functions need locks in place
>>> ---
>>>   drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>>> index c910e74d46ff..cd8d52a967aa 100644
>>> --- a/drivers/video/fbdev/core/fbcon.c
>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>>>   	struct fb_info *oldinfo = NULL;
>>>    	int found, err = 0;
>>>   
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>   	if (oldidx == newidx)
>>>   		return 0;
>>>   
>>> @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>   {
>>>   	int i, new_idx = -1, ret = 0;
>>>   
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>   	if (!fbcon_has_console_bind)
>>>   		return 0;
>>>   
>>> @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>   {
>>>   	int i, idx;
>>>   
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>   	idx = info->node;
>>>   	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>   		if (con2fb_map[i] == idx)
>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>   static void fbcon_remap_all(int idx)
>>>   {
>>>   	int i;
>>> +
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>   	for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>   		set_con2fb_map(i, idx, 0);
>>>   
>>> @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
>>>   {
>>>   	int ret = 0, i, idx;
>>>   
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>   	idx = info->node;
>>>   	fbcon_select_primary(info);
>>>     
>>
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 14:52       ` Steven Rostedt
  2018-07-11 15:01         ` Hans de Goede
@ 2018-07-11 15:07         ` Thomas Zimmermann
  2018-07-11 15:14           ` Hans de Goede
  1 sibling, 1 reply; 28+ messages in thread
From: Thomas Zimmermann @ 2018-07-11 15:07 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Hans de Goede, Bartlomiej Zolnierkiewicz, Petr Mladek,
	Sergey Senozhatsky, linux-fbdev, dri-devel, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 3047 bytes --]

Hi

Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
> 
> What if you make lockless_register_fb visible to fbcon, and then we can
> have a macro:

There are more of these macro invocations under drivers/tty/vt, which
also mess up the log during debugging.

WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
...' construct [1]. I thought about turning this into a config option.

Best regards
Thomas

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203

> 
> #define WARN_FB_CONSOLE_UNLOCKED() 			\
> 	do {						\
> 		if (unlikely(!lockless_register_fb))	\
> 			WARN_CONSOLE_UNLOCKED();	\
> 	} while (0)
> 
> And use that instead?
> 
> -- Steve
> 
> 
>> Best regards
>> Thomas
>>
>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v3:
>>> -New patch in v3 of this patchset
>>>
>>> Changes in v4:
>>> -Keep the comments about which fbcon functions need locks in place
>>> ---
>>>  drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>>> index c910e74d46ff..cd8d52a967aa 100644
>>> --- a/drivers/video/fbdev/core/fbcon.c
>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>>>  	struct fb_info *oldinfo = NULL;
>>>   	int found, err = 0;
>>>  
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>  	if (oldidx == newidx)
>>>  		return 0;
>>>  
>>> @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>  {
>>>  	int i, new_idx = -1, ret = 0;
>>>  
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>  	if (!fbcon_has_console_bind)
>>>  		return 0;
>>>  
>>> @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>  {
>>>  	int i, idx;
>>>  
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>  	idx = info->node;
>>>  	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>  		if (con2fb_map[i] == idx)
>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>  static void fbcon_remap_all(int idx)
>>>  {
>>>  	int i;
>>> +
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>  	for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>  		set_con2fb_map(i, idx, 0);
>>>  
>>> @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
>>>  {
>>>  	int ret = 0, i, idx;
>>>  
>>> +	WARN_CONSOLE_UNLOCKED();
>>> +
>>>  	idx = info->node;
>>>  	fbcon_select_primary(info);
>>>    
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:01         ` Hans de Goede
@ 2018-07-11 15:07           ` Daniel Vetter
  0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2018-07-11 15:07 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Steven Rostedt, Thomas Zimmermann, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, Jul 11, 2018 at 5:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11-07-18 16:52, Steven Rostedt wrote:
>>
>> On Wed, 11 Jul 2018 16:46:11 +0200
>> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>>> Hi
>>>
>>> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
>>>>
>>>> Replace comments about places where the console lock should be held with
>>>> calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
>>>
>>>
>>> Debugging fbcon sometimes requires to not take the console lock. This
>>> patch breaks the debugging workaround provided by
>>> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
>>> the missing lock.
>>>
>>
>> What if you make lockless_register_fb visible to fbcon, and then we can
>> have a macro:
>>
>> #define WARN_FB_CONSOLE_UNLOCKED()                      \
>>         do {                                            \
>>                 if (unlikely(!lockless_register_fb))    \
>>                         WARN_CONSOLE_UNLOCKED();        \
>>         } while (0)
>>
>> And use that instead?
>
>
> Yes I'm already preparing a patch like that :)
>
> Without the unlikely though, this is not used in hot paths.

Note that you don't even need an EXPORT_SYMBOL since this is all in
the same module.
-Daniel

> Regards,
>
> Hans
>
>
>
>
>>
>> -- Steve
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v3:
>>>> -New patch in v3 of this patchset
>>>>
>>>> Changes in v4:
>>>> -Keep the comments about which fbcon functions need locks in place
>>>> ---
>>>>   drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>> b/drivers/video/fbdev/core/fbcon.c
>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int
>>>> user)
>>>>         struct fb_info *oldinfo = NULL;
>>>>         int found, err = 0;
>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>         if (oldidx == newidx)
>>>>                 return 0;
>>>>   @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>   {
>>>>         int i, new_idx = -1, ret = 0;
>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>         if (!fbcon_has_console_bind)
>>>>                 return 0;
>>>>   @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info
>>>> *info)
>>>>   {
>>>>         int i, idx;
>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>         idx = info->node;
>>>>         for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>                 if (con2fb_map[i] == idx)
>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info
>>>> *info)
>>>>   static void fbcon_remap_all(int idx)
>>>>   {
>>>>         int i;
>>>> +
>>>> +       WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>         for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>                 set_con2fb_map(i, idx, 0);
>>>>   @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info
>>>> *info)
>>>>   {
>>>>         int ret = 0, i, idx;
>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>         idx = info->node;
>>>>         fbcon_select_primary(info);
>>>>
>>>
>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:07         ` Thomas Zimmermann
@ 2018-07-11 15:14           ` Hans de Goede
  2018-07-11 15:28             ` Daniel Vetter
  0 siblings, 1 reply; 28+ messages in thread
From: Hans de Goede @ 2018-07-11 15:14 UTC (permalink / raw)
  To: Thomas Zimmermann, Steven Rostedt
  Cc: Bartlomiej Zolnierkiewicz, Petr Mladek, Sergey Senozhatsky,
	linux-fbdev, dri-devel, linux-kernel

Hi,

On 11-07-18 17:07, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>
>> What if you make lockless_register_fb visible to fbcon, and then we can
>> have a macro:
> 
> There are more of these macro invocations under drivers/tty/vt, which
> also mess up the log during debugging.

Hmm, so this option is already broken (in a way) then, my first reaction
to your mail was that we should just remove that option. But that seemed
a bit harsh to me so I've been working on a fix for the last 10 minutes
or so.

But if it is already broken I'm tempted to just remove the option and
be done with it.  We really need less cruft in the fbdev/fbcon code not
more.

> WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
> ...' construct [1]. I thought about turning this into a config option.

Ah I noticed the #if but I did not notice that it was a "#if 1".

If you want to fix lockless_register_fb it really should be replaced
with a runtime check, not a Kconfig option. This would require having
some lockless variable in the console code itself, which then gets
set by the fbdev code during init based on its lockless_register_fb
setting.

But as said I think we should seriously consider just removing
lockless_register_fb.

Regards,

Hans






> 
> Best regards
> Thomas
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203
> 
>>
>> #define WARN_FB_CONSOLE_UNLOCKED() 			\
>> 	do {						\
>> 		if (unlikely(!lockless_register_fb))	\
>> 			WARN_CONSOLE_UNLOCKED();	\
>> 	} while (0)
>>
>> And use that instead?
>>
>> -- Steve
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v3:
>>>> -New patch in v3 of this patchset
>>>>
>>>> Changes in v4:
>>>> -Keep the comments about which fbcon functions need locks in place
>>>> ---
>>>>   drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int user)
>>>>   	struct fb_info *oldinfo = NULL;
>>>>    	int found, err = 0;
>>>>   
>>>> +	WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>   	if (oldidx == newidx)
>>>>   		return 0;
>>>>   
>>>> @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>   {
>>>>   	int i, new_idx = -1, ret = 0;
>>>>   
>>>> +	WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>   	if (!fbcon_has_console_bind)
>>>>   		return 0;
>>>>   
>>>> @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>>   {
>>>>   	int i, idx;
>>>>   
>>>> +	WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>   	idx = info->node;
>>>>   	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>   		if (con2fb_map[i] == idx)
>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info *info)
>>>>   static void fbcon_remap_all(int idx)
>>>>   {
>>>>   	int i;
>>>> +
>>>> +	WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>   	for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>   		set_con2fb_map(i, idx, 0);
>>>>   
>>>> @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info *info)
>>>>   {
>>>>   	int ret = 0, i, idx;
>>>>   
>>>> +	WARN_CONSOLE_UNLOCKED();
>>>> +
>>>>   	idx = info->node;
>>>>   	fbcon_select_primary(info);
>>>>     
>>>
>>
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:14           ` Hans de Goede
@ 2018-07-11 15:28             ` Daniel Vetter
  2018-07-11 15:35               ` Hans de Goede
  0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2018-07-11 15:28 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thomas Zimmermann, Steven Rostedt, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>
>>>
>>> What if you make lockless_register_fb visible to fbcon, and then we can
>>> have a macro:
>>
>>
>> There are more of these macro invocations under drivers/tty/vt, which
>> also mess up the log during debugging.
>
>
> Hmm, so this option is already broken (in a way) then, my first reaction
> to your mail was that we should just remove that option. But that seemed
> a bit harsh to me so I've been working on a fix for the last 10 minutes
> or so.
>
> But if it is already broken I'm tempted to just remove the option and
> be done with it.  We really need less cruft in the fbdev/fbcon code not
> more.

Please don't remove it, it makes debugging kms driver issues on
initial modeset (which is usually run from framebuffer_register, while
hodling the console_lock) impossible.
-Daniel

>> WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
>> ...' construct [1]. I thought about turning this into a config option.
>
>
> Ah I noticed the #if but I did not notice that it was a "#if 1".
>
> If you want to fix lockless_register_fb it really should be replaced
> with a runtime check, not a Kconfig option. This would require having
> some lockless variable in the console code itself, which then gets
> set by the fbdev code during init based on its lockless_register_fb
> setting.
>
> But as said I think we should seriously consider just removing
> lockless_register_fb.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>>
>> Best regards
>> Thomas
>>
>> [1]
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203
>>
>>>
>>> #define WARN_FB_CONSOLE_UNLOCKED()                      \
>>>         do {                                            \
>>>                 if (unlikely(!lockless_register_fb))    \
>>>                         WARN_CONSOLE_UNLOCKED();        \
>>>         } while (0)
>>>
>>> And use that instead?
>>>
>>> -- Steve
>>>
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Changes in v3:
>>>>> -New patch in v3 of this patchset
>>>>>
>>>>> Changes in v4:
>>>>> -Keep the comments about which fbcon functions need locks in place
>>>>> ---
>>>>>   drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>>> b/drivers/video/fbdev/core/fbcon.c
>>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int
>>>>> user)
>>>>>         struct fb_info *oldinfo = NULL;
>>>>>         int found, err = 0;
>>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>>         if (oldidx == newidx)
>>>>>                 return 0;
>>>>>   @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>>   {
>>>>>         int i, new_idx = -1, ret = 0;
>>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>>         if (!fbcon_has_console_bind)
>>>>>                 return 0;
>>>>>   @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>> *info)
>>>>>   {
>>>>>         int i, idx;
>>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>>         idx = info->node;
>>>>>         for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>>                 if (con2fb_map[i] == idx)
>>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>> *info)
>>>>>   static void fbcon_remap_all(int idx)
>>>>>   {
>>>>>         int i;
>>>>> +
>>>>> +       WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>>         for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>>                 set_con2fb_map(i, idx, 0);
>>>>>   @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info
>>>>> *info)
>>>>>   {
>>>>>         int ret = 0, i, idx;
>>>>>   +     WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>>         idx = info->node;
>>>>>         fbcon_select_primary(info);
>>>>>
>>>>
>>>>
>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:28             ` Daniel Vetter
@ 2018-07-11 15:35               ` Hans de Goede
  2018-07-11 15:41                 ` Steven Rostedt
  2018-07-11 15:42                 ` Daniel Vetter
  0 siblings, 2 replies; 28+ messages in thread
From: Hans de Goede @ 2018-07-11 15:35 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Thomas Zimmermann, Steven Rostedt, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

Hi,

On 11-07-18 17:28, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>>
>>> Hi
>>>
>>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>>
>>>>
>>>> What if you make lockless_register_fb visible to fbcon, and then we can
>>>> have a macro:
>>>
>>>
>>> There are more of these macro invocations under drivers/tty/vt, which
>>> also mess up the log during debugging.
>>
>>
>> Hmm, so this option is already broken (in a way) then, my first reaction
>> to your mail was that we should just remove that option. But that seemed
>> a bit harsh to me so I've been working on a fix for the last 10 minutes
>> or so.
>>
>> But if it is already broken I'm tempted to just remove the option and
>> be done with it.  We really need less cruft in the fbdev/fbcon code not
>> more.
> 
> Please don't remove it, it makes debugging kms driver issues on
> initial modeset (which is usually run from framebuffer_register, while
> hodling the console_lock) impossible.

OK, so if we don't remove it, we should probably make it so that it
can be used without triggering any WARN_ONs, which would require changing
the existing WARN_CONSOLE_UNLOCKED() so that the calls from drivers/tty/vt/vt.c
also do not trigger it ?

I guess one can just ignore the oopses when debugging, but debugging surely
would be easier if there are just no oopses ?

Regards,

Hans




> -Daniel
> 
>>> WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
>>> ...' construct [1]. I thought about turning this into a config option.
>>
>>
>> Ah I noticed the #if but I did not notice that it was a "#if 1".
>>
>> If you want to fix lockless_register_fb it really should be replaced
>> with a runtime check, not a Kconfig option. This would require having
>> some lockless variable in the console code itself, which then gets
>> set by the fbdev code during init based on its lockless_register_fb
>> setting.
>>
>> But as said I think we should seriously consider just removing
>> lockless_register_fb.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1]
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203
>>>
>>>>
>>>> #define WARN_FB_CONSOLE_UNLOCKED()                      \
>>>>          do {                                            \
>>>>                  if (unlikely(!lockless_register_fb))    \
>>>>                          WARN_CONSOLE_UNLOCKED();        \
>>>>          } while (0)
>>>>
>>>> And use that instead?
>>>>
>>>> -- Steve
>>>>
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>> -New patch in v3 of this patchset
>>>>>>
>>>>>> Changes in v4:
>>>>>> -Keep the comments about which fbcon functions need locks in place
>>>>>> ---
>>>>>>    drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>>>    1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>>>> b/drivers/video/fbdev/core/fbcon.c
>>>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int
>>>>>> user)
>>>>>>          struct fb_info *oldinfo = NULL;
>>>>>>          int found, err = 0;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          if (oldidx == newidx)
>>>>>>                  return 0;
>>>>>>    @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>>>    {
>>>>>>          int i, new_idx = -1, ret = 0;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          if (!fbcon_has_console_bind)
>>>>>>                  return 0;
>>>>>>    @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>>> *info)
>>>>>>    {
>>>>>>          int i, idx;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          idx = info->node;
>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>>>                  if (con2fb_map[i] == idx)
>>>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>>> *info)
>>>>>>    static void fbcon_remap_all(int idx)
>>>>>>    {
>>>>>>          int i;
>>>>>> +
>>>>>> +       WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>>>                  set_con2fb_map(i, idx, 0);
>>>>>>    @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info
>>>>>> *info)
>>>>>>    {
>>>>>>          int ret = 0, i, idx;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          idx = info->node;
>>>>>>          fbcon_select_primary(info);
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:35               ` Hans de Goede
@ 2018-07-11 15:41                 ` Steven Rostedt
  2018-07-11 15:42                 ` Daniel Vetter
  1 sibling, 0 replies; 28+ messages in thread
From: Steven Rostedt @ 2018-07-11 15:41 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Daniel Vetter, Thomas Zimmermann, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, 11 Jul 2018 17:35:10 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> OK, so if we don't remove it, we should probably make it so that it
> can be used without triggering any WARN_ONs, which would require changing
> the existing WARN_CONSOLE_UNLOCKED() so that the calls from drivers/tty/vt/vt.c
> also do not trigger it ?
> 
> I guess one can just ignore the oopses when debugging, but debugging surely
> would be easier if there are just no oopses ?

What about adding this patch (untested, not even compiled), and then
set it from the fb module.

-- Steve

diff --git a/include/linux/console.h b/include/linux/console.h
index dfd6b0e97855..11cd4956a57f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -200,8 +200,10 @@ void vcs_make_sysfs(int index);
 void vcs_remove_sysfs(int index);
 
 /* Some debug stub to catch some of the obvious races in the VT code */
+extern bool ignore_console_lock_warning;
 #if 1
-#define WARN_CONSOLE_UNLOCKED()	WARN_ON(!is_console_locked() && !oops_in_progress)
+#define WARN_CONSOLE_UNLOCKED()						\
+	WARN_ON(!ignore_console_lock_warning && !is_console_locked() && !oops_in_progress)
 #else
 #define WARN_CONSOLE_UNLOCKED()
 #endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..fa15d7ddf0c4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -66,6 +66,9 @@ int console_printk[4] = {
 	CONSOLE_LOGLEVEL_DEFAULT,	/* default_console_loglevel */
 };
 
+bool ignore_console_lock_warning;
+EXPORT_SYMBOL(ignore_console_lock_warning);
+
 /*
  * Low level drivers may need that to know if they can schedule in
  * their unblank() callback or not. So let's export it.

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:35               ` Hans de Goede
  2018-07-11 15:41                 ` Steven Rostedt
@ 2018-07-11 15:42                 ` Daniel Vetter
  2018-07-11 17:35                   ` Hans de Goede
  1 sibling, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2018-07-11 15:42 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thomas Zimmermann, Steven Rostedt, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11-07-18 17:28, Daniel Vetter wrote:
>>
>> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>>>
>>>>
>>>> Hi
>>>>
>>>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>>>
>>>>>
>>>>>
>>>>> What if you make lockless_register_fb visible to fbcon, and then we can
>>>>> have a macro:
>>>>
>>>>
>>>>
>>>> There are more of these macro invocations under drivers/tty/vt, which
>>>> also mess up the log during debugging.
>>>
>>>
>>>
>>> Hmm, so this option is already broken (in a way) then, my first reaction
>>> to your mail was that we should just remove that option. But that seemed
>>> a bit harsh to me so I've been working on a fix for the last 10 minutes
>>> or so.
>>>
>>> But if it is already broken I'm tempted to just remove the option and
>>> be done with it.  We really need less cruft in the fbdev/fbcon code not
>>> more.
>>
>>
>> Please don't remove it, it makes debugging kms driver issues on
>> initial modeset (which is usually run from framebuffer_register, while
>> hodling the console_lock) impossible.
>
>
> OK, so if we don't remove it, we should probably make it so that it
> can be used without triggering any WARN_ONs, which would require changing
> the existing WARN_CONSOLE_UNLOCKED() so that the calls from
> drivers/tty/vt/vt.c
> also do not trigger it ?
>
> I guess one can just ignore the oopses when debugging, but debugging surely
> would be easier if there are just no oopses ?

I'd say let's only bother with the ones in fbcon.c. Avoids the trouble
with having to expose the fb module option to vt.c somehow. The ones
in vt.c are as old as the git history (from a quick check at least),
and in my debugging they never have been annoying (or I somehow didn't
ever hit them, not idea).

Also note that none if this matters long-term because I have a really
fancy plan to fix up this entire console_locking mess, which will also
remove the need for the lockless_fb_register debug hack.
-Daniel

>
> Regards,
>
> Hans
>
>
>
>
>
>> -Daniel
>>
>>>> WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
>>>> ...' construct [1]. I thought about turning this into a config option.
>>>
>>>
>>>
>>> Ah I noticed the #if but I did not notice that it was a "#if 1".
>>>
>>> If you want to fix lockless_register_fb it really should be replaced
>>> with a runtime check, not a Kconfig option. This would require having
>>> some lockless variable in the console code itself, which then gets
>>> set by the fbdev code during init based on its lockless_register_fb
>>> setting.
>>>
>>> But as said I think we should seriously consider just removing
>>> lockless_register_fb.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> [1]
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203
>>>>
>>>>>
>>>>> #define WARN_FB_CONSOLE_UNLOCKED()                      \
>>>>>          do {                                            \
>>>>>                  if (unlikely(!lockless_register_fb))    \
>>>>>                          WARN_CONSOLE_UNLOCKED();        \
>>>>>          } while (0)
>>>>>
>>>>> And use that instead?
>>>>>
>>>>> -- Steve
>>>>>
>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>> Changes in v3:
>>>>>>> -New patch in v3 of this patchset
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>> -Keep the comments about which fbcon functions need locks in place
>>>>>>> ---
>>>>>>>    drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>>>>    1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>>>>> b/drivers/video/fbdev/core/fbcon.c
>>>>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx,
>>>>>>> int
>>>>>>> user)
>>>>>>>          struct fb_info *oldinfo = NULL;
>>>>>>>          int found, err = 0;
>>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>>> +
>>>>>>>          if (oldidx == newidx)
>>>>>>>                  return 0;
>>>>>>>    @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>>>>    {
>>>>>>>          int i, new_idx = -1, ret = 0;
>>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>>> +
>>>>>>>          if (!fbcon_has_console_bind)
>>>>>>>                  return 0;
>>>>>>>    @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct
>>>>>>> fb_info
>>>>>>> *info)
>>>>>>>    {
>>>>>>>          int i, idx;
>>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>>> +
>>>>>>>          idx = info->node;
>>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>>>>                  if (con2fb_map[i] == idx)
>>>>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>>>> *info)
>>>>>>>    static void fbcon_remap_all(int idx)
>>>>>>>    {
>>>>>>>          int i;
>>>>>>> +
>>>>>>> +       WARN_CONSOLE_UNLOCKED();
>>>>>>> +
>>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>>>>                  set_con2fb_map(i, idx, 0);
>>>>>>>    @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct
>>>>>>> fb_info
>>>>>>> *info)
>>>>>>>    {
>>>>>>>          int ret = 0, i, idx;
>>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>>> +
>>>>>>>          idx = info->node;
>>>>>>>          fbcon_select_primary(info);
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>>
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 15:42                 ` Daniel Vetter
@ 2018-07-11 17:35                   ` Hans de Goede
  2018-07-11 17:56                     ` Daniel Vetter
  0 siblings, 1 reply; 28+ messages in thread
From: Hans de Goede @ 2018-07-11 17:35 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Thomas Zimmermann, Steven Rostedt, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

Hi,

On 11-07-18 17:42, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11-07-18 17:28, Daniel Vetter wrote:
>>>
>>> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>>>>
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>>>>
>>>>>>
>>>>>>
>>>>>> What if you make lockless_register_fb visible to fbcon, and then we can
>>>>>> have a macro:
>>>>>
>>>>>
>>>>>
>>>>> There are more of these macro invocations under drivers/tty/vt, which
>>>>> also mess up the log during debugging.
>>>>
>>>>
>>>>
>>>> Hmm, so this option is already broken (in a way) then, my first reaction
>>>> to your mail was that we should just remove that option. But that seemed
>>>> a bit harsh to me so I've been working on a fix for the last 10 minutes
>>>> or so.
>>>>
>>>> But if it is already broken I'm tempted to just remove the option and
>>>> be done with it.  We really need less cruft in the fbdev/fbcon code not
>>>> more.
>>>
>>>
>>> Please don't remove it, it makes debugging kms driver issues on
>>> initial modeset (which is usually run from framebuffer_register, while
>>> hodling the console_lock) impossible.
>>
>>
>> OK, so if we don't remove it, we should probably make it so that it
>> can be used without triggering any WARN_ONs, which would require changing
>> the existing WARN_CONSOLE_UNLOCKED() so that the calls from
>> drivers/tty/vt/vt.c
>> also do not trigger it ?
>>
>> I guess one can just ignore the oopses when debugging, but debugging surely
>> would be easier if there are just no oopses ?
> 
> I'd say let's only bother with the ones in fbcon.c. Avoids the trouble
> with having to expose the fb module option to vt.c somehow.

The plan was actually do the things the other way around, add a flag to
vt.c which when set disables the WARN_ON calls and then have fbdev[.ko]
set that when the fb.lockless_fb_register option is set.

> The ones
> in vt.c are as old as the git history (from a quick check at least),
> and in my debugging they never have been annoying (or I somehow didn't
> ever hit them, not idea).

There is a #if 1 #define #else #define empty around the WARN_CONSOLE_UNLOCKED()
call in include/linux/console.h I've the feeling that is there as a hack
to be able to quickly disable the WARN_ONs when debugging.

Have you seen Steven's suggestion which he send about the same time
as your mail I'm replying to here ? I personally think that doing
something like that makes sense (for as long as we have the need
for the lockless_fb_register debug hack).

Note I've 2 patches ready to go to only fix this in fbcon.c, but I
think a more thorough fix makes sense.

Regards,

Hans


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 17:35                   ` Hans de Goede
@ 2018-07-11 17:56                     ` Daniel Vetter
  2018-07-11 19:19                       ` Steven Rostedt
  0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2018-07-11 17:56 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Thomas Zimmermann, Steven Rostedt, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, Jul 11, 2018 at 7:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 11-07-18 17:42, Daniel Vetter wrote:
>>
>> On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 11-07-18 17:28, Daniel Vetter wrote:
>>>>
>>>>
>>>> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> What if you make lockless_register_fb visible to fbcon, and then we
>>>>>>> can
>>>>>>> have a macro:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> There are more of these macro invocations under drivers/tty/vt, which
>>>>>> also mess up the log during debugging.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Hmm, so this option is already broken (in a way) then, my first
>>>>> reaction
>>>>> to your mail was that we should just remove that option. But that
>>>>> seemed
>>>>> a bit harsh to me so I've been working on a fix for the last 10 minutes
>>>>> or so.
>>>>>
>>>>> But if it is already broken I'm tempted to just remove the option and
>>>>> be done with it.  We really need less cruft in the fbdev/fbcon code not
>>>>> more.
>>>>
>>>>
>>>>
>>>> Please don't remove it, it makes debugging kms driver issues on
>>>> initial modeset (which is usually run from framebuffer_register, while
>>>> hodling the console_lock) impossible.
>>>
>>>
>>>
>>> OK, so if we don't remove it, we should probably make it so that it
>>> can be used without triggering any WARN_ONs, which would require changing
>>> the existing WARN_CONSOLE_UNLOCKED() so that the calls from
>>> drivers/tty/vt/vt.c
>>> also do not trigger it ?
>>>
>>> I guess one can just ignore the oopses when debugging, but debugging
>>> surely
>>> would be easier if there are just no oopses ?
>>
>>
>> I'd say let's only bother with the ones in fbcon.c. Avoids the trouble
>> with having to expose the fb module option to vt.c somehow.
>
>
> The plan was actually do the things the other way around, add a flag to
> vt.c which when set disables the WARN_ON calls and then have fbdev[.ko]
> set that when the fb.lockless_fb_register option is set.
>
>> The ones
>> in vt.c are as old as the git history (from a quick check at least),
>> and in my debugging they never have been annoying (or I somehow didn't
>> ever hit them, not idea).
>
>
> There is a #if 1 #define #else #define empty around the
> WARN_CONSOLE_UNLOCKED()
> call in include/linux/console.h I've the feeling that is there as a hack
> to be able to quickly disable the WARN_ONs when debugging.
>
> Have you seen Steven's suggestion which he send about the same time
> as your mail I'm replying to here ? I personally think that doing
> something like that makes sense (for as long as we have the need
> for the lockless_fb_register debug hack).
>
> Note I've 2 patches ready to go to only fix this in fbcon.c, but I
> think a more thorough fix makes sense.

Yeah Steven's suggestion looks reasonable to fix this all for good.
The #if 1 predates git history, so no idea why it was added or by whom
:-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 17:56                     ` Daniel Vetter
@ 2018-07-11 19:19                       ` Steven Rostedt
  2018-07-11 19:41                         ` Hans de Goede
  2018-07-12 10:16                         ` Thomas Zimmermann
  0 siblings, 2 replies; 28+ messages in thread
From: Steven Rostedt @ 2018-07-11 19:19 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Hans de Goede, Thomas Zimmermann, Petr Mladek,
	Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Linux Kernel Mailing List, dri-devel, Sergey Senozhatsky

On Wed, 11 Jul 2018 19:56:02 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> > Have you seen Steven's suggestion which he send about the same time
> > as your mail I'm replying to here ? I personally think that doing
> > something like that makes sense (for as long as we have the need
> > for the lockless_fb_register debug hack).
> >
> > Note I've 2 patches ready to go to only fix this in fbcon.c, but I
> > think a more thorough fix makes sense.  
> 
> Yeah Steven's suggestion looks reasonable to fix this all for good.
> The #if 1 predates git history, so no idea why it was added or by whom
> :-)

I just sent the patch. If the printk maintainers take it, then you can
update the fb driver to set the ignore_console_lock_warning when
lockless_fb_register is set.

-- Steve

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 19:19                       ` Steven Rostedt
@ 2018-07-11 19:41                         ` Hans de Goede
  2018-07-12 10:16                         ` Thomas Zimmermann
  1 sibling, 0 replies; 28+ messages in thread
From: Hans de Goede @ 2018-07-11 19:41 UTC (permalink / raw)
  To: Steven Rostedt, Daniel Vetter
  Cc: Thomas Zimmermann, Petr Mladek, Linux Fbdev development list,
	Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List, dri-devel,
	Sergey Senozhatsky

Hi,

On 11-07-18 21:19, Steven Rostedt wrote:
> On Wed, 11 Jul 2018 19:56:02 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
>>> Have you seen Steven's suggestion which he send about the same time
>>> as your mail I'm replying to here ? I personally think that doing
>>> something like that makes sense (for as long as we have the need
>>> for the lockless_fb_register debug hack).
>>>
>>> Note I've 2 patches ready to go to only fix this in fbcon.c, but I
>>> think a more thorough fix makes sense.
>>
>> Yeah Steven's suggestion looks reasonable to fix this all for good.
>> The #if 1 predates git history, so no idea why it was added or by whom
>> :-)
> 
> I just sent the patch. If the printk maintainers take it, then you can
> update the fb driver to set the ignore_console_lock_warning when
> lockless_fb_register is set.

Thanks for doing this. I will wait with sending a fbcon / fbdev patch
till the fate of your patch is clear then.

Regards,

Hans

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 14:46     ` Thomas Zimmermann
  2018-07-11 14:52       ` Steven Rostedt
@ 2018-07-11 23:59       ` Sergey Senozhatsky
  1 sibling, 0 replies; 28+ messages in thread
From: Sergey Senozhatsky @ 2018-07-11 23:59 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: Hans de Goede, Bartlomiej Zolnierkiewicz, Petr Mladek,
	Sergey Senozhatsky, linux-fbdev, Steven Rostedt, dri-devel,
	linux-kernel

Hi,

On (07/11/18 16:46), Thomas Zimmermann wrote:
> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
> > Replace comments about places where the console lock should be held with
> > calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
> 
> Debugging fbcon sometimes requires to not take the console lock. This
> patch breaks the debugging workaround provided by
> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
> the missing lock.

Hmm. I once dealt with WARN_CONSOLE_UNLOCKED(), and back then, IIRC,
I really wanted to turn it into WARN_ONCE_CONSOLE_UNLOCKED(), which
would WARN_ON_ONCE() instead of WARN_ON(). It's just a bit too noisy
and verbose and a single backtrace was already enough.

	-ss

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
  2018-07-11 19:19                       ` Steven Rostedt
  2018-07-11 19:41                         ` Hans de Goede
@ 2018-07-12 10:16                         ` Thomas Zimmermann
  1 sibling, 0 replies; 28+ messages in thread
From: Thomas Zimmermann @ 2018-07-12 10:16 UTC (permalink / raw)
  To: Steven Rostedt, Daniel Vetter
  Cc: Petr Mladek, Linux Fbdev development list, Sergey Senozhatsky,
	Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List, dri-devel,
	Hans de Goede


[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

Am 11.07.2018 um 21:19 schrieb Steven Rostedt:
> I just sent the patch. If the printk maintainers take it, then you can
> update the fb driver to set the ignore_console_lock_warning when
> lockless_fb_register is set.

Thank you.

> 
> -- Steve
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2018-07-12 10:16 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180628090357epcas5p28361ab4b3ce11c179a167548f4851983@epcas5p2.samsung.com>
2018-06-28  9:03 ` [PATCH v5 0/3] console/fbcon: Add support for deferred console takeover Hans de Goede
2018-06-28  9:03   ` [PATCH v5 1/3] printk: Export is_console_locked Hans de Goede
2018-06-28  9:03   ` [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable Hans de Goede
2018-07-11 14:46     ` Thomas Zimmermann
2018-07-11 14:52       ` Steven Rostedt
2018-07-11 15:01         ` Hans de Goede
2018-07-11 15:07           ` Daniel Vetter
2018-07-11 15:07         ` Thomas Zimmermann
2018-07-11 15:14           ` Hans de Goede
2018-07-11 15:28             ` Daniel Vetter
2018-07-11 15:35               ` Hans de Goede
2018-07-11 15:41                 ` Steven Rostedt
2018-07-11 15:42                 ` Daniel Vetter
2018-07-11 17:35                   ` Hans de Goede
2018-07-11 17:56                     ` Daniel Vetter
2018-07-11 19:19                       ` Steven Rostedt
2018-07-11 19:41                         ` Hans de Goede
2018-07-12 10:16                         ` Thomas Zimmermann
2018-07-11 23:59       ` Sergey Senozhatsky
2018-06-28  9:03   ` [PATCH v5 3/3] console/fbcon: Add support for deferred console takeover Hans de Goede
2018-07-03 16:13     ` Sergey Senozhatsky
2018-07-03 16:14       ` Steven Rostedt
2018-07-03 16:36         ` Sergey Senozhatsky
2018-06-28 13:50   ` [PATCH v5 0/3] " Bartlomiej Zolnierkiewicz
2018-06-28 15:24     ` Hans de Goede
2018-06-28 22:44     ` Gustavo Padovan
2018-06-29 10:08       ` Bartlomiej Zolnierkiewicz
2018-06-29 12:51         ` Gustavo Padovan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).