LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2 00/19] fbcon patches, take two
@ 2022-02-08 21:08 Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
                   ` (18 more replies)
  0 siblings, 19 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter

Hi all,

Second round, mostly just compile fixed and some minor polish to commit
messages. Also MAINTAINERS patch and fbcon scrolling patches are out
because they landed already.

There's still a handful here that need review (and somehow intel-gfx-ci
just keeled over on this).

Cheers, Daniel

Daniel Vetter (19):
  fbcon: delete a few unneeded forward decl
  fbcon: Move fbcon_bmove(_rec) functions
  fbcon: Introduce wrapper for console->fb_info lookup
  fbcon: delete delayed loading code
  fbdev/sysfs: Fix locking
  fbcon: Use delayed work for cursor
  fbcon: Replace FBCON_FLAGS_INIT with a boolean
  fb: Delete fb_info->queue
  fbcon: Extract fbcon_open/release helpers
  fbcon: Ditch error handling for con2fb_release_oldinfo
  fbcon: move more common code into fb_open()
  fbcon: use lock_fb_info in fbcon_open/release
  fbcon: Consistently protect deferred_takeover with console_lock()
  fbcon: Move console_lock for register/unlink/unregister
  fbcon: Move more code into fbcon_release
  fbcon: untangle fbcon_exit
  fbcon: Maintain a private array of fb_info
  Revert "fbdev: Prevent probing generic drivers if a FB is already
    registered"
  fbdev: Make registered_fb[] private to fbmem.c

 drivers/video/fbdev/core/fbcon.c   | 692 ++++++++++++++---------------
 drivers/video/fbdev/core/fbcon.h   |   8 +-
 drivers/video/fbdev/core/fbmem.c   |  35 +-
 drivers/video/fbdev/core/fbsysfs.c |   2 +
 drivers/video/fbdev/efifb.c        |  11 -
 drivers/video/fbdev/simplefb.c     |  11 -
 include/linux/fb.h                 |   8 +-
 7 files changed, 342 insertions(+), 425 deletions(-)

-- 
2.34.1


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

* [PATCH v2 01/19] fbcon: delete a few unneeded forward decl
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:17   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
                   ` (17 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Geert Uytterhoeven, Daniel Vetter, Helge Deller,
	Daniel Vetter, Thomas Zimmermann, Du Cheng, Tetsuo Handa,
	Claudio Suarez, Greg Kroah-Hartman

I didn't bother with any code movement to fix the others, these just
got a bit in the way.

v2: Rebase on top of Helge's reverts.

Acked-by: Sam Ravnborg <sam@ravnborg.org> (v1)
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/core/fbcon.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 2fc1b80a26ad..235eaab37d84 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -163,29 +163,14 @@ static int fbcon_cursor_noblink;
  *  Interface used by the world
  */
 
-static const char *fbcon_startup(void);
-static void fbcon_init(struct vc_data *vc, int init);
-static void fbcon_deinit(struct vc_data *vc);
-static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
-			int width);
-static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos);
-static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,
-			int count, int ypos, int xpos);
 static void fbcon_clear_margins(struct vc_data *vc, int bottom_only);
-static void fbcon_cursor(struct vc_data *vc, int mode);
 static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
 			int height, int width);
-static int fbcon_switch(struct vc_data *vc);
-static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch);
 static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
 
 /*
  *  Internal routines
  */
-static __inline__ void ywrap_up(struct vc_data *vc, int count);
-static __inline__ void ywrap_down(struct vc_data *vc, int count);
-static __inline__ void ypan_up(struct vc_data *vc, int count);
-static __inline__ void ypan_down(struct vc_data *vc, int count);
 static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
 			    int dy, int dx, int height, int width, u_int y_break);
 static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
@@ -194,8 +179,8 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
 			      int line, int count, int dy);
 static void fbcon_modechanged(struct fb_info *info);
 static void fbcon_set_all_vcs(struct fb_info *info);
-static void fbcon_start(void);
 static void fbcon_exit(void);
+
 static struct device *fbcon_device;
 
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION
-- 
2.34.1


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

* [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 23:06   ` Javier Martinez Canillas
  2022-02-10 11:17   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup Daniel Vetter
                   ` (16 subsequent siblings)
  18 siblings, 2 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Daniel Vetter, Helge Deller, Daniel Vetter, Thomas Zimmermann,
	Du Cheng, Tetsuo Handa, Claudio Suarez, Greg Kroah-Hartman

Avoids two forward declarations, and more importantly, matches what
I've done in my fbcon scrolling restore patches - so I need this to
avoid a bunch of conflicts in rebasing since we ended up merging
Helge's series instead.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/core/fbcon.c | 134 +++++++++++++++----------------
 1 file changed, 65 insertions(+), 69 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 235eaab37d84..e925bb608e25 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -164,15 +164,11 @@ static int fbcon_cursor_noblink;
  */
 
 static void fbcon_clear_margins(struct vc_data *vc, int bottom_only);
-static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
-			int height, int width);
 static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
 
 /*
  *  Internal routines
  */
-static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
-			    int dy, int dx, int height, int width, u_int y_break);
 static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 			   int unit);
 static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
@@ -1667,6 +1663,71 @@ static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p,
 	}
 }
 
+static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
+			    int dy, int dx, int height, int width, u_int y_break)
+{
+	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fbcon_ops *ops = info->fbcon_par;
+	u_int b;
+
+	if (sy < y_break && sy + height > y_break) {
+		b = y_break - sy;
+		if (dy < sy) {	/* Avoid trashing self */
+			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+					y_break);
+			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+					height - b, width, y_break);
+		} else {
+			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+					height - b, width, y_break);
+			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+					y_break);
+		}
+		return;
+	}
+
+	if (dy < y_break && dy + height > y_break) {
+		b = y_break - dy;
+		if (dy < sy) {	/* Avoid trashing self */
+			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+					y_break);
+			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+					height - b, width, y_break);
+		} else {
+			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+					height - b, width, y_break);
+			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+					y_break);
+		}
+		return;
+	}
+	ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
+		   height, width);
+}
+
+static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
+			int height, int width)
+{
+	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fbcon_display *p = &fb_display[vc->vc_num];
+
+	if (fbcon_is_inactive(vc, info))
+		return;
+
+	if (!width || !height)
+		return;
+
+	/*  Split blits that cross physical y_wrap case.
+	 *  Pathological case involves 4 blits, better to use recursive
+	 *  code rather than unrolled case
+	 *
+	 *  Recursive invocations don't need to erase the cursor over and
+	 *  over again, so we use fbcon_bmove_rec()
+	 */
+	fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width,
+			p->vrows - p->yscroll);
+}
+
 static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
 		enum con_scroll dir, unsigned int count)
 {
@@ -1867,71 +1928,6 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
 }
 
 
-static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
-			int height, int width)
-{
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
-	struct fbcon_display *p = &fb_display[vc->vc_num];
-
-	if (fbcon_is_inactive(vc, info))
-		return;
-
-	if (!width || !height)
-		return;
-
-	/*  Split blits that cross physical y_wrap case.
-	 *  Pathological case involves 4 blits, better to use recursive
-	 *  code rather than unrolled case
-	 *
-	 *  Recursive invocations don't need to erase the cursor over and
-	 *  over again, so we use fbcon_bmove_rec()
-	 */
-	fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width,
-			p->vrows - p->yscroll);
-}
-
-static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
-			    int dy, int dx, int height, int width, u_int y_break)
-{
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
-	struct fbcon_ops *ops = info->fbcon_par;
-	u_int b;
-
-	if (sy < y_break && sy + height > y_break) {
-		b = y_break - sy;
-		if (dy < sy) {	/* Avoid trashing self */
-			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
-					y_break);
-			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
-					height - b, width, y_break);
-		} else {
-			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
-					height - b, width, y_break);
-			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
-					y_break);
-		}
-		return;
-	}
-
-	if (dy < y_break && dy + height > y_break) {
-		b = y_break - dy;
-		if (dy < sy) {	/* Avoid trashing self */
-			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
-					y_break);
-			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
-					height - b, width, y_break);
-		} else {
-			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
-					height - b, width, y_break);
-			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
-					y_break);
-		}
-		return;
-	}
-	ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
-		   height, width);
-}
-
 static void updatescrollmode_accel(struct fbcon_display *p,
 					struct fb_info *info,
 					struct vc_data *vc)
-- 
2.34.1


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

* [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:18   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 04/19] fbcon: delete delayed loading code Daniel Vetter
                   ` (15 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Helge Deller, Daniel Vetter,
	Greg Kroah-Hartman, Tetsuo Handa, Du Cheng, Claudio Suarez,
	Thomas Zimmermann

Half of it is protected by console_lock, but the other half is a lot
more awkward: Registration/deregistration of fbdev are serialized, but
we don't really clear out anything in con2fb_map and so there's
potential for use-after free mixups.

First step is to encapsulate the lookup.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbcon.c | 76 ++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 32 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index e925bb608e25..b75e638cb83d 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -110,6 +110,18 @@ static struct fbcon_display fb_display[MAX_NR_CONSOLES];
 static signed char con2fb_map[MAX_NR_CONSOLES];
 static signed char con2fb_map_boot[MAX_NR_CONSOLES];
 
+static struct fb_info *fbcon_info_from_console(int console)
+{
+	WARN_CONSOLE_UNLOCKED();
+
+	/*
+	 * Note that only con2fb_map is protected by the console lock,
+	 * registered_fb is protected by a separate mutex. This lookup can
+	 * therefore race.
+	 */
+	return registered_fb[con2fb_map[console]];
+}
+
 static int logo_lines;
 /* logo_shown is an index to vc_cons when >= 0; otherwise follows FBCON_LOGO
    enums.  */
@@ -199,7 +211,7 @@ static void fbcon_rotate(struct fb_info *info, u32 rotate)
 	if (!ops || ops->currcon == -1)
 		return;
 
-	fb_info = registered_fb[con2fb_map[ops->currcon]];
+	fb_info = fbcon_info_from_console(ops->currcon);
 
 	if (info == fb_info) {
 		struct fbcon_display *p = &fb_display[ops->currcon];
@@ -226,7 +238,7 @@ static void fbcon_rotate_all(struct fb_info *info, u32 rotate)
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		vc = vc_cons[i].d;
 		if (!vc || vc->vc_mode != KD_TEXT ||
-		    registered_fb[con2fb_map[i]] != info)
+		    fbcon_info_from_console(i) != info)
 			continue;
 
 		p = &fb_display[vc->vc_num];
@@ -356,7 +368,7 @@ static void fb_flashcursor(struct work_struct *work)
 		vc = vc_cons[ops->currcon].d;
 
 	if (!vc || !con_is_visible(vc) ||
- 	    registered_fb[con2fb_map[vc->vc_num]] != info ||
+	    fbcon_info_from_console(vc->vc_num) != info ||
 	    vc->vc_deccm != 1) {
 		console_unlock();
 		return;
@@ -791,7 +803,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
 	if (show_logo) {
 		struct vc_data *fg_vc = vc_cons[fg_console].d;
 		struct fb_info *fg_info =
-			registered_fb[con2fb_map[fg_console]];
+			fbcon_info_from_console(fg_console);
 
 		fbcon_prepare_logo(fg_vc, fg_info, fg_vc->vc_cols,
 				   fg_vc->vc_rows, fg_vc->vc_cols,
@@ -1014,7 +1026,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 	if (con2fb_map[vc->vc_num] == -1)
 		con2fb_map[vc->vc_num] = info_idx;
 
-	info = registered_fb[con2fb_map[vc->vc_num]];
+	info = fbcon_info_from_console(vc->vc_num);
 
 	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
 		logo_shown = FBCON_LOGO_DONTSHOW;
@@ -1231,7 +1243,7 @@ static void fbcon_deinit(struct vc_data *vc)
 static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
 			int width)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	struct fbcon_display *p = &fb_display[vc->vc_num];
@@ -1269,7 +1281,7 @@ static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
 static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,
 			int count, int ypos, int xpos)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
@@ -1289,7 +1301,7 @@ static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos)
 
 static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	if (!fbcon_is_inactive(vc, info))
@@ -1298,7 +1310,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
 
 static void fbcon_cursor(struct vc_data *vc, int mode)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
  	int c = scr_readw((u16 *) vc->vc_pos);
 
@@ -1392,7 +1404,7 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 
 static __inline__ void ywrap_up(struct vc_data *vc, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
@@ -1411,7 +1423,7 @@ static __inline__ void ywrap_up(struct vc_data *vc, int count)
 
 static __inline__ void ywrap_down(struct vc_data *vc, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
@@ -1430,7 +1442,7 @@ static __inline__ void ywrap_down(struct vc_data *vc, int count)
 
 static __inline__ void ypan_up(struct vc_data *vc, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
@@ -1454,7 +1466,7 @@ static __inline__ void ypan_up(struct vc_data *vc, int count)
 
 static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
@@ -1478,7 +1490,7 @@ static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
 
 static __inline__ void ypan_down(struct vc_data *vc, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fbcon_ops *ops = info->fbcon_par;
 
@@ -1502,7 +1514,7 @@ static __inline__ void ypan_down(struct vc_data *vc, int count)
 
 static __inline__ void ypan_down_redraw(struct vc_data *vc, int t, int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
@@ -1666,7 +1678,7 @@ static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p,
 static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
 			    int dy, int dx, int height, int width, u_int y_break)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	u_int b;
 
@@ -1708,7 +1720,7 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
 static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
 			int height, int width)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 
 	if (fbcon_is_inactive(vc, info))
@@ -1731,7 +1743,7 @@ static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
 static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
 		enum con_scroll dir, unsigned int count)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
 
@@ -1996,7 +2008,7 @@ static void updatescrollmode(struct fbcon_display *p,
 static int fbcon_resize(struct vc_data *vc, unsigned int width, 
 			unsigned int height, unsigned int user)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	struct fb_var_screeninfo var = info->var;
@@ -2065,7 +2077,7 @@ static int fbcon_switch(struct vc_data *vc)
 	struct fb_var_screeninfo var;
 	int i, ret, prev_console;
 
-	info = registered_fb[con2fb_map[vc->vc_num]];
+	info = fbcon_info_from_console(vc->vc_num);
 	ops = info->fbcon_par;
 
 	if (logo_shown >= 0) {
@@ -2079,7 +2091,7 @@ static int fbcon_switch(struct vc_data *vc)
 
 	prev_console = ops->currcon;
 	if (prev_console != -1)
-		old_info = registered_fb[con2fb_map[prev_console]];
+		old_info = fbcon_info_from_console(prev_console);
 	/*
 	 * FIXME: If we have multiple fbdev's loaded, we need to
 	 * update all info->currcon.  Perhaps, we can place this
@@ -2202,7 +2214,7 @@ static void fbcon_generic_blank(struct vc_data *vc, struct fb_info *info,
 
 static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	if (mode_switch) {
@@ -2244,7 +2256,7 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
 
 static int fbcon_debug_enter(struct vc_data *vc)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	ops->save_graphics = ops->graphics;
@@ -2257,7 +2269,7 @@ static int fbcon_debug_enter(struct vc_data *vc)
 
 static int fbcon_debug_leave(struct vc_data *vc)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	ops->graphics = ops->save_graphics;
@@ -2393,7 +2405,7 @@ static void set_vc_hi_font(struct vc_data *vc, bool set)
 static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
 			     const u8 * data, int userfont)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	struct fbcon_ops *ops = info->fbcon_par;
 	struct fbcon_display *p = &fb_display[vc->vc_num];
 	int resize;
@@ -2447,7 +2459,7 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
 static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
 			  unsigned int flags)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	unsigned charcount = font->charcount;
 	int w = font->width;
 	int h = font->height;
@@ -2511,7 +2523,7 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
 
 static int fbcon_set_def_font(struct vc_data *vc, struct console_font *font, char *name)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	const struct font_desc *f;
 
 	if (!name)
@@ -2535,7 +2547,7 @@ static struct fb_cmap palette_cmap = {
 
 static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table)
 {
-	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
 	int i, j, k, depth;
 	u8 val;
 
@@ -2651,7 +2663,7 @@ static void fbcon_modechanged(struct fb_info *info)
 		return;
 	vc = vc_cons[ops->currcon].d;
 	if (vc->vc_mode != KD_TEXT ||
-	    registered_fb[con2fb_map[ops->currcon]] != info)
+	    fbcon_info_from_console(ops->currcon) != info)
 		return;
 
 	p = &fb_display[vc->vc_num];
@@ -2691,7 +2703,7 @@ static void fbcon_set_all_vcs(struct fb_info *info)
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
 		vc = vc_cons[i].d;
 		if (!vc || vc->vc_mode != KD_TEXT ||
-		    registered_fb[con2fb_map[i]] != info)
+		    fbcon_info_from_console(i) != info)
 			continue;
 
 		if (con_is_visible(vc)) {
@@ -2954,7 +2966,7 @@ void fbcon_fb_blanked(struct fb_info *info, int blank)
 
 	vc = vc_cons[ops->currcon].d;
 	if (vc->vc_mode != KD_TEXT ||
-			registered_fb[con2fb_map[ops->currcon]] != info)
+			fbcon_info_from_console(ops->currcon) != info)
 		return;
 
 	if (con_is_visible(vc)) {
@@ -2974,7 +2986,7 @@ void fbcon_new_modelist(struct fb_info *info)
 	const struct fb_videomode *mode;
 
 	for (i = first_fb_vc; i <= last_fb_vc; i++) {
-		if (registered_fb[con2fb_map[i]] != info)
+		if (fbcon_info_from_console(i) != info)
 			continue;
 		if (!fb_display[i].mode)
 			continue;
-- 
2.34.1


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

* [PATCH v2 04/19] fbcon: delete delayed loading code
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (2 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:20   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 05/19] fbdev/sysfs: Fix locking Daniel Vetter
                   ` (14 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Helge Deller, Daniel Vetter,
	Claudio Suarez, Greg Kroah-Hartman, Tetsuo Handa, Du Cheng

Before

commit 6104c37094e729f3d4ce65797002112735d49cd1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Aug 1 17:32:07 2017 +0200

    fbcon: Make fbcon a built-time depency for fbdev

it was possible to load fbcon and fbdev drivers in any order, which
means that fbcon init had to handle the case where fbdev drivers where
already registered.

This is no longer possible, hence delete that code.

Note that the exit case is a bit more complex and will be done in a
separate patch.

Since I had to audit the entire fbcon load code I also spotted a wrong
function name in a comment in fbcon_startup(), which this patch also
fixes.

v2: Explain why we also fix the comment (Sam)

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Du Cheng <ducheng2@gmail.com>
---
 drivers/video/fbdev/core/fbcon.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index b75e638cb83d..83f0223f5333 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -944,7 +944,7 @@ static const char *fbcon_startup(void)
 		return display_desc;
 	/*
 	 * Instead of blindly using registered_fb[0], we use info_idx, set by
-	 * fb_console_init();
+	 * fbcon_fb_registered();
 	 */
 	info = registered_fb[info_idx];
 	if (!info)
@@ -3299,17 +3299,6 @@ static void fbcon_start(void)
 		return;
 	}
 #endif
-
-	if (num_registered_fb) {
-		int i;
-
-		for_each_registered_fb(i) {
-			info_idx = i;
-			break;
-		}
-
-		do_fbcon_takeover(0);
-	}
 }
 
 static void fbcon_exit(void)
-- 
2.34.1


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

* [PATCH v2 05/19] fbdev/sysfs: Fix locking
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (3 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 04/19] fbcon: delete delayed loading code Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:22   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
                   ` (13 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Helge Deller, Daniel Vetter,
	Qing Wang

fb_set_var requires we hold the fb_info lock. Or at least this now
matches what the ioctl does ...

Note that ps3fb and sh_mobile_lcdcfb are busted in different ways here,
but I will not fix them up.

Also in practice this isn't a big deal, because really variable fbdev
state is actually protected by console_lock (because fbcon just
doesn't bother with lock_fb_info() at all), and lock_fb_info
protecting anything is really just a neat lie. But that's a much
bigger fish to fry.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Qing Wang <wangqing@vivo.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/video/fbdev/core/fbsysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index 26892940c213..8c1ee9ecec3d 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -91,9 +91,11 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 
 	var->activate |= FB_ACTIVATE_FORCE;
 	console_lock();
+	lock_fb_info(fb_info);
 	err = fb_set_var(fb_info, var);
 	if (!err)
 		fbcon_update_vcs(fb_info, var->activate & FB_ACTIVATE_ALL);
+	unlock_fb_info(fb_info);
 	console_unlock();
 	if (err)
 		return err;
-- 
2.34.1


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

* [PATCH v2 06/19] fbcon: Use delayed work for cursor
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (4 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 05/19] fbdev/sysfs: Fix locking Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 23:59   ` Javier Martinez Canillas
                     ` (2 more replies)
  2022-02-08 21:08 ` [PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean Daniel Vetter
                   ` (12 subsequent siblings)
  18 siblings, 3 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Daniel Vetter, Daniel Vetter, Claudio Suarez, Du Cheng,
	Thomas Zimmermann, Greg Kroah-Hartman, Tetsuo Handa

Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
code we initialize the cursor_work completely when we allocate the
fbcon_ops structure, instead of trying to cope with console
re-initialization.

The motiviation here is that fbcon code stops using the fb_info.queue,
which helps with locking issues around cleanup and all that in a later
patch.

Also note that this allows us to ditch the hand-rolled work cleanup in
fbcon_exit - we already call fbcon_del_cursor_timer, which takes care
of everything. Plus this was racy anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/video/fbdev/core/fbcon.c | 85 +++++++++++++-------------------
 drivers/video/fbdev/core/fbcon.h |  4 +-
 2 files changed, 35 insertions(+), 54 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 83f0223f5333..a368ed602e2e 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -350,8 +350,8 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
 
 static void fb_flashcursor(struct work_struct *work)
 {
-	struct fb_info *info = container_of(work, struct fb_info, queue);
-	struct fbcon_ops *ops = info->fbcon_par;
+	struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work);
+	struct fb_info *info;
 	struct vc_data *vc = NULL;
 	int c;
 	int mode;
@@ -364,7 +364,10 @@ static void fb_flashcursor(struct work_struct *work)
 	if (ret == 0)
 		return;
 
-	if (ops && ops->currcon != -1)
+	/* protected by console_lock */
+	info = ops->info;
+
+	if (ops->currcon != -1)
 		vc = vc_cons[ops->currcon].d;
 
 	if (!vc || !con_is_visible(vc) ||
@@ -380,42 +383,25 @@ static void fb_flashcursor(struct work_struct *work)
 	ops->cursor(vc, info, mode, get_color(vc, info, c, 1),
 		    get_color(vc, info, c, 0));
 	console_unlock();
-}
 
-static void cursor_timer_handler(struct timer_list *t)
-{
-	struct fbcon_ops *ops = from_timer(ops, t, cursor_timer);
-	struct fb_info *info = ops->info;
-
-	queue_work(system_power_efficient_wq, &info->queue);
-	mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
+	queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
+			   ops->cur_blink_jiffies);
 }
 
-static void fbcon_add_cursor_timer(struct fb_info *info)
+static void fbcon_add_cursor_work(struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if ((!info->queue.func || info->queue.func == fb_flashcursor) &&
-	    !(ops->flags & FBCON_FLAGS_CURSOR_TIMER) &&
-	    !fbcon_cursor_noblink) {
-		if (!info->queue.func)
-			INIT_WORK(&info->queue, fb_flashcursor);
-
-		timer_setup(&ops->cursor_timer, cursor_timer_handler, 0);
-		mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
-		ops->flags |= FBCON_FLAGS_CURSOR_TIMER;
-	}
+	if (!fbcon_cursor_noblink)
+		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
+				   ops->cur_blink_jiffies);
 }
 
-static void fbcon_del_cursor_timer(struct fb_info *info)
+static void fbcon_del_cursor_work(struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
-	if (info->queue.func == fb_flashcursor &&
-	    ops->flags & FBCON_FLAGS_CURSOR_TIMER) {
-		del_timer_sync(&ops->cursor_timer);
-		ops->flags &= ~FBCON_FLAGS_CURSOR_TIMER;
-	}
+	cancel_delayed_work_sync(&ops->cursor_work);
 }
 
 #ifndef MODULE
@@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
 		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
 		if (!ops)
 			err = -ENOMEM;
+
+		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
 	}
 
 	if (!err) {
@@ -751,7 +739,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 	}
 
 	if (!err) {
-		fbcon_del_cursor_timer(oldinfo);
+		fbcon_del_cursor_work(oldinfo);
 		kfree(ops->cursor_state.mask);
 		kfree(ops->cursor_data);
 		kfree(ops->cursor_src);
@@ -867,7 +855,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 				 logo_shown != FBCON_LOGO_DONTSHOW);
 
 		if (!found)
-			fbcon_add_cursor_timer(info);
+			fbcon_add_cursor_work(info);
 		con2fb_map_boot[unit] = newidx;
 		con2fb_init_display(vc, info, unit, show_logo);
 	}
@@ -964,6 +952,8 @@ static const char *fbcon_startup(void)
 		return NULL;
 	}
 
+	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
+
 	ops->currcon = -1;
 	ops->graphics = 1;
 	ops->cur_rotate = -1;
@@ -1006,7 +996,7 @@ static const char *fbcon_startup(void)
 		 info->var.yres,
 		 info->var.bits_per_pixel);
 
-	fbcon_add_cursor_timer(info);
+	fbcon_add_cursor_work(info);
 	return display_desc;
 }
 
@@ -1194,7 +1184,7 @@ static void fbcon_deinit(struct vc_data *vc)
 		goto finished;
 
 	if (con_is_visible(vc))
-		fbcon_del_cursor_timer(info);
+		fbcon_del_cursor_work(info);
 
 	ops->flags &= ~FBCON_FLAGS_INIT;
 finished:
@@ -1320,9 +1310,9 @@ static void fbcon_cursor(struct vc_data *vc, int mode)
 		return;
 
 	if (vc->vc_cursor_type & CUR_SW)
-		fbcon_del_cursor_timer(info);
+		fbcon_del_cursor_work(info);
 	else
-		fbcon_add_cursor_timer(info);
+		fbcon_add_cursor_work(info);
 
 	ops->cursor_flash = (mode == CM_ERASE) ? 0 : 1;
 
@@ -2132,14 +2122,14 @@ static int fbcon_switch(struct vc_data *vc)
 		}
 
 		if (old_info != info)
-			fbcon_del_cursor_timer(old_info);
+			fbcon_del_cursor_work(old_info);
 	}
 
 	if (fbcon_is_inactive(vc, info) ||
 	    ops->blank_state != FB_BLANK_UNBLANK)
-		fbcon_del_cursor_timer(info);
+		fbcon_del_cursor_work(info);
 	else
-		fbcon_add_cursor_timer(info);
+		fbcon_add_cursor_work(info);
 
 	set_blitting_type(vc, info);
 	ops->cursor_reset = 1;
@@ -2247,9 +2237,9 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
 
 	if (mode_switch || fbcon_is_inactive(vc, info) ||
 	    ops->blank_state != FB_BLANK_UNBLANK)
-		fbcon_del_cursor_timer(info);
+		fbcon_del_cursor_work(info);
 	else
-		fbcon_add_cursor_timer(info);
+		fbcon_add_cursor_work(info);
 
 	return 0;
 }
@@ -3181,7 +3171,7 @@ static ssize_t show_cursor_blink(struct device *device,
 	if (!ops)
 		goto err;
 
-	blink = (ops->flags & FBCON_FLAGS_CURSOR_TIMER) ? 1 : 0;
+	blink = delayed_work_pending(&ops->cursor_work);
 err:
 	console_unlock();
 	return snprintf(buf, PAGE_SIZE, "%d\n", blink);
@@ -3210,10 +3200,10 @@ static ssize_t store_cursor_blink(struct device *device,
 
 	if (blink) {
 		fbcon_cursor_noblink = 0;
-		fbcon_add_cursor_timer(info);
+		fbcon_add_cursor_work(info);
 	} else {
 		fbcon_cursor_noblink = 1;
-		fbcon_del_cursor_timer(info);
+		fbcon_del_cursor_work(info);
 	}
 
 err:
@@ -3314,15 +3304,9 @@ static void fbcon_exit(void)
 #endif
 
 	for_each_registered_fb(i) {
-		int pending = 0;
-
 		mapped = 0;
 		info = registered_fb[i];
 
-		if (info->queue.func)
-			pending = cancel_work_sync(&info->queue);
-		pr_debug("fbcon: %s pending work\n", (pending ? "canceled" : "no"));
-
 		for (j = first_fb_vc; j <= last_fb_vc; j++) {
 			if (con2fb_map[j] == i) {
 				mapped = 1;
@@ -3338,15 +3322,12 @@ static void fbcon_exit(void)
 			if (info->fbcon_par) {
 				struct fbcon_ops *ops = info->fbcon_par;
 
-				fbcon_del_cursor_timer(info);
+				fbcon_del_cursor_work(info);
 				kfree(ops->cursor_src);
 				kfree(ops->cursor_state.mask);
 				kfree(info->fbcon_par);
 				info->fbcon_par = NULL;
 			}
-
-			if (info->queue.func == fb_flashcursor)
-				info->queue.func = NULL;
 		}
 	}
 }
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 969d41ecede5..6708ca0048aa 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -14,11 +14,11 @@
 #include <linux/types.h>
 #include <linux/vt_buffer.h>
 #include <linux/vt_kern.h>
+#include <linux/workqueue.h>
 
 #include <asm/io.h>
 
 #define FBCON_FLAGS_INIT         1
-#define FBCON_FLAGS_CURSOR_TIMER 2
 
    /*
     *    This is the interface between the low-level console driver and the
@@ -68,7 +68,7 @@ struct fbcon_ops {
 	int  (*update_start)(struct fb_info *info);
 	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
 	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
-	struct timer_list cursor_timer; /* Cursor timer */
+	struct delayed_work cursor_work; /* Cursor timer */
 	struct fb_cursor cursor_state;
 	struct fbcon_display *p;
 	struct fb_info *info;
-- 
2.34.1


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

* [PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (5 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 08/19] fb: Delete fb_info->queue Daniel Vetter
                   ` (11 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Thomas Zimmermann, Sam Ravnborg, Daniel Vetter, Daniel Vetter,
	Tetsuo Handa, Greg Kroah-Hartman, Du Cheng, Claudio Suarez

It's only one flag and slightly tidier code.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Claudio Suarez <cssk@net-c.es>
---
 drivers/video/fbdev/core/fbcon.c | 11 +++++------
 drivers/video/fbdev/core/fbcon.h |  4 +---
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index a368ed602e2e..058e885d24f6 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -775,7 +775,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
 
 	ops->currcon = fg_console;
 
-	if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT)) {
+	if (info->fbops->fb_set_par && !ops->initialized) {
 		ret = info->fbops->fb_set_par(info);
 
 		if (ret)
@@ -784,7 +784,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
 				"error code %d\n", ret);
 	}
 
-	ops->flags |= FBCON_FLAGS_INIT;
+	ops->initialized = true;
 	ops->graphics = 0;
 	fbcon_set_disp(info, &info->var, unit);
 
@@ -1103,8 +1103,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 	 * We need to do it in fbcon_init() to prevent screen corruption.
 	 */
 	if (con_is_visible(vc) && vc->vc_mode == KD_TEXT) {
-		if (info->fbops->fb_set_par &&
-		    !(ops->flags & FBCON_FLAGS_INIT)) {
+		if (info->fbops->fb_set_par && !ops->initialized) {
 			ret = info->fbops->fb_set_par(info);
 
 			if (ret)
@@ -1113,7 +1112,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 					"error code %d\n", ret);
 		}
 
-		ops->flags |= FBCON_FLAGS_INIT;
+		ops->initialized = true;
 	}
 
 	ops->graphics = 0;
@@ -1186,7 +1185,7 @@ static void fbcon_deinit(struct vc_data *vc)
 	if (con_is_visible(vc))
 		fbcon_del_cursor_work(info);
 
-	ops->flags &= ~FBCON_FLAGS_INIT;
+	ops->initialized = false;
 finished:
 
 	fbcon_free_font(p, free_font);
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 6708ca0048aa..0eaf54a21151 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -18,8 +18,6 @@
 
 #include <asm/io.h>
 
-#define FBCON_FLAGS_INIT         1
-
    /*
     *    This is the interface between the low-level console driver and the
     *    low-level frame buffer device
@@ -79,7 +77,7 @@ struct fbcon_ops {
 	int    blank_state;
 	int    graphics;
 	int    save_graphics; /* for debug enter/leave */
-	int    flags;
+	bool   initialized;
 	int    rotate;
 	int    cur_rotate;
 	char  *cursor_data;
-- 
2.34.1


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

* [PATCH v2 08/19] fb: Delete fb_info->queue
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (6 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:38   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Daniel Vetter
                   ` (10 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Helge Deller

It was only used by fbcon, and that now switched to its own,
private work.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
 include/linux/fb.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3d7306c9a706..23b19cf8bccd 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -449,7 +449,6 @@ struct fb_info {
 	struct fb_var_screeninfo var;	/* Current var */
 	struct fb_fix_screeninfo fix;	/* Current fix */
 	struct fb_monspecs monspecs;	/* Current Monitor specs */
-	struct work_struct queue;	/* Framebuffer event queue */
 	struct fb_pixmap pixmap;	/* Image hardware mapper */
 	struct fb_pixmap sprite;	/* Cursor hardware mapper */
 	struct fb_cmap cmap;		/* Current cmap */
-- 
2.34.1


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

* [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (7 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 08/19] fb: Delete fb_info->queue Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 11:46   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo Daniel Vetter
                   ` (9 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Claudio Suarez,
	Greg Kroah-Hartman, Tetsuo Handa, Du Cheng

There's two minor behaviour changes in here:
- in error paths we now consistently call fb_ops->fb_release
- fb_release really can't fail (fbmem.c ignores it too) and there's no
  reasonable cleanup we can do anyway.

Note that everything in fbcon.c is protected by the big console_lock()
lock (especially all the global variables), so the minor changes in
ordering of setup/cleanup do not matter.

v2: Explain a bit better why this is all correct (Sam)

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Du Cheng <ducheng2@gmail.com>
---
 drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++----------------
 1 file changed, 53 insertions(+), 54 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 058e885d24f6..3e1a3e7bf527 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
 
 #endif /* CONFIG_MISC_TILEBLITTING */
 
+static int fbcon_open(struct fb_info *info)
+{
+	if (!try_module_get(info->fbops->owner))
+		return -ENODEV;
+
+	if (info->fbops->fb_open &&
+	    info->fbops->fb_open(info, 0)) {
+		module_put(info->fbops->owner);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void fbcon_release(struct fb_info *info)
+{
+	if (info->fbops->fb_release)
+		info->fbops->fb_release(info, 0);
+
+	module_put(info->fbops->owner);
+}
 
 static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
 				  int unit, int oldidx)
 {
 	struct fbcon_ops *ops = NULL;
-	int err = 0;
-
-	if (!try_module_get(info->fbops->owner))
-		err = -ENODEV;
+	int err;
 
-	if (!err && info->fbops->fb_open &&
-	    info->fbops->fb_open(info, 0))
-		err = -ENODEV;
+	err = fbcon_open(info);
+	if (err)
+		return err;
 
 	if (!err) {
 		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
@@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
 
 	if (err) {
 		con2fb_map[unit] = oldidx;
-		module_put(info->fbops->owner);
+		fbcon_release(info);
 	}
 
 	return err;
@@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 				  int oldidx, int found)
 {
 	struct fbcon_ops *ops = oldinfo->fbcon_par;
-	int err = 0, ret;
+	int ret;
 
-	if (oldinfo->fbops->fb_release &&
-	    oldinfo->fbops->fb_release(oldinfo, 0)) {
-		con2fb_map[unit] = oldidx;
-		if (!found && newinfo->fbops->fb_release)
-			newinfo->fbops->fb_release(newinfo, 0);
-		if (!found)
-			module_put(newinfo->fbops->owner);
-		err = -ENODEV;
-	}
+	fbcon_release(oldinfo);
 
-	if (!err) {
-		fbcon_del_cursor_work(oldinfo);
-		kfree(ops->cursor_state.mask);
-		kfree(ops->cursor_data);
-		kfree(ops->cursor_src);
-		kfree(ops->fontbuffer);
-		kfree(oldinfo->fbcon_par);
-		oldinfo->fbcon_par = NULL;
-		module_put(oldinfo->fbops->owner);
-		/*
-		  If oldinfo and newinfo are driving the same hardware,
-		  the fb_release() method of oldinfo may attempt to
-		  restore the hardware state.  This will leave the
-		  newinfo in an undefined state. Thus, a call to
-		  fb_set_par() may be needed for the newinfo.
-		*/
-		if (newinfo && newinfo->fbops->fb_set_par) {
-			ret = newinfo->fbops->fb_set_par(newinfo);
+	fbcon_del_cursor_work(oldinfo);
+	kfree(ops->cursor_state.mask);
+	kfree(ops->cursor_data);
+	kfree(ops->cursor_src);
+	kfree(ops->fontbuffer);
+	kfree(oldinfo->fbcon_par);
+	oldinfo->fbcon_par = NULL;
+	/*
+	  If oldinfo and newinfo are driving the same hardware,
+	  the fb_release() method of oldinfo may attempt to
+	  restore the hardware state.  This will leave the
+	  newinfo in an undefined state. Thus, a call to
+	  fb_set_par() may be needed for the newinfo.
+	*/
+	if (newinfo && newinfo->fbops->fb_set_par) {
+		ret = newinfo->fbops->fb_set_par(newinfo);
 
-			if (ret)
-				printk(KERN_ERR "con2fb_release_oldinfo: "
-					"detected unhandled fb_set_par error, "
-					"error code %d\n", ret);
-		}
+		if (ret)
+			printk(KERN_ERR "con2fb_release_oldinfo: "
+				"detected unhandled fb_set_par error, "
+				"error code %d\n", ret);
 	}
 
-	return err;
+	return 0;
 }
 
 static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
@@ -919,7 +926,6 @@ static const char *fbcon_startup(void)
 	struct fbcon_display *p = &fb_display[fg_console];
 	struct vc_data *vc = vc_cons[fg_console].d;
 	const struct font_desc *font = NULL;
-	struct module *owner;
 	struct fb_info *info = NULL;
 	struct fbcon_ops *ops;
 	int rows, cols;
@@ -938,17 +944,12 @@ static const char *fbcon_startup(void)
 	if (!info)
 		return NULL;
 	
-	owner = info->fbops->owner;
-	if (!try_module_get(owner))
+	if (fbcon_open(info))
 		return NULL;
-	if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) {
-		module_put(owner);
-		return NULL;
-	}
 
 	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
 	if (!ops) {
-		module_put(owner);
+		fbcon_release(info);
 		return NULL;
 	}
 
@@ -3314,10 +3315,6 @@ static void fbcon_exit(void)
 		}
 
 		if (mapped) {
-			if (info->fbops->fb_release)
-				info->fbops->fb_release(info, 0);
-			module_put(info->fbops->owner);
-
 			if (info->fbcon_par) {
 				struct fbcon_ops *ops = info->fbcon_par;
 
@@ -3327,6 +3324,8 @@ static void fbcon_exit(void)
 				kfree(info->fbcon_par);
 				info->fbcon_par = NULL;
 			}
+
+			fbcon_release(info);
 		}
 	}
 }
-- 
2.34.1


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

* [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (8 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 14:14   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 11/19] fbcon: move more common code into fb_open() Daniel Vetter
                   ` (8 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Thomas Zimmermann,
	Greg Kroah-Hartman, Claudio Suarez, Du Cheng, Tetsuo Handa

It doesn't ever fail anymore.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/video/fbdev/core/fbcon.c | 37 +++++++++++---------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 3e1a3e7bf527..a60891005d44 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -739,9 +739,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
 	return err;
 }
 
-static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
-				  struct fb_info *newinfo, int unit,
-				  int oldidx, int found)
+static void con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
+				   struct fb_info *newinfo)
 {
 	struct fbcon_ops *ops = oldinfo->fbcon_par;
 	int ret;
@@ -770,8 +769,6 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 				"detected unhandled fb_set_par error, "
 				"error code %d\n", ret);
 	}
-
-	return 0;
 }
 
 static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
@@ -825,7 +822,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 	int oldidx = con2fb_map[unit];
 	struct fb_info *info = registered_fb[newidx];
 	struct fb_info *oldinfo = NULL;
-	int found, err = 0;
+	int found, err = 0, show_logo;
 
 	WARN_CONSOLE_UNLOCKED();
 
@@ -854,18 +851,15 @@ static int set_con2fb_map(int unit, int newidx, int user)
 	 * fbcon should release it.
 	 */
 	if (!err && oldinfo && !search_fb_in_map(oldidx))
-		err = con2fb_release_oldinfo(vc, oldinfo, info, unit, oldidx,
-					     found);
+		con2fb_release_oldinfo(vc, oldinfo, info);
 
-	if (!err) {
-		int show_logo = (fg_console == 0 && !user &&
-				 logo_shown != FBCON_LOGO_DONTSHOW);
+	show_logo = (fg_console == 0 && !user &&
+			 logo_shown != FBCON_LOGO_DONTSHOW);
 
-		if (!found)
-			fbcon_add_cursor_work(info);
-		con2fb_map_boot[unit] = newidx;
-		con2fb_init_display(vc, info, unit, show_logo);
-	}
+	if (!found)
+		fbcon_add_cursor_work(info);
+	con2fb_map_boot[unit] = newidx;
+	con2fb_init_display(vc, info, unit, show_logo);
 
 	if (!search_fb_in_map(info_idx))
 		info_idx = newidx;
@@ -2769,7 +2763,7 @@ static inline void fbcon_unbind(void) {}
 /* called with console_lock held */
 void fbcon_fb_unbind(struct fb_info *info)
 {
-	int i, new_idx = -1, ret = 0;
+	int i, new_idx = -1;
 	int idx = info->node;
 
 	WARN_CONSOLE_UNLOCKED();
@@ -2803,13 +2797,8 @@ void fbcon_fb_unbind(struct fb_info *info)
 			if (con2fb_map[i] == idx) {
 				con2fb_map[i] = -1;
 				if (!search_fb_in_map(idx)) {
-					ret = con2fb_release_oldinfo(vc_cons[i].d,
-								     info, NULL, i,
-								     idx, 0);
-					if (ret) {
-						con2fb_map[i] = idx;
-						return;
-					}
+					con2fb_release_oldinfo(vc_cons[i].d,
+							       info, NULL);
 				}
 			}
 		}
-- 
2.34.1


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

* [PATCH v2 11/19] fbcon: move more common code into fb_open()
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (9 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-10 14:16   ` Thomas Zimmermann
  2022-02-08 21:08 ` [PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release Daniel Vetter
                   ` (7 subsequent siblings)
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, kernel test robot, Daniel Vetter, Daniel Vetter,
	Greg Kroah-Hartman, Tetsuo Handa, Thomas Zimmermann,
	Claudio Suarez, Du Cheng

No idea why con2fb_acquire_newinfo() initializes much less than
fbcon_startup(), but so be it. From a quick look most of the
un-initialized stuff should be fairly harmless, but who knows.

Note that the error handling for the con2fb_acquire_newinfo() failure
case was very strange: Callers updated con2fb_map to the new value
before calling this function, but upon error con2fb_acquire_newinfo
reset it to the old value. Since I removed the call to fbcon_release
anyway that strange error path was sticking out like a sore thumb,
hence I removed it. Which also allows us to remove the oldidx
parameter from that function.

v2: Explain what's going on with oldidx and error paths (Sam)

v3: Drop unused variable (0day)

Acked-by: Sam Ravnborg <sam@ravnborg.org> (v2)
Cc: kernel test robot <lkp@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
---
 drivers/video/fbdev/core/fbcon.c | 75 +++++++++++++-------------------
 1 file changed, 30 insertions(+), 45 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index a60891005d44..f0213a0e3870 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -682,8 +682,18 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
 
 #endif /* CONFIG_MISC_TILEBLITTING */
 
+static void fbcon_release(struct fb_info *info)
+{
+	if (info->fbops->fb_release)
+		info->fbops->fb_release(info, 0);
+
+	module_put(info->fbops->owner);
+}
+
 static int fbcon_open(struct fb_info *info)
 {
+	struct fbcon_ops *ops;
+
 	if (!try_module_get(info->fbops->owner))
 		return -ENODEV;
 
@@ -693,48 +703,31 @@ static int fbcon_open(struct fb_info *info)
 		return -ENODEV;
 	}
 
-	return 0;
-}
+	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
+	if (!ops) {
+		fbcon_release(info);
+		return -ENOMEM;
+	}
 
-static void fbcon_release(struct fb_info *info)
-{
-	if (info->fbops->fb_release)
-		info->fbops->fb_release(info, 0);
+	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
+	ops->info = info;
+	info->fbcon_par = ops;
+	ops->cur_blink_jiffies = HZ / 5;
 
-	module_put(info->fbops->owner);
+	return 0;
 }
 
 static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
-				  int unit, int oldidx)
+				  int unit)
 {
-	struct fbcon_ops *ops = NULL;
 	int err;
 
 	err = fbcon_open(info);
 	if (err)
 		return err;
 
-	if (!err) {
-		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
-		if (!ops)
-			err = -ENOMEM;
-
-		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
-	}
-
-	if (!err) {
-		ops->cur_blink_jiffies = HZ / 5;
-		ops->info = info;
-		info->fbcon_par = ops;
-
-		if (vc)
-			set_blitting_type(vc, info);
-	}
-
-	if (err) {
-		con2fb_map[unit] = oldidx;
-		fbcon_release(info);
-	}
+	if (vc)
+		set_blitting_type(vc, info);
 
 	return err;
 }
@@ -842,9 +835,11 @@ static int set_con2fb_map(int unit, int newidx, int user)
 
 	found = search_fb_in_map(newidx);
 
-	con2fb_map[unit] = newidx;
-	if (!err && !found)
-		err = con2fb_acquire_newinfo(vc, info, unit, oldidx);
+	if (!err && !found) {
+		err = con2fb_acquire_newinfo(vc, info, unit);
+		if (!err)
+			con2fb_map[unit] = newidx;
+	}
 
 	/*
 	 * If old fb is not mapped to any of the consoles,
@@ -941,20 +936,10 @@ static const char *fbcon_startup(void)
 	if (fbcon_open(info))
 		return NULL;
 
-	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
-	if (!ops) {
-		fbcon_release(info);
-		return NULL;
-	}
-
-	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
-
+	ops = info->fbcon_par;
 	ops->currcon = -1;
 	ops->graphics = 1;
 	ops->cur_rotate = -1;
-	ops->cur_blink_jiffies = HZ / 5;
-	ops->info = info;
-	info->fbcon_par = ops;
 
 	p->con_rotate = initial_rotation;
 	if (p->con_rotate == -1)
@@ -1024,7 +1009,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 		return;
 
 	if (!info->fbcon_par)
-		con2fb_acquire_newinfo(vc, info, vc->vc_num, -1);
+		con2fb_acquire_newinfo(vc, info, vc->vc_num);
 
 	/* If we are not the first console on this
 	   fb, copy the font from that console */
-- 
2.34.1


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

* [PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (10 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 11/19] fbcon: move more common code into fb_open() Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock() Daniel Vetter
                   ` (6 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Claudio Suarez,
	Tetsuo Handa, Thomas Zimmermann, Greg Kroah-Hartman, Du Cheng,
	Matthew Wilcox, William Kucharski, Alex Deucher, Zheyu Ma,
	Zhen Lei, Xiyu Yang

Now we get to the real motiviation, because fbmem.c insists that
that's the right lock for these.

Ofc fbcon.c has a lot more places where it probably should call
lock_fb_info(). But looking at fbmem.c at least most of these seem to
be protected by console_lock() too, which is probably what papers over
any issues.

Note that this means we're shuffling around a bit the locking sections
for some of the console takeover and unbind paths, but not all:
- console binding/unbinding from the console layer never with
lock_fb_info
- unbind (as opposed to unlink) never bother with lock_fb_info

Also the real serialization against set_par and set_pan are still
doing by wrapping the entire ioctl code in console_lock(). So this
shuffling shouldn't be worse than what we had from a "can you trigger
races?" pov, but it's at least clearer.

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

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index f0213a0e3870..cc960bf49991 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -684,8 +684,10 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
 
 static void fbcon_release(struct fb_info *info)
 {
+	lock_fb_info(info);
 	if (info->fbops->fb_release)
 		info->fbops->fb_release(info, 0);
+	unlock_fb_info(info);
 
 	module_put(info->fbops->owner);
 }
@@ -697,11 +699,14 @@ static int fbcon_open(struct fb_info *info)
 	if (!try_module_get(info->fbops->owner))
 		return -ENODEV;
 
+	lock_fb_info(info);
 	if (info->fbops->fb_open &&
 	    info->fbops->fb_open(info, 0)) {
+		unlock_fb_info(info);
 		module_put(info->fbops->owner);
 		return -ENODEV;
 	}
+	unlock_fb_info(info);
 
 	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
 	if (!ops) {
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index ad9aac06427a..37656883e7bd 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1674,9 +1674,7 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 		console_lock();
 	else
 		atomic_inc(&ignore_console_lock_warning);
-	lock_fb_info(fb_info);
 	ret = fbcon_fb_registered(fb_info);
-	unlock_fb_info(fb_info);
 
 	if (!lockless_register_fb)
 		console_unlock();
@@ -1693,9 +1691,7 @@ static void unbind_console(struct fb_info *fb_info)
 		return;
 
 	console_lock();
-	lock_fb_info(fb_info);
 	fbcon_fb_unbind(fb_info);
-	unlock_fb_info(fb_info);
 	console_unlock();
 }
 
-- 
2.34.1


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

* [PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock()
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (11 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister Daniel Vetter
                   ` (5 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Du Cheng,
	Tetsuo Handa, Claudio Suarez, Thomas Zimmermann

This shouldn't be a problem in practice since until we've actually
taken over the console there's nothing we've registered with the
console/vt subsystem, so the exit/unbind path that check this can't
do the wrong thing. But it's confusing, so fix it by moving it a tad
later.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbcon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index cc960bf49991..4f9752ee9189 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -3227,6 +3227,9 @@ static void fbcon_register_existing_fbs(struct work_struct *work)
 
 	console_lock();
 
+	deferred_takeover = false;
+	logo_shown = FBCON_LOGO_DONTSHOW;
+
 	for_each_registered_fb(i)
 		fbcon_fb_registered(registered_fb[i]);
 
@@ -3244,8 +3247,6 @@ static int fbcon_output_notifier(struct notifier_block *nb,
 	pr_info("fbcon: Taking over console\n");
 
 	dummycon_unregister_output_notifier(&fbcon_output_nb);
-	deferred_takeover = false;
-	logo_shown = FBCON_LOGO_DONTSHOW;
 
 	/* We may get called in atomic context */
 	schedule_work(&fbcon_deferred_takeover_work);
-- 
2.34.1


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

* [PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (12 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock() Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 15/19] fbcon: Move more code into fbcon_release Daniel Vetter
                   ` (4 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Thomas Zimmermann,
	Du Cheng, Claudio Suarez, Greg Kroah-Hartman, Tetsuo Handa,
	Matthew Wilcox, Zheyu Ma, Guenter Roeck, Alex Deucher, Zhen Lei,
	Xiyu Yang

Ideally console_lock becomes an implementation detail of fbcon.c and
doesn't show up anywhere in fbmem.c. We're still pretty far from that,
but at least the register/unregister code is there now.

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

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

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


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

* [PATCH v2 15/19] fbcon: Move more code into fbcon_release
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (13 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 16/19] fbcon: untangle fbcon_exit Daniel Vetter
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Tetsuo Handa,
	Greg Kroah-Hartman, Du Cheng, Claudio Suarez

con2fb_release_oldinfo() has a bunch more kfree() calls than
fbcon_exit(), but since kfree() on NULL is harmless doing that in both
places should be ok. This is also a bit more symmetric now again with
fbcon_open also allocating the fbcon_ops structure.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Claudio Suarez <cssk@net-c.es>
---
 drivers/video/fbdev/core/fbcon.c | 33 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index abb419a091c6..685b4a9e5546 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -690,6 +690,18 @@ static void fbcon_release(struct fb_info *info)
 	unlock_fb_info(info);
 
 	module_put(info->fbops->owner);
+
+	if (info->fbcon_par) {
+		struct fbcon_ops *ops = info->fbcon_par;
+
+		fbcon_del_cursor_work(info);
+		kfree(ops->cursor_state.mask);
+		kfree(ops->cursor_data);
+		kfree(ops->cursor_src);
+		kfree(ops->fontbuffer);
+		kfree(info->fbcon_par);
+		info->fbcon_par = NULL;
+	}
 }
 
 static int fbcon_open(struct fb_info *info)
@@ -740,18 +752,10 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
 static void con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 				   struct fb_info *newinfo)
 {
-	struct fbcon_ops *ops = oldinfo->fbcon_par;
 	int ret;
 
 	fbcon_release(oldinfo);
 
-	fbcon_del_cursor_work(oldinfo);
-	kfree(ops->cursor_state.mask);
-	kfree(ops->cursor_data);
-	kfree(ops->cursor_src);
-	kfree(ops->fontbuffer);
-	kfree(oldinfo->fbcon_par);
-	oldinfo->fbcon_par = NULL;
 	/*
 	  If oldinfo and newinfo are driving the same hardware,
 	  the fb_release() method of oldinfo may attempt to
@@ -3315,19 +3319,8 @@ static void fbcon_exit(void)
 			}
 		}
 
-		if (mapped) {
-			if (info->fbcon_par) {
-				struct fbcon_ops *ops = info->fbcon_par;
-
-				fbcon_del_cursor_work(info);
-				kfree(ops->cursor_src);
-				kfree(ops->cursor_state.mask);
-				kfree(info->fbcon_par);
-				info->fbcon_par = NULL;
-			}
-
+		if (mapped)
 			fbcon_release(info);
-		}
 	}
 }
 
-- 
2.34.1


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

* [PATCH v2 16/19] fbcon: untangle fbcon_exit
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (14 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 15/19] fbcon: Move more code into fbcon_release Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 17/19] fbcon: Maintain a private array of fb_info Daniel Vetter
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Greg Kroah-Hartman,
	Claudio Suarez, Du Cheng, Tetsuo Handa

There's a bunch of confusions going on here:
- The deferred fbcon setup notifier should only be cleaned up from
  fb_console_exit(), to be symmetric with fb_console_init()
- We also need to make sure we don't race with the work, which means
  temporarily dropping the console lock (or we can deadlock)
- That also means no point in clearing deferred_takeover, we are
  unloading everything anyway.
- Finally rename fbcon_exit to fbcon_release_all and move it, since
  that's what's it doing when being called from consw->con_deinit
  through fbcon_deinit.

To answer a question from Sam just quoting my own reply:

> We loose the call to fbcon_release_all() here [in fb_console_exit()].
> We have part of the old fbcon_exit() above, but miss the release parts.

Ah yes that's the entire point of this change. The release_all in the
fbcon exit path was only needed when fbcon was a separate module
indepedent from core fb.ko. Which means it was possible to unload fbcon
while having fbdev drivers registered.

But since we've merged them that has become impossible, so by the time the
fb.ko module can be unloaded, there's guaranteed to be no fbdev drivers
left. And hence removing them is pointless.

v2: Explain the why better (Sam)

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/video/fbdev/core/fbcon.c | 63 ++++++++++++++++----------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 685b4a9e5546..944f514c77ec 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -187,7 +187,6 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
 			      int line, int count, int dy);
 static void fbcon_modechanged(struct fb_info *info);
 static void fbcon_set_all_vcs(struct fb_info *info);
-static void fbcon_exit(void);
 
 static struct device *fbcon_device;
 
@@ -1146,6 +1145,27 @@ static void fbcon_free_font(struct fbcon_display *p, bool freefont)
 
 static void set_vc_hi_font(struct vc_data *vc, bool set);
 
+static void fbcon_release_all(void)
+{
+	struct fb_info *info;
+	int i, j, mapped;
+
+	for_each_registered_fb(i) {
+		mapped = 0;
+		info = registered_fb[i];
+
+		for (j = first_fb_vc; j <= last_fb_vc; j++) {
+			if (con2fb_map[j] == i) {
+				mapped = 1;
+				con2fb_map[j] = -1;
+			}
+		}
+
+		if (mapped)
+			fbcon_release(info);
+	}
+}
+
 static void fbcon_deinit(struct vc_data *vc)
 {
 	struct fbcon_display *p = &fb_display[vc->vc_num];
@@ -1185,7 +1205,7 @@ static void fbcon_deinit(struct vc_data *vc)
 		set_vc_hi_font(vc, false);
 
 	if (!con_is_bound(&fb_con))
-		fbcon_exit();
+		fbcon_release_all();
 
 	if (vc->vc_num == logo_shown)
 		logo_shown = FBCON_LOGO_CANSHOW;
@@ -3296,34 +3316,6 @@ static void fbcon_start(void)
 #endif
 }
 
-static void fbcon_exit(void)
-{
-	struct fb_info *info;
-	int i, j, mapped;
-
-#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
-	if (deferred_takeover) {
-		dummycon_unregister_output_notifier(&fbcon_output_nb);
-		deferred_takeover = false;
-	}
-#endif
-
-	for_each_registered_fb(i) {
-		mapped = 0;
-		info = registered_fb[i];
-
-		for (j = first_fb_vc; j <= last_fb_vc; j++) {
-			if (con2fb_map[j] == i) {
-				mapped = 1;
-				con2fb_map[j] = -1;
-			}
-		}
-
-		if (mapped)
-			fbcon_release(info);
-	}
-}
-
 void __init fb_console_init(void)
 {
 	int i;
@@ -3363,10 +3355,19 @@ static void __exit fbcon_deinit_device(void)
 
 void __exit fb_console_exit(void)
 {
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+	console_lock();
+	if (deferred_takeover)
+		dummycon_unregister_output_notifier(&fbcon_output_nb);
+	console_unlock();
+
+	cancel_work_sync(&fbcon_deferred_takeover_work);
+#endif
+
 	console_lock();
 	fbcon_deinit_device();
 	device_destroy(fb_class, MKDEV(0, 0));
-	fbcon_exit();
+
 	do_unregister_con_driver(&fb_con);
 	console_unlock();
 }	
-- 
2.34.1


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

* [PATCH v2 17/19] fbcon: Maintain a private array of fb_info
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (15 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 16/19] fbcon: untangle fbcon_exit Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Daniel Vetter
  2022-02-08 21:08 ` [PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c Daniel Vetter
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Sam Ravnborg, Daniel Vetter, Daniel Vetter, Tetsuo Handa,
	Claudio Suarez, Du Cheng, Greg Kroah-Hartman

Accessing the one in fbmem.c without taking the right locks is a bad
idea. Instead maintain our own private copy, which is fully protected
by console_lock() (like everything else in fbcon.c). That copy is
serialized through fbcon_fb_registered/unregistered() calls.

Also this means we do not need to hold a full fb_info reference, which
is nice because doing so would mean a refcount loop between the
console and the fb_info. But it's also not nice since it means
console_lock() must be held absolutely everywhere. Well strictly
speaking we could still try to do some refcounting games again by
calling get_fb_info before we drop the console_lock. But things will
get tricky.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/video/fbdev/core/fbcon.c | 82 +++++++++++++++++---------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 944f514c77ec..6a7d470beec7 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -86,10 +86,6 @@
  * - fbcon state itself is protected by the console_lock, and the code does a
  *   pretty good job at making sure that lock is held everywhere it's needed.
  *
- * - access to the registered_fb array is entirely unprotected. This should use
- *   proper object lifetime handling, i.e. get/put_fb_info. This also means
- *   switching from indices to proper pointers for fb_info everywhere.
- *
  * - fbcon doesn't bother with fb_lock/unlock at all. This is buggy, since it
  *   means concurrent access to the same fbdev from both fbcon and userspace
  *   will blow up. To fix this all fbcon calls from fbmem.c need to be moved out
@@ -107,6 +103,13 @@ enum {
 
 static struct fbcon_display fb_display[MAX_NR_CONSOLES];
 
+struct fb_info *fbcon_registered_fb[FB_MAX];
+int fbcon_num_registered_fb;
+
+#define fbcon_for_each_registered_fb(i)		\
+	for (i = 0; WARN_CONSOLE_UNLOCKED(), i < FB_MAX; i++)		\
+		if (!fbcon_registered_fb[i]) {} else
+
 static signed char con2fb_map[MAX_NR_CONSOLES];
 static signed char con2fb_map_boot[MAX_NR_CONSOLES];
 
@@ -114,12 +117,7 @@ static struct fb_info *fbcon_info_from_console(int console)
 {
 	WARN_CONSOLE_UNLOCKED();
 
-	/*
-	 * Note that only con2fb_map is protected by the console lock,
-	 * registered_fb is protected by a separate mutex. This lookup can
-	 * therefore race.
-	 */
-	return registered_fb[con2fb_map[console]];
+	return fbcon_registered_fb[con2fb_map[console]];
 }
 
 static int logo_lines;
@@ -518,7 +516,7 @@ static int do_fbcon_takeover(int show_logo)
 {
 	int err, i;
 
-	if (!num_registered_fb)
+	if (!fbcon_num_registered_fb)
 		return -ENODEV;
 
 	if (!show_logo)
@@ -821,7 +819,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 {
 	struct vc_data *vc = vc_cons[unit].d;
 	int oldidx = con2fb_map[unit];
-	struct fb_info *info = registered_fb[newidx];
+	struct fb_info *info = fbcon_registered_fb[newidx];
 	struct fb_info *oldinfo = NULL;
 	int found, err = 0, show_logo;
 
@@ -839,7 +837,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 	}
 
 	if (oldidx != -1)
-		oldinfo = registered_fb[oldidx];
+		oldinfo = fbcon_registered_fb[oldidx];
 
 	found = search_fb_in_map(newidx);
 
@@ -931,13 +929,13 @@ static const char *fbcon_startup(void)
 	 *  If num_registered_fb is zero, this is a call for the dummy part.
 	 *  The frame buffer devices weren't initialized yet.
 	 */
-	if (!num_registered_fb || info_idx == -1)
+	if (!fbcon_num_registered_fb || info_idx == -1)
 		return display_desc;
 	/*
 	 * Instead of blindly using registered_fb[0], we use info_idx, set by
 	 * fbcon_fb_registered();
 	 */
-	info = registered_fb[info_idx];
+	info = fbcon_registered_fb[info_idx];
 	if (!info)
 		return NULL;
 	
@@ -1150,9 +1148,9 @@ static void fbcon_release_all(void)
 	struct fb_info *info;
 	int i, j, mapped;
 
-	for_each_registered_fb(i) {
+	fbcon_for_each_registered_fb(i) {
 		mapped = 0;
-		info = registered_fb[i];
+		info = fbcon_registered_fb[i];
 
 		for (j = first_fb_vc; j <= last_fb_vc; j++) {
 			if (con2fb_map[j] == i) {
@@ -1179,7 +1177,7 @@ static void fbcon_deinit(struct vc_data *vc)
 	if (idx == -1)
 		goto finished;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 
 	if (!info)
 		goto finished;
@@ -2098,9 +2096,9 @@ static int fbcon_switch(struct vc_data *vc)
 	 *
 	 * info->currcon = vc->vc_num;
 	 */
-	for_each_registered_fb(i) {
-		if (registered_fb[i]->fbcon_par) {
-			struct fbcon_ops *o = registered_fb[i]->fbcon_par;
+	fbcon_for_each_registered_fb(i) {
+		if (fbcon_registered_fb[i]->fbcon_par) {
+			struct fbcon_ops *o = fbcon_registered_fb[i]->fbcon_par;
 
 			o->currcon = vc->vc_num;
 		}
@@ -2745,7 +2743,7 @@ int fbcon_mode_deleted(struct fb_info *info,
 		j = con2fb_map[i];
 		if (j == -1)
 			continue;
-		fb_info = registered_fb[j];
+		fb_info = fbcon_registered_fb[j];
 		if (fb_info != info)
 			continue;
 		p = &fb_display[i];
@@ -2801,7 +2799,7 @@ void fbcon_fb_unbind(struct fb_info *info)
 				set_con2fb_map(i, new_idx, 0);
 		}
 	} else {
-		struct fb_info *info = registered_fb[idx];
+		struct fb_info *info = fbcon_registered_fb[idx];
 
 		/* This is sort of like set_con2fb_map, except it maps
 		 * the consoles to no device and then releases the
@@ -2831,6 +2829,9 @@ void fbcon_fb_unregistered(struct fb_info *info)
 
 	console_lock();
 
+	fbcon_registered_fb[info->node] = NULL;
+	fbcon_num_registered_fb--;
+
 	if (deferred_takeover) {
 		console_unlock();
 		return;
@@ -2845,7 +2846,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
 	if (idx == info_idx) {
 		info_idx = -1;
 
-		for_each_registered_fb(i) {
+		fbcon_for_each_registered_fb(i) {
 			info_idx = i;
 			break;
 		}
@@ -2861,7 +2862,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
 	if (primary_device == idx)
 		primary_device = -1;
 
-	if (!num_registered_fb)
+	if (!fbcon_num_registered_fb)
 		do_unregister_con_driver(&fb_con);
 	console_unlock();
 }
@@ -2936,6 +2937,9 @@ int fbcon_fb_registered(struct fb_info *info)
 	else
 		atomic_inc(&ignore_console_lock_warning);
 
+	fbcon_registered_fb[info->node] = info;
+	fbcon_num_registered_fb++;
+
 	idx = info->node;
 	fbcon_select_primary(info);
 
@@ -3055,9 +3059,9 @@ int fbcon_set_con2fb_map_ioctl(void __user *argp)
 		return -EINVAL;
 	if (con2fb.framebuffer >= FB_MAX)
 		return -EINVAL;
-	if (!registered_fb[con2fb.framebuffer])
+	if (!fbcon_registered_fb[con2fb.framebuffer])
 		request_module("fb%d", con2fb.framebuffer);
-	if (!registered_fb[con2fb.framebuffer]) {
+	if (!fbcon_registered_fb[con2fb.framebuffer]) {
 		return -EINVAL;
 	}
 
@@ -3124,10 +3128,10 @@ static ssize_t store_rotate(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	if (idx == -1 || registered_fb[idx] == NULL)
+	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 	rotate = simple_strtoul(buf, last, 0);
 	fbcon_rotate(info, rotate);
 err:
@@ -3146,10 +3150,10 @@ static ssize_t store_rotate_all(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	if (idx == -1 || registered_fb[idx] == NULL)
+	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 	rotate = simple_strtoul(buf, last, 0);
 	fbcon_rotate_all(info, rotate);
 err:
@@ -3166,10 +3170,10 @@ static ssize_t show_rotate(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	if (idx == -1 || registered_fb[idx] == NULL)
+	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 	rotate = fbcon_get_rotate(info);
 err:
 	console_unlock();
@@ -3186,10 +3190,10 @@ static ssize_t show_cursor_blink(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	if (idx == -1 || registered_fb[idx] == NULL)
+	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 	ops = info->fbcon_par;
 
 	if (!ops)
@@ -3212,10 +3216,10 @@ static ssize_t store_cursor_blink(struct device *device,
 	console_lock();
 	idx = con2fb_map[fg_console];
 
-	if (idx == -1 || registered_fb[idx] == NULL)
+	if (idx == -1 || fbcon_registered_fb[idx] == NULL)
 		goto err;
 
-	info = registered_fb[idx];
+	info = fbcon_registered_fb[idx];
 
 	if (!info->fbcon_par)
 		goto err;
@@ -3275,8 +3279,8 @@ static void fbcon_register_existing_fbs(struct work_struct *work)
 	deferred_takeover = false;
 	logo_shown = FBCON_LOGO_DONTSHOW;
 
-	for_each_registered_fb(i)
-		fbcon_fb_registered(registered_fb[i]);
+	fbcon_for_each_registered_fb(i)
+		fbcon_fb_registered(fbcon_registered_fb[i]);
 
 	console_unlock();
 }
-- 
2.34.1


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

* [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (16 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 17/19] fbcon: Maintain a private array of fb_info Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  2022-02-09  0:19   ` Javier Martinez Canillas
  2022-02-08 21:08 ` [PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c Daniel Vetter
  18 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Thomas Zimmermann, Zack Rusin, Javier Martinez Canillas,
	Hans de Goede, Ilya Trukhanov, Daniel Vetter, Peter Jones

This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.

With

commit 27599aacbaefcbf2af7b06b0029459bbf682000d
Author: Thomas Zimmermann <tzimmermann@suse.de>
Date:   Tue Jan 25 10:12:18 2022 +0100

    fbdev: Hot-unplug firmware fb devices on forced removal

this should be fixed properly and we can remove this somewhat hackish
check here (e.g. this won't catch drm drivers if fbdev emulation isn't
enabled).

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Zack Rusin <zackr@vmware.com>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Zack Rusin <zackr@vmware.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Ilya Trukhanov <lahvuun@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Peter Jones <pjones@redhat.com>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/video/fbdev/efifb.c    | 11 -----------
 drivers/video/fbdev/simplefb.c | 11 -----------
 2 files changed, 22 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ea42ba6445b2..edca3703b964 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -351,17 +351,6 @@ static int efifb_probe(struct platform_device *dev)
 	char *option = NULL;
 	efi_memory_desc_t md;
 
-	/*
-	 * Generic drivers must not be registered if a framebuffer exists.
-	 * If a native driver was probed, the display hardware was already
-	 * taken and attempting to use the system framebuffer is dangerous.
-	 */
-	if (num_registered_fb > 0) {
-		dev_err(&dev->dev,
-			"efifb: a framebuffer is already registered\n");
-		return -EINVAL;
-	}
-
 	if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled)
 		return -ENODEV;
 
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 94fc9c6d0411..0ef41173325a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -413,17 +413,6 @@ static int simplefb_probe(struct platform_device *pdev)
 	struct simplefb_par *par;
 	struct resource *res, *mem;
 
-	/*
-	 * Generic drivers must not be registered if a framebuffer exists.
-	 * If a native driver was probed, the display hardware was already
-	 * taken and attempting to use the system framebuffer is dangerous.
-	 */
-	if (num_registered_fb > 0) {
-		dev_err(&pdev->dev,
-			"simplefb: a framebuffer is already registered\n");
-		return -EINVAL;
-	}
-
 	if (fb_get_options("simplefb", NULL))
 		return -ENODEV;
 
-- 
2.34.1


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

* [PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c
  2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
                   ` (17 preceding siblings ...)
  2022-02-08 21:08 ` [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Daniel Vetter
@ 2022-02-08 21:08 ` Daniel Vetter
  18 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-02-08 21:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	kernel test robot, Jens Frederich, Jon Nettleton,
	Greg Kroah-Hartman, linux-staging, Daniel Vetter, Daniel Vetter,
	Helge Deller, Matthew Wilcox, Sam Ravnborg, Tetsuo Handa,
	Zhen Lei, Alex Deucher, Xiyu Yang, Zheyu Ma, Guenter Roeck

Well except when the olpc dcon fbdev driver is enabled, that thing
digs around in there in rather unfixable ways.

Cc oldc_dcon maintainers as fyi.

v2: I typoed the config name (0day)

Cc: kernel test robot <lkp@intel.com>
Cc: Jens Frederich <jfrederich@gmail.com>
Cc: Jon Nettleton <jon.nettleton@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-staging@lists.linux.dev
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Helge Deller <deller@gmx.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Cc: linux-fbdev@vger.kernel.org
Cc: Zheyu Ma <zheyuma97@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
---
 drivers/video/fbdev/core/fbmem.c | 8 ++++++--
 include/linux/fb.h               | 7 +++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 6f6f7a763969..6f0eb596a2cd 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -49,10 +49,14 @@
 static DEFINE_MUTEX(registration_lock);
 
 struct fb_info *registered_fb[FB_MAX] __read_mostly;
-EXPORT_SYMBOL(registered_fb);
-
 int num_registered_fb __read_mostly;
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
+EXPORT_SYMBOL(registered_fb);
 EXPORT_SYMBOL(num_registered_fb);
+#endif
+#define for_each_registered_fb(i)		\
+	for (i = 0; i < FB_MAX; i++)		\
+		if (!registered_fb[i]) {} else
 
 bool fb_center_logo __read_mostly;
 
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 23b19cf8bccd..afaa1474a283 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -623,16 +623,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
 extern int fb_get_options(const char *name, char **option);
 extern int fb_new_modelist(struct fb_info *info);
 
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
 extern struct fb_info *registered_fb[FB_MAX];
+
 extern int num_registered_fb;
+#endif
 extern bool fb_center_logo;
 extern int fb_logo_count;
 extern struct class *fb_class;
 
-#define for_each_registered_fb(i)		\
-	for (i = 0; i < FB_MAX; i++)		\
-		if (!registered_fb[i]) {} else
-
 static inline void lock_fb_info(struct fb_info *info)
 {
 	mutex_lock(&info->lock);
-- 
2.34.1


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

* Re: [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
  2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
@ 2022-02-08 23:06   ` Javier Martinez Canillas
  2022-02-10 11:17   ` Thomas Zimmermann
  1 sibling, 0 replies; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-02-08 23:06 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Thomas Zimmermann, Du Cheng, Tetsuo Handa,
	Intel Graphics Development, LKML, Claudio Suarez,
	Greg Kroah-Hartman, Daniel Vetter, Helge Deller

On 2/8/22 22:08, Daniel Vetter wrote:
> Avoids two forward declarations, and more importantly, matches what
> I've done in my fbcon scrolling restore patches - so I need this to
> avoid a bunch of conflicts in rebasing since we ended up merging
> Helge's series instead.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
  2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
@ 2022-02-08 23:59   ` Javier Martinez Canillas
  2022-02-10 11:37   ` Thomas Zimmermann
  2022-02-10 11:43   ` Tetsuo Handa
  2 siblings, 0 replies; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-02-08 23:59 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Thomas Zimmermann, Du Cheng, Tetsuo Handa,
	Intel Graphics Development, LKML, Claudio Suarez,
	Greg Kroah-Hartman, Daniel Vetter

Hello Daniel,

On 2/8/22 22:08, Daniel Vetter wrote:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
> 

Maybe also make it more explicit in the commit message that the delayed
work is replacing a timer that was used before for the cursor ?

> The motiviation here is that fbcon code stops using the fb_info.queue,

motivation

[snip]

>     /*
>      *    This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
>  	int  (*update_start)(struct fb_info *info);
>  	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
>  	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
> -	struct timer_list cursor_timer; /* Cursor timer */
> +	struct delayed_work cursor_work; /* Cursor timer */

A delayed_work uses a timer underneath but I wonder if the comment also
needs to be updated since technically isn't a timer anymore but deferred
work that gets re-scheduled each time on fb_flashcursor().

The patch looks good to me and makes the logic much simpler than before.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-02-08 21:08 ` [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Daniel Vetter
@ 2022-02-09  0:19   ` Javier Martinez Canillas
  2022-04-05  8:36     ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-02-09  0:19 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Thomas Zimmermann,
	Zack Rusin, Hans de Goede, Ilya Trukhanov, Daniel Vetter,
	Peter Jones

On 2/8/22 22:08, Daniel Vetter wrote:
> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
> 
> With
> 
> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
> Author: Thomas Zimmermann <tzimmermann@suse.de>
> Date:   Tue Jan 25 10:12:18 2022 +0100
> 
>     fbdev: Hot-unplug firmware fb devices on forced removal
> 
> this should be fixed properly and we can remove this somewhat hackish
> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
> enabled).
>

Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
of platform devices matched with fbdev drivers to be properly unregistered if
a DRM driver attempts to remove all the conflicting framebuffers.

But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
a FB is already registered") worked around is different. It happens when the
DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
kicking out of conflicting framebuffers already happened and these drivers
will be allowed to probe even when a DRM driver is already present.

We need a clearer way to prevent it, but can't revert fb561bf9abde until that.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 01/19] fbcon: delete a few unneeded forward decl
  2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
@ 2022-02-10 11:17   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:17 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Sam Ravnborg,
	Geert Uytterhoeven, Daniel Vetter, Helge Deller, Daniel Vetter,
	Du Cheng, Tetsuo Handa, Claudio Suarez, Greg Kroah-Hartman


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> I didn't bother with any code movement to fix the others, these just
> got a bit in the way.
> 
> v2: Rebase on top of Helge's reverts.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org> (v1)
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 2fc1b80a26ad..235eaab37d84 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -163,29 +163,14 @@ static int fbcon_cursor_noblink;
>    *  Interface used by the world
>    */
>   
> -static const char *fbcon_startup(void);
> -static void fbcon_init(struct vc_data *vc, int init);
> -static void fbcon_deinit(struct vc_data *vc);
> -static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
> -			int width);
> -static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos);
> -static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,
> -			int count, int ypos, int xpos);
>   static void fbcon_clear_margins(struct vc_data *vc, int bottom_only);
> -static void fbcon_cursor(struct vc_data *vc, int mode);
>   static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
>   			int height, int width);
> -static int fbcon_switch(struct vc_data *vc);
> -static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch);
>   static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
>   
>   /*
>    *  Internal routines
>    */
> -static __inline__ void ywrap_up(struct vc_data *vc, int count);
> -static __inline__ void ywrap_down(struct vc_data *vc, int count);
> -static __inline__ void ypan_up(struct vc_data *vc, int count);
> -static __inline__ void ypan_down(struct vc_data *vc, int count);
>   static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
>   			    int dy, int dx, int height, int width, u_int y_break);
>   static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
> @@ -194,8 +179,8 @@ static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
>   			      int line, int count, int dy);
>   static void fbcon_modechanged(struct fb_info *info);
>   static void fbcon_set_all_vcs(struct fb_info *info);
> -static void fbcon_start(void);
>   static void fbcon_exit(void);
> +
>   static struct device *fbcon_device;
>   
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions
  2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
  2022-02-08 23:06   ` Javier Martinez Canillas
@ 2022-02-10 11:17   ` Thomas Zimmermann
  1 sibling, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:17 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Helge Deller, Daniel Vetter, Du Cheng, Tetsuo Handa,
	Claudio Suarez, Greg Kroah-Hartman


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> Avoids two forward declarations, and more importantly, matches what
> I've done in my fbcon scrolling restore patches - so I need this to
> avoid a bunch of conflicts in rebasing since we ended up merging
> Helge's series instead.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 134 +++++++++++++++----------------
>   1 file changed, 65 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 235eaab37d84..e925bb608e25 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -164,15 +164,11 @@ static int fbcon_cursor_noblink;
>    */
>   
>   static void fbcon_clear_margins(struct vc_data *vc, int bottom_only);
> -static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
> -			int height, int width);
>   static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
>   
>   /*
>    *  Internal routines
>    */
> -static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
> -			    int dy, int dx, int height, int width, u_int y_break);
>   static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
>   			   int unit);
>   static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
> @@ -1667,6 +1663,71 @@ static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p,
>   	}
>   }
>   
> +static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
> +			    int dy, int dx, int height, int width, u_int y_break)
> +{
> +	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fbcon_ops *ops = info->fbcon_par;
> +	u_int b;
> +
> +	if (sy < y_break && sy + height > y_break) {
> +		b = y_break - sy;
> +		if (dy < sy) {	/* Avoid trashing self */
> +			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> +					y_break);
> +			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> +					height - b, width, y_break);
> +		} else {
> +			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> +					height - b, width, y_break);
> +			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> +					y_break);
> +		}
> +		return;
> +	}
> +
> +	if (dy < y_break && dy + height > y_break) {
> +		b = y_break - dy;
> +		if (dy < sy) {	/* Avoid trashing self */
> +			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> +					y_break);
> +			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> +					height - b, width, y_break);
> +		} else {
> +			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> +					height - b, width, y_break);
> +			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> +					y_break);
> +		}
> +		return;
> +	}
> +	ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
> +		   height, width);
> +}
> +
> +static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
> +			int height, int width)
> +{
> +	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fbcon_display *p = &fb_display[vc->vc_num];
> +
> +	if (fbcon_is_inactive(vc, info))
> +		return;
> +
> +	if (!width || !height)
> +		return;
> +
> +	/*  Split blits that cross physical y_wrap case.
> +	 *  Pathological case involves 4 blits, better to use recursive
> +	 *  code rather than unrolled case
> +	 *
> +	 *  Recursive invocations don't need to erase the cursor over and
> +	 *  over again, so we use fbcon_bmove_rec()
> +	 */
> +	fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width,
> +			p->vrows - p->yscroll);
> +}
> +
>   static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
>   		enum con_scroll dir, unsigned int count)
>   {
> @@ -1867,71 +1928,6 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
>   }
>   
>   
> -static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
> -			int height, int width)
> -{
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> -	struct fbcon_display *p = &fb_display[vc->vc_num];
> -
> -	if (fbcon_is_inactive(vc, info))
> -		return;
> -
> -	if (!width || !height)
> -		return;
> -
> -	/*  Split blits that cross physical y_wrap case.
> -	 *  Pathological case involves 4 blits, better to use recursive
> -	 *  code rather than unrolled case
> -	 *
> -	 *  Recursive invocations don't need to erase the cursor over and
> -	 *  over again, so we use fbcon_bmove_rec()
> -	 */
> -	fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width,
> -			p->vrows - p->yscroll);
> -}
> -
> -static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
> -			    int dy, int dx, int height, int width, u_int y_break)
> -{
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> -	struct fbcon_ops *ops = info->fbcon_par;
> -	u_int b;
> -
> -	if (sy < y_break && sy + height > y_break) {
> -		b = y_break - sy;
> -		if (dy < sy) {	/* Avoid trashing self */
> -			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> -					y_break);
> -			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> -					height - b, width, y_break);
> -		} else {
> -			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> -					height - b, width, y_break);
> -			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> -					y_break);
> -		}
> -		return;
> -	}
> -
> -	if (dy < y_break && dy + height > y_break) {
> -		b = y_break - dy;
> -		if (dy < sy) {	/* Avoid trashing self */
> -			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> -					y_break);
> -			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> -					height - b, width, y_break);
> -		} else {
> -			fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
> -					height - b, width, y_break);
> -			fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
> -					y_break);
> -		}
> -		return;
> -	}
> -	ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
> -		   height, width);
> -}
> -
>   static void updatescrollmode_accel(struct fbcon_display *p,
>   					struct fb_info *info,
>   					struct vc_data *vc)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup
  2022-02-08 21:08 ` [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup Daniel Vetter
@ 2022-02-10 11:18   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:18 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Sam Ravnborg,
	Daniel Vetter, Helge Deller, Daniel Vetter, Greg Kroah-Hartman,
	Tetsuo Handa, Du Cheng, Claudio Suarez


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> Half of it is protected by console_lock, but the other half is a lot
> more awkward: Registration/deregistration of fbdev are serialized, but
> we don't really clear out anything in con2fb_map and so there's
> potential for use-after free mixups.
> 
> First step is to encapsulate the lookup.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 76 ++++++++++++++++++--------------
>   1 file changed, 44 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index e925bb608e25..b75e638cb83d 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -110,6 +110,18 @@ static struct fbcon_display fb_display[MAX_NR_CONSOLES];
>   static signed char con2fb_map[MAX_NR_CONSOLES];
>   static signed char con2fb_map_boot[MAX_NR_CONSOLES];
>   
> +static struct fb_info *fbcon_info_from_console(int console)
> +{
> +	WARN_CONSOLE_UNLOCKED();
> +
> +	/*
> +	 * Note that only con2fb_map is protected by the console lock,
> +	 * registered_fb is protected by a separate mutex. This lookup can
> +	 * therefore race.
> +	 */
> +	return registered_fb[con2fb_map[console]];
> +}
> +
>   static int logo_lines;
>   /* logo_shown is an index to vc_cons when >= 0; otherwise follows FBCON_LOGO
>      enums.  */
> @@ -199,7 +211,7 @@ static void fbcon_rotate(struct fb_info *info, u32 rotate)
>   	if (!ops || ops->currcon == -1)
>   		return;
>   
> -	fb_info = registered_fb[con2fb_map[ops->currcon]];
> +	fb_info = fbcon_info_from_console(ops->currcon);
>   
>   	if (info == fb_info) {
>   		struct fbcon_display *p = &fb_display[ops->currcon];
> @@ -226,7 +238,7 @@ static void fbcon_rotate_all(struct fb_info *info, u32 rotate)
>   	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>   		vc = vc_cons[i].d;
>   		if (!vc || vc->vc_mode != KD_TEXT ||
> -		    registered_fb[con2fb_map[i]] != info)
> +		    fbcon_info_from_console(i) != info)
>   			continue;
>   
>   		p = &fb_display[vc->vc_num];
> @@ -356,7 +368,7 @@ static void fb_flashcursor(struct work_struct *work)
>   		vc = vc_cons[ops->currcon].d;
>   
>   	if (!vc || !con_is_visible(vc) ||
> - 	    registered_fb[con2fb_map[vc->vc_num]] != info ||
> +	    fbcon_info_from_console(vc->vc_num) != info ||
>   	    vc->vc_deccm != 1) {
>   		console_unlock();
>   		return;
> @@ -791,7 +803,7 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
>   	if (show_logo) {
>   		struct vc_data *fg_vc = vc_cons[fg_console].d;
>   		struct fb_info *fg_info =
> -			registered_fb[con2fb_map[fg_console]];
> +			fbcon_info_from_console(fg_console);
>   
>   		fbcon_prepare_logo(fg_vc, fg_info, fg_vc->vc_cols,
>   				   fg_vc->vc_rows, fg_vc->vc_cols,
> @@ -1014,7 +1026,7 @@ static void fbcon_init(struct vc_data *vc, int init)
>   	if (con2fb_map[vc->vc_num] == -1)
>   		con2fb_map[vc->vc_num] = info_idx;
>   
> -	info = registered_fb[con2fb_map[vc->vc_num]];
> +	info = fbcon_info_from_console(vc->vc_num);
>   
>   	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
>   		logo_shown = FBCON_LOGO_DONTSHOW;
> @@ -1231,7 +1243,7 @@ static void fbcon_deinit(struct vc_data *vc)
>   static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
>   			int width)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
> @@ -1269,7 +1281,7 @@ static void fbcon_clear(struct vc_data *vc, int sy, int sx, int height,
>   static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,
>   			int count, int ypos, int xpos)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> @@ -1289,7 +1301,7 @@ static void fbcon_putc(struct vc_data *vc, int c, int ypos, int xpos)
>   
>   static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
>   	if (!fbcon_is_inactive(vc, info))
> @@ -1298,7 +1310,7 @@ static void fbcon_clear_margins(struct vc_data *vc, int bottom_only)
>   
>   static void fbcon_cursor(struct vc_data *vc, int mode)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>    	int c = scr_readw((u16 *) vc->vc_pos);
>   
> @@ -1392,7 +1404,7 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
>   
>   static __inline__ void ywrap_up(struct vc_data *vc, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
> @@ -1411,7 +1423,7 @@ static __inline__ void ywrap_up(struct vc_data *vc, int count)
>   
>   static __inline__ void ywrap_down(struct vc_data *vc, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
> @@ -1430,7 +1442,7 @@ static __inline__ void ywrap_down(struct vc_data *vc, int count)
>   
>   static __inline__ void ypan_up(struct vc_data *vc, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> @@ -1454,7 +1466,7 @@ static __inline__ void ypan_up(struct vc_data *vc, int count)
>   
>   static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
> @@ -1478,7 +1490,7 @@ static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
>   
>   static __inline__ void ypan_down(struct vc_data *vc, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> @@ -1502,7 +1514,7 @@ static __inline__ void ypan_down(struct vc_data *vc, int count)
>   
>   static __inline__ void ypan_down_redraw(struct vc_data *vc, int t, int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
> @@ -1666,7 +1678,7 @@ static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p,
>   static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
>   			    int dy, int dx, int height, int width, u_int y_break)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	u_int b;
>   
> @@ -1708,7 +1720,7 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
>   static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
>   			int height, int width)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   
>   	if (fbcon_is_inactive(vc, info))
> @@ -1731,7 +1743,7 @@ static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
>   static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
>   		enum con_scroll dir, unsigned int count)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
>   
> @@ -1996,7 +2008,7 @@ static void updatescrollmode(struct fbcon_display *p,
>   static int fbcon_resize(struct vc_data *vc, unsigned int width,
>   			unsigned int height, unsigned int user)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	struct fb_var_screeninfo var = info->var;
> @@ -2065,7 +2077,7 @@ static int fbcon_switch(struct vc_data *vc)
>   	struct fb_var_screeninfo var;
>   	int i, ret, prev_console;
>   
> -	info = registered_fb[con2fb_map[vc->vc_num]];
> +	info = fbcon_info_from_console(vc->vc_num);
>   	ops = info->fbcon_par;
>   
>   	if (logo_shown >= 0) {
> @@ -2079,7 +2091,7 @@ static int fbcon_switch(struct vc_data *vc)
>   
>   	prev_console = ops->currcon;
>   	if (prev_console != -1)
> -		old_info = registered_fb[con2fb_map[prev_console]];
> +		old_info = fbcon_info_from_console(prev_console);
>   	/*
>   	 * FIXME: If we have multiple fbdev's loaded, we need to
>   	 * update all info->currcon.  Perhaps, we can place this
> @@ -2202,7 +2214,7 @@ static void fbcon_generic_blank(struct vc_data *vc, struct fb_info *info,
>   
>   static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
>   	if (mode_switch) {
> @@ -2244,7 +2256,7 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
>   
>   static int fbcon_debug_enter(struct vc_data *vc)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
>   	ops->save_graphics = ops->graphics;
> @@ -2257,7 +2269,7 @@ static int fbcon_debug_enter(struct vc_data *vc)
>   
>   static int fbcon_debug_leave(struct vc_data *vc)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
>   	ops->graphics = ops->save_graphics;
> @@ -2393,7 +2405,7 @@ static void set_vc_hi_font(struct vc_data *vc, bool set)
>   static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
>   			     const u8 * data, int userfont)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	struct fbcon_ops *ops = info->fbcon_par;
>   	struct fbcon_display *p = &fb_display[vc->vc_num];
>   	int resize;
> @@ -2447,7 +2459,7 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
>   static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>   			  unsigned int flags)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	unsigned charcount = font->charcount;
>   	int w = font->width;
>   	int h = font->height;
> @@ -2511,7 +2523,7 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
>   
>   static int fbcon_set_def_font(struct vc_data *vc, struct console_font *font, char *name)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	const struct font_desc *f;
>   
>   	if (!name)
> @@ -2535,7 +2547,7 @@ static struct fb_cmap palette_cmap = {
>   
>   static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table)
>   {
> -	struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> +	struct fb_info *info = fbcon_info_from_console(vc->vc_num);
>   	int i, j, k, depth;
>   	u8 val;
>   
> @@ -2651,7 +2663,7 @@ static void fbcon_modechanged(struct fb_info *info)
>   		return;
>   	vc = vc_cons[ops->currcon].d;
>   	if (vc->vc_mode != KD_TEXT ||
> -	    registered_fb[con2fb_map[ops->currcon]] != info)
> +	    fbcon_info_from_console(ops->currcon) != info)
>   		return;
>   
>   	p = &fb_display[vc->vc_num];
> @@ -2691,7 +2703,7 @@ static void fbcon_set_all_vcs(struct fb_info *info)
>   	for (i = first_fb_vc; i <= last_fb_vc; i++) {
>   		vc = vc_cons[i].d;
>   		if (!vc || vc->vc_mode != KD_TEXT ||
> -		    registered_fb[con2fb_map[i]] != info)
> +		    fbcon_info_from_console(i) != info)
>   			continue;
>   
>   		if (con_is_visible(vc)) {
> @@ -2954,7 +2966,7 @@ void fbcon_fb_blanked(struct fb_info *info, int blank)
>   
>   	vc = vc_cons[ops->currcon].d;
>   	if (vc->vc_mode != KD_TEXT ||
> -			registered_fb[con2fb_map[ops->currcon]] != info)
> +			fbcon_info_from_console(ops->currcon) != info)
>   		return;
>   
>   	if (con_is_visible(vc)) {
> @@ -2974,7 +2986,7 @@ void fbcon_new_modelist(struct fb_info *info)
>   	const struct fb_videomode *mode;
>   
>   	for (i = first_fb_vc; i <= last_fb_vc; i++) {
> -		if (registered_fb[con2fb_map[i]] != info)
> +		if (fbcon_info_from_console(i) != info)
>   			continue;
>   		if (!fb_display[i].mode)
>   			continue;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 04/19] fbcon: delete delayed loading code
  2022-02-08 21:08 ` [PATCH v2 04/19] fbcon: delete delayed loading code Daniel Vetter
@ 2022-02-10 11:20   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:20 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Du Cheng, Tetsuo Handa, Intel Graphics Development,
	LKML, Claudio Suarez, Greg Kroah-Hartman, Daniel Vetter,
	Sam Ravnborg, Helge Deller


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> Before
> 
> commit 6104c37094e729f3d4ce65797002112735d49cd1
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Aug 1 17:32:07 2017 +0200
> 
>      fbcon: Make fbcon a built-time depency for fbdev
> 
> it was possible to load fbcon and fbdev drivers in any order, which
> means that fbcon init had to handle the case where fbdev drivers where
> already registered.
> 
> This is no longer possible, hence delete that code.
> 
> Note that the exit case is a bit more complex and will be done in a
> separate patch.
> 
> Since I had to audit the entire fbcon load code I also spotted a wrong
> function name in a comment in fbcon_startup(), which this patch also
> fixes.
> 
> v2: Explain why we also fix the comment (Sam)
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Du Cheng <ducheng2@gmail.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index b75e638cb83d..83f0223f5333 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -944,7 +944,7 @@ static const char *fbcon_startup(void)
>   		return display_desc;
>   	/*
>   	 * Instead of blindly using registered_fb[0], we use info_idx, set by
> -	 * fb_console_init();
> +	 * fbcon_fb_registered();
>   	 */
>   	info = registered_fb[info_idx];
>   	if (!info)
> @@ -3299,17 +3299,6 @@ static void fbcon_start(void)
>   		return;
>   	}
>   #endif
> -
> -	if (num_registered_fb) {
> -		int i;
> -
> -		for_each_registered_fb(i) {
> -			info_idx = i;
> -			break;
> -		}
> -
> -		do_fbcon_takeover(0);
> -	}
>   }
>   
>   static void fbcon_exit(void)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 05/19] fbdev/sysfs: Fix locking
  2022-02-08 21:08 ` [PATCH v2 05/19] fbdev/sysfs: Fix locking Daniel Vetter
@ 2022-02-10 11:22   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:22 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Intel Graphics Development, LKML, Qing Wang,
	Daniel Vetter, Sam Ravnborg, Helge Deller


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> fb_set_var requires we hold the fb_info lock. Or at least this now
> matches what the ioctl does ...
> 
> Note that ps3fb and sh_mobile_lcdcfb are busted in different ways here,
> but I will not fix them up.
> 
> Also in practice this isn't a big deal, because really variable fbdev
> state is actually protected by console_lock (because fbcon just
> doesn't bother with lock_fb_info() at all), and lock_fb_info
> protecting anything is really just a neat lie. But that's a much
> bigger fish to fry.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Qing Wang <wangqing@vivo.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbsysfs.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index 26892940c213..8c1ee9ecec3d 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -91,9 +91,11 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
>   
>   	var->activate |= FB_ACTIVATE_FORCE;
>   	console_lock();
> +	lock_fb_info(fb_info);
>   	err = fb_set_var(fb_info, var);
>   	if (!err)
>   		fbcon_update_vcs(fb_info, var->activate & FB_ACTIVATE_ALL);
> +	unlock_fb_info(fb_info);
>   	console_unlock();
>   	if (err)
>   		return err;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
  2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
  2022-02-08 23:59   ` Javier Martinez Canillas
@ 2022-02-10 11:37   ` Thomas Zimmermann
  2022-02-10 11:43   ` Tetsuo Handa
  2 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:37 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Daniel Vetter, Claudio Suarez, Du Cheng, Greg Kroah-Hartman,
	Tetsuo Handa


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> Allows us to delete a bunch of hand-rolled stuff. Also to simplify the
> code we initialize the cursor_work completely when we allocate the
> fbcon_ops structure, instead of trying to cope with console
> re-initialization.
> 
> The motiviation here is that fbcon code stops using the fb_info.queue,
> which helps with locking issues around cleanup and all that in a later
> patch.
> 
> Also note that this allows us to ditch the hand-rolled work cleanup in
> fbcon_exit - we already call fbcon_del_cursor_timer, which takes care
> of everything. Plus this was racy anyway.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>   drivers/video/fbdev/core/fbcon.c | 85 +++++++++++++-------------------
>   drivers/video/fbdev/core/fbcon.h |  4 +-
>   2 files changed, 35 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 83f0223f5333..a368ed602e2e 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -350,8 +350,8 @@ static int get_color(struct vc_data *vc, struct fb_info *info,
>   
>   static void fb_flashcursor(struct work_struct *work)
>   {
> -	struct fb_info *info = container_of(work, struct fb_info, queue);
> -	struct fbcon_ops *ops = info->fbcon_par;
> +	struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work);
> +	struct fb_info *info;
>   	struct vc_data *vc = NULL;
>   	int c;
>   	int mode;
> @@ -364,7 +364,10 @@ static void fb_flashcursor(struct work_struct *work)
>   	if (ret == 0)
>   		return;
>   
> -	if (ops && ops->currcon != -1)
> +	/* protected by console_lock */
> +	info = ops->info;
> +
> +	if (ops->currcon != -1)
>   		vc = vc_cons[ops->currcon].d;
>   
>   	if (!vc || !con_is_visible(vc) ||
> @@ -380,42 +383,25 @@ static void fb_flashcursor(struct work_struct *work)
>   	ops->cursor(vc, info, mode, get_color(vc, info, c, 1),
>   		    get_color(vc, info, c, 0));
>   	console_unlock();
> -}
>   
> -static void cursor_timer_handler(struct timer_list *t)
> -{
> -	struct fbcon_ops *ops = from_timer(ops, t, cursor_timer);
> -	struct fb_info *info = ops->info;
> -
> -	queue_work(system_power_efficient_wq, &info->queue);
> -	mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
> +	queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
> +			   ops->cur_blink_jiffies);
>   }
>   
> -static void fbcon_add_cursor_timer(struct fb_info *info)
> +static void fbcon_add_cursor_work(struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	if ((!info->queue.func || info->queue.func == fb_flashcursor) &&
> -	    !(ops->flags & FBCON_FLAGS_CURSOR_TIMER) &&
> -	    !fbcon_cursor_noblink) {
> -		if (!info->queue.func)
> -			INIT_WORK(&info->queue, fb_flashcursor);
> -
> -		timer_setup(&ops->cursor_timer, cursor_timer_handler, 0);
> -		mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
> -		ops->flags |= FBCON_FLAGS_CURSOR_TIMER;
> -	}
> +	if (!fbcon_cursor_noblink)
> +		queue_delayed_work(system_power_efficient_wq, &ops->cursor_work,
> +				   ops->cur_blink_jiffies);
>   }
>   
> -static void fbcon_del_cursor_timer(struct fb_info *info)
> +static void fbcon_del_cursor_work(struct fb_info *info)
>   {
>   	struct fbcon_ops *ops = info->fbcon_par;
>   
> -	if (info->queue.func == fb_flashcursor &&
> -	    ops->flags & FBCON_FLAGS_CURSOR_TIMER) {
> -		del_timer_sync(&ops->cursor_timer);
> -		ops->flags &= ~FBCON_FLAGS_CURSOR_TIMER;
> -	}
> +	cancel_delayed_work_sync(&ops->cursor_work);
>   }
>   
>   #ifndef MODULE
> @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
>   		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
>   		if (!ops)
>   			err = -ENOMEM;
> +
> +		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);

There's similar code in fbcon_startup() when there should be a single 
init function for fbcon_ops. Maybe something for later.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

>   	}
>   
>   	if (!err) {
> @@ -751,7 +739,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
>   	}
>   
>   	if (!err) {
> -		fbcon_del_cursor_timer(oldinfo);
> +		fbcon_del_cursor_work(oldinfo);
>   		kfree(ops->cursor_state.mask);
>   		kfree(ops->cursor_data);
>   		kfree(ops->cursor_src);
> @@ -867,7 +855,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   				 logo_shown != FBCON_LOGO_DONTSHOW);
>   
>   		if (!found)
> -			fbcon_add_cursor_timer(info);
> +			fbcon_add_cursor_work(info);
>   		con2fb_map_boot[unit] = newidx;
>   		con2fb_init_display(vc, info, unit, show_logo);
>   	}
> @@ -964,6 +952,8 @@ static const char *fbcon_startup(void)
>   		return NULL;
>   	}
>   
> +	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
> +
>   	ops->currcon = -1;
>   	ops->graphics = 1;
>   	ops->cur_rotate = -1;
> @@ -1006,7 +996,7 @@ static const char *fbcon_startup(void)
>   		 info->var.yres,
>   		 info->var.bits_per_pixel);
>   
> -	fbcon_add_cursor_timer(info);
> +	fbcon_add_cursor_work(info);
>   	return display_desc;
>   }
>   
> @@ -1194,7 +1184,7 @@ static void fbcon_deinit(struct vc_data *vc)
>   		goto finished;
>   
>   	if (con_is_visible(vc))
> -		fbcon_del_cursor_timer(info);
> +		fbcon_del_cursor_work(info);
>   
>   	ops->flags &= ~FBCON_FLAGS_INIT;
>   finished:
> @@ -1320,9 +1310,9 @@ static void fbcon_cursor(struct vc_data *vc, int mode)
>   		return;
>   
>   	if (vc->vc_cursor_type & CUR_SW)
> -		fbcon_del_cursor_timer(info);
> +		fbcon_del_cursor_work(info);
>   	else
> -		fbcon_add_cursor_timer(info);
> +		fbcon_add_cursor_work(info);
>   
>   	ops->cursor_flash = (mode == CM_ERASE) ? 0 : 1;
>   
> @@ -2132,14 +2122,14 @@ static int fbcon_switch(struct vc_data *vc)
>   		}
>   
>   		if (old_info != info)
> -			fbcon_del_cursor_timer(old_info);
> +			fbcon_del_cursor_work(old_info);
>   	}
>   
>   	if (fbcon_is_inactive(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
> -		fbcon_del_cursor_timer(info);
> +		fbcon_del_cursor_work(info);
>   	else
> -		fbcon_add_cursor_timer(info);
> +		fbcon_add_cursor_work(info);
>   
>   	set_blitting_type(vc, info);
>   	ops->cursor_reset = 1;
> @@ -2247,9 +2237,9 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
>   
>   	if (mode_switch || fbcon_is_inactive(vc, info) ||
>   	    ops->blank_state != FB_BLANK_UNBLANK)
> -		fbcon_del_cursor_timer(info);
> +		fbcon_del_cursor_work(info);
>   	else
> -		fbcon_add_cursor_timer(info);
> +		fbcon_add_cursor_work(info);
>   
>   	return 0;
>   }
> @@ -3181,7 +3171,7 @@ static ssize_t show_cursor_blink(struct device *device,
>   	if (!ops)
>   		goto err;
>   
> -	blink = (ops->flags & FBCON_FLAGS_CURSOR_TIMER) ? 1 : 0;
> +	blink = delayed_work_pending(&ops->cursor_work);
>   err:
>   	console_unlock();
>   	return snprintf(buf, PAGE_SIZE, "%d\n", blink);
> @@ -3210,10 +3200,10 @@ static ssize_t store_cursor_blink(struct device *device,
>   
>   	if (blink) {
>   		fbcon_cursor_noblink = 0;
> -		fbcon_add_cursor_timer(info);
> +		fbcon_add_cursor_work(info);
>   	} else {
>   		fbcon_cursor_noblink = 1;
> -		fbcon_del_cursor_timer(info);
> +		fbcon_del_cursor_work(info);
>   	}
>   
>   err:
> @@ -3314,15 +3304,9 @@ static void fbcon_exit(void)
>   #endif
>   
>   	for_each_registered_fb(i) {
> -		int pending = 0;
> -
>   		mapped = 0;
>   		info = registered_fb[i];
>   
> -		if (info->queue.func)
> -			pending = cancel_work_sync(&info->queue);
> -		pr_debug("fbcon: %s pending work\n", (pending ? "canceled" : "no"));
> -
>   		for (j = first_fb_vc; j <= last_fb_vc; j++) {
>   			if (con2fb_map[j] == i) {
>   				mapped = 1;
> @@ -3338,15 +3322,12 @@ static void fbcon_exit(void)
>   			if (info->fbcon_par) {
>   				struct fbcon_ops *ops = info->fbcon_par;
>   
> -				fbcon_del_cursor_timer(info);
> +				fbcon_del_cursor_work(info);
>   				kfree(ops->cursor_src);
>   				kfree(ops->cursor_state.mask);
>   				kfree(info->fbcon_par);
>   				info->fbcon_par = NULL;
>   			}
> -
> -			if (info->queue.func == fb_flashcursor)
> -				info->queue.func = NULL;
>   		}
>   	}
>   }
> diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
> index 969d41ecede5..6708ca0048aa 100644
> --- a/drivers/video/fbdev/core/fbcon.h
> +++ b/drivers/video/fbdev/core/fbcon.h
> @@ -14,11 +14,11 @@
>   #include <linux/types.h>
>   #include <linux/vt_buffer.h>
>   #include <linux/vt_kern.h>
> +#include <linux/workqueue.h>
>   
>   #include <asm/io.h>
>   
>   #define FBCON_FLAGS_INIT         1
> -#define FBCON_FLAGS_CURSOR_TIMER 2
>   
>      /*
>       *    This is the interface between the low-level console driver and the
> @@ -68,7 +68,7 @@ struct fbcon_ops {
>   	int  (*update_start)(struct fb_info *info);
>   	int  (*rotate_font)(struct fb_info *info, struct vc_data *vc);
>   	struct fb_var_screeninfo var;  /* copy of the current fb_var_screeninfo */
> -	struct timer_list cursor_timer; /* Cursor timer */
> +	struct delayed_work cursor_work; /* Cursor timer */
>   	struct fb_cursor cursor_state;
>   	struct fbcon_display *p;
>   	struct fb_info *info;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 08/19] fb: Delete fb_info->queue
  2022-02-08 21:08 ` [PATCH v2 08/19] fb: Delete fb_info->queue Daniel Vetter
@ 2022-02-10 11:38   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:38 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Intel Graphics Development, LKML, Daniel Vetter,
	Sam Ravnborg, Helge Deller


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> It was only used by fbcon, and that now switched to its own,
> private work.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   include/linux/fb.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 3d7306c9a706..23b19cf8bccd 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -449,7 +449,6 @@ struct fb_info {
>   	struct fb_var_screeninfo var;	/* Current var */
>   	struct fb_fix_screeninfo fix;	/* Current fix */
>   	struct fb_monspecs monspecs;	/* Current Monitor specs */
> -	struct work_struct queue;	/* Framebuffer event queue */
>   	struct fb_pixmap pixmap;	/* Image hardware mapper */
>   	struct fb_pixmap sprite;	/* Cursor hardware mapper */
>   	struct fb_cmap cmap;		/* Current cmap */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
  2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
  2022-02-08 23:59   ` Javier Martinez Canillas
  2022-02-10 11:37   ` Thomas Zimmermann
@ 2022-02-10 11:43   ` Tetsuo Handa
  2022-04-05 20:54     ` Daniel Vetter
  2 siblings, 1 reply; 49+ messages in thread
From: Tetsuo Handa @ 2022-02-10 11:43 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Daniel Vetter,
	Daniel Vetter, Claudio Suarez, Du Cheng, Thomas Zimmermann,
	Greg Kroah-Hartman

On 2022/02/09 6:08, Daniel Vetter wrote:
> @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
>  		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
>  		if (!ops)
>  			err = -ENOMEM;
> +
> +		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
>  	}
>  
>  	if (!err) {

Memory allocation fault injection will hit NULL pointer dereference.

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

* Re: [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers
  2022-02-08 21:08 ` [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Daniel Vetter
@ 2022-02-10 11:46   ` Thomas Zimmermann
  2022-04-05  8:45     ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 11:46 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-fbdev, Du Cheng, Tetsuo Handa, Intel Graphics Development,
	LKML, Claudio Suarez, Greg Kroah-Hartman, Daniel Vetter,
	Sam Ravnborg


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

Hi

Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> There's two minor behaviour changes in here:
> - in error paths we now consistently call fb_ops->fb_release
> - fb_release really can't fail (fbmem.c ignores it too) and there's no
>    reasonable cleanup we can do anyway.
> 
> Note that everything in fbcon.c is protected by the big console_lock()
> lock (especially all the global variables), so the minor changes in
> ordering of setup/cleanup do not matter.
> 
> v2: Explain a bit better why this is all correct (Sam)
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Du Cheng <ducheng2@gmail.com>
> ---
>   drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++----------------
>   1 file changed, 53 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 058e885d24f6..3e1a3e7bf527 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
>   
>   #endif /* CONFIG_MISC_TILEBLITTING */
>   
> +static int fbcon_open(struct fb_info *info)
> +{
> +	if (!try_module_get(info->fbops->owner))
> +		return -ENODEV;
> +
> +	if (info->fbops->fb_open &&
> +	    info->fbops->fb_open(info, 0)) {
> +		module_put(info->fbops->owner);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fbcon_release(struct fb_info *info)
> +{
> +	if (info->fbops->fb_release)
> +		info->fbops->fb_release(info, 0);
> +
> +	module_put(info->fbops->owner);
> +}
>   
>   static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
>   				  int unit, int oldidx)
>   {
>   	struct fbcon_ops *ops = NULL;
> -	int err = 0;
> -
> -	if (!try_module_get(info->fbops->owner))
> -		err = -ENODEV;
> +	int err;
>   
> -	if (!err && info->fbops->fb_open &&
> -	    info->fbops->fb_open(info, 0))
> -		err = -ENODEV;
> +	err = fbcon_open(info);
> +	if (err)
> +		return err;
>   
>   	if (!err) {
>   		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
>   
>   	if (err) {
>   		con2fb_map[unit] = oldidx;
> -		module_put(info->fbops->owner);
> +		fbcon_release(info);
>   	}
>   
>   	return err;
> @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
>   				  int oldidx, int found)
>   {
>   	struct fbcon_ops *ops = oldinfo->fbcon_par;
> -	int err = 0, ret;
> +	int ret;
>   
> -	if (oldinfo->fbops->fb_release &&
> -	    oldinfo->fbops->fb_release(oldinfo, 0)) {
> -		con2fb_map[unit] = oldidx;

We don't need oldidx any longer?

There's some logic wrt to the parameter 'found' here and in 
set_con2fb_map() that appears to be relevant.

Best regards
Thomas


> -		if (!found && newinfo->fbops->fb_release)
> -			newinfo->fbops->fb_release(newinfo, 0);
> -		if (!found)
> -			module_put(newinfo->fbops->owner);
> -		err = -ENODEV;
> -	}
> +	fbcon_release(oldinfo);
>   
> -	if (!err) {
> -		fbcon_del_cursor_work(oldinfo);
> -		kfree(ops->cursor_state.mask);
> -		kfree(ops->cursor_data);
> -		kfree(ops->cursor_src);
> -		kfree(ops->fontbuffer);
> -		kfree(oldinfo->fbcon_par);
> -		oldinfo->fbcon_par = NULL;
> -		module_put(oldinfo->fbops->owner);
> -		/*
> -		  If oldinfo and newinfo are driving the same hardware,
> -		  the fb_release() method of oldinfo may attempt to
> -		  restore the hardware state.  This will leave the
> -		  newinfo in an undefined state. Thus, a call to
> -		  fb_set_par() may be needed for the newinfo.
> -		*/
> -		if (newinfo && newinfo->fbops->fb_set_par) {
> -			ret = newinfo->fbops->fb_set_par(newinfo);
> +	fbcon_del_cursor_work(oldinfo);
> +	kfree(ops->cursor_state.mask);
> +	kfree(ops->cursor_data);
> +	kfree(ops->cursor_src);
> +	kfree(ops->fontbuffer);
> +	kfree(oldinfo->fbcon_par);
> +	oldinfo->fbcon_par = NULL;
> +	/*
> +	  If oldinfo and newinfo are driving the same hardware,
> +	  the fb_release() method of oldinfo may attempt to
> +	  restore the hardware state.  This will leave the
> +	  newinfo in an undefined state. Thus, a call to
> +	  fb_set_par() may be needed for the newinfo.
> +	*/
> +	if (newinfo && newinfo->fbops->fb_set_par) {
> +		ret = newinfo->fbops->fb_set_par(newinfo);
>   
> -			if (ret)
> -				printk(KERN_ERR "con2fb_release_oldinfo: "
> -					"detected unhandled fb_set_par error, "
> -					"error code %d\n", ret);
> -		}
> +		if (ret)
> +			printk(KERN_ERR "con2fb_release_oldinfo: "
> +				"detected unhandled fb_set_par error, "
> +				"error code %d\n", ret);
>   	}
>   
> -	return err;
> +	return 0;
>   }
>   
>   static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
> @@ -919,7 +926,6 @@ static const char *fbcon_startup(void)
>   	struct fbcon_display *p = &fb_display[fg_console];
>   	struct vc_data *vc = vc_cons[fg_console].d;
>   	const struct font_desc *font = NULL;
> -	struct module *owner;
>   	struct fb_info *info = NULL;
>   	struct fbcon_ops *ops;
>   	int rows, cols;
> @@ -938,17 +944,12 @@ static const char *fbcon_startup(void)
>   	if (!info)
>   		return NULL;
>   	
> -	owner = info->fbops->owner;
> -	if (!try_module_get(owner))
> +	if (fbcon_open(info))
>   		return NULL;
> -	if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) {
> -		module_put(owner);
> -		return NULL;
> -	}
>   
>   	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
>   	if (!ops) {
> -		module_put(owner);
> +		fbcon_release(info);
>   		return NULL;
>   	}
>   
> @@ -3314,10 +3315,6 @@ static void fbcon_exit(void)
>   		}
>   
>   		if (mapped) {
> -			if (info->fbops->fb_release)
> -				info->fbops->fb_release(info, 0);
> -			module_put(info->fbops->owner);
> -
>   			if (info->fbcon_par) {
>   				struct fbcon_ops *ops = info->fbcon_par;
>   
> @@ -3327,6 +3324,8 @@ static void fbcon_exit(void)
>   				kfree(info->fbcon_par);
>   				info->fbcon_par = NULL;
>   			}
> +
> +			fbcon_release(info);
>   		}
>   	}
>   }

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo
  2022-02-08 21:08 ` [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo Daniel Vetter
@ 2022-02-10 14:14   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 14:14 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Sam Ravnborg,
	Daniel Vetter, Daniel Vetter, Greg Kroah-Hartman, Claudio Suarez,
	Du Cheng, Tetsuo Handa


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> It doesn't ever fail anymore.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/core/fbcon.c | 37 +++++++++++---------------------
>   1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 3e1a3e7bf527..a60891005d44 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -739,9 +739,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
>   	return err;
>   }
>   
> -static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
> -				  struct fb_info *newinfo, int unit,
> -				  int oldidx, int found)
> +static void con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
> +				   struct fb_info *newinfo)
>   {
>   	struct fbcon_ops *ops = oldinfo->fbcon_par;
>   	int ret;
> @@ -770,8 +769,6 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
>   				"detected unhandled fb_set_par error, "
>   				"error code %d\n", ret);
>   	}
> -
> -	return 0;
>   }
>   
>   static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
> @@ -825,7 +822,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   	int oldidx = con2fb_map[unit];
>   	struct fb_info *info = registered_fb[newidx];
>   	struct fb_info *oldinfo = NULL;
> -	int found, err = 0;
> +	int found, err = 0, show_logo;
>   
>   	WARN_CONSOLE_UNLOCKED();
>   
> @@ -854,18 +851,15 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   	 * fbcon should release it.
>   	 */
>   	if (!err && oldinfo && !search_fb_in_map(oldidx))
> -		err = con2fb_release_oldinfo(vc, oldinfo, info, unit, oldidx,
> -					     found);
> +		con2fb_release_oldinfo(vc, oldinfo, info);
>   
> -	if (!err) {
> -		int show_logo = (fg_console == 0 && !user &&
> -				 logo_shown != FBCON_LOGO_DONTSHOW);
> +	show_logo = (fg_console == 0 && !user &&
> +			 logo_shown != FBCON_LOGO_DONTSHOW);
>   
> -		if (!found)
> -			fbcon_add_cursor_work(info);
> -		con2fb_map_boot[unit] = newidx;
> -		con2fb_init_display(vc, info, unit, show_logo);
> -	}
> +	if (!found)
> +		fbcon_add_cursor_work(info);
> +	con2fb_map_boot[unit] = newidx;
> +	con2fb_init_display(vc, info, unit, show_logo);
>   
>   	if (!search_fb_in_map(info_idx))
>   		info_idx = newidx;
> @@ -2769,7 +2763,7 @@ static inline void fbcon_unbind(void) {}
>   /* called with console_lock held */
>   void fbcon_fb_unbind(struct fb_info *info)
>   {
> -	int i, new_idx = -1, ret = 0;
> +	int i, new_idx = -1;
>   	int idx = info->node;
>   
>   	WARN_CONSOLE_UNLOCKED();
> @@ -2803,13 +2797,8 @@ void fbcon_fb_unbind(struct fb_info *info)
>   			if (con2fb_map[i] == idx) {
>   				con2fb_map[i] = -1;
>   				if (!search_fb_in_map(idx)) {
> -					ret = con2fb_release_oldinfo(vc_cons[i].d,
> -								     info, NULL, i,
> -								     idx, 0);
> -					if (ret) {
> -						con2fb_map[i] = idx;
> -						return;
> -					}
> +					con2fb_release_oldinfo(vc_cons[i].d,
> +							       info, NULL);
>   				}
>   			}
>   		}

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 11/19] fbcon: move more common code into fb_open()
  2022-02-08 21:08 ` [PATCH v2 11/19] fbcon: move more common code into fb_open() Daniel Vetter
@ 2022-02-10 14:16   ` Thomas Zimmermann
  0 siblings, 0 replies; 49+ messages in thread
From: Thomas Zimmermann @ 2022-02-10 14:16 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, linux-fbdev, LKML, Sam Ravnborg,
	kernel test robot, Daniel Vetter, Daniel Vetter,
	Greg Kroah-Hartman, Tetsuo Handa, Claudio Suarez, Du Cheng


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



Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> No idea why con2fb_acquire_newinfo() initializes much less than
> fbcon_startup(), but so be it. From a quick look most of the
> un-initialized stuff should be fairly harmless, but who knows.
> 
> Note that the error handling for the con2fb_acquire_newinfo() failure
> case was very strange: Callers updated con2fb_map to the new value
> before calling this function, but upon error con2fb_acquire_newinfo
> reset it to the old value. Since I removed the call to fbcon_release
> anyway that strange error path was sticking out like a sore thumb,
> hence I removed it. Which also allows us to remove the oldidx
> parameter from that function.
> 
> v2: Explain what's going on with oldidx and error paths (Sam)
> 
> v3: Drop unused variable (0day)
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org> (v2)
> Cc: kernel test robot <lkp@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Du Cheng <ducheng2@gmail.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

That's the init function I was looking for, I guess.

> ---
>   drivers/video/fbdev/core/fbcon.c | 75 +++++++++++++-------------------
>   1 file changed, 30 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index a60891005d44..f0213a0e3870 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -682,8 +682,18 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
>   
>   #endif /* CONFIG_MISC_TILEBLITTING */
>   
> +static void fbcon_release(struct fb_info *info)
> +{
> +	if (info->fbops->fb_release)
> +		info->fbops->fb_release(info, 0);
> +
> +	module_put(info->fbops->owner);
> +}
> +
>   static int fbcon_open(struct fb_info *info)
>   {
> +	struct fbcon_ops *ops;
> +
>   	if (!try_module_get(info->fbops->owner))
>   		return -ENODEV;
>   
> @@ -693,48 +703,31 @@ static int fbcon_open(struct fb_info *info)
>   		return -ENODEV;
>   	}
>   
> -	return 0;
> -}
> +	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> +	if (!ops) {
> +		fbcon_release(info);
> +		return -ENOMEM;
> +	}
>   
> -static void fbcon_release(struct fb_info *info)
> -{
> -	if (info->fbops->fb_release)
> -		info->fbops->fb_release(info, 0);
> +	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
> +	ops->info = info;
> +	info->fbcon_par = ops;
> +	ops->cur_blink_jiffies = HZ / 5;
>   
> -	module_put(info->fbops->owner);
> +	return 0;
>   }
>   
>   static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
> -				  int unit, int oldidx)
> +				  int unit)
>   {
> -	struct fbcon_ops *ops = NULL;
>   	int err;
>   
>   	err = fbcon_open(info);
>   	if (err)
>   		return err;
>   
> -	if (!err) {
> -		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> -		if (!ops)
> -			err = -ENOMEM;
> -
> -		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
> -	}
> -
> -	if (!err) {
> -		ops->cur_blink_jiffies = HZ / 5;
> -		ops->info = info;
> -		info->fbcon_par = ops;
> -
> -		if (vc)
> -			set_blitting_type(vc, info);
> -	}
> -
> -	if (err) {
> -		con2fb_map[unit] = oldidx;
> -		fbcon_release(info);
> -	}
> +	if (vc)
> +		set_blitting_type(vc, info);
>   
>   	return err;
>   }
> @@ -842,9 +835,11 @@ static int set_con2fb_map(int unit, int newidx, int user)
>   
>   	found = search_fb_in_map(newidx);
>   
> -	con2fb_map[unit] = newidx;
> -	if (!err && !found)
> -		err = con2fb_acquire_newinfo(vc, info, unit, oldidx);
> +	if (!err && !found) {
> +		err = con2fb_acquire_newinfo(vc, info, unit);
> +		if (!err)
> +			con2fb_map[unit] = newidx;
> +	}
>   
>   	/*
>   	 * If old fb is not mapped to any of the consoles,
> @@ -941,20 +936,10 @@ static const char *fbcon_startup(void)
>   	if (fbcon_open(info))
>   		return NULL;
>   
> -	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> -	if (!ops) {
> -		fbcon_release(info);
> -		return NULL;
> -	}
> -
> -	INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
> -
> +	ops = info->fbcon_par;
>   	ops->currcon = -1;
>   	ops->graphics = 1;
>   	ops->cur_rotate = -1;
> -	ops->cur_blink_jiffies = HZ / 5;
> -	ops->info = info;
> -	info->fbcon_par = ops;
>   
>   	p->con_rotate = initial_rotation;
>   	if (p->con_rotate == -1)
> @@ -1024,7 +1009,7 @@ static void fbcon_init(struct vc_data *vc, int init)
>   		return;
>   
>   	if (!info->fbcon_par)
> -		con2fb_acquire_newinfo(vc, info, vc->vc_num, -1);
> +		con2fb_acquire_newinfo(vc, info, vc->vc_num);
>   
>   	/* If we are not the first console on this
>   	   fb, copy the font from that console */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-02-09  0:19   ` Javier Martinez Canillas
@ 2022-04-05  8:36     ` Daniel Vetter
  2022-04-05  8:40       ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05  8:36 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Daniel Vetter, DRI Development, Intel Graphics Development,
	linux-fbdev, LKML, Thomas Zimmermann, Zack Rusin, Hans de Goede,
	Ilya Trukhanov, Daniel Vetter, Peter Jones

On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
> On 2/8/22 22:08, Daniel Vetter wrote:
> > This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
> > 
> > With
> > 
> > commit 27599aacbaefcbf2af7b06b0029459bbf682000d
> > Author: Thomas Zimmermann <tzimmermann@suse.de>
> > Date:   Tue Jan 25 10:12:18 2022 +0100
> > 
> >     fbdev: Hot-unplug firmware fb devices on forced removal
> > 
> > this should be fixed properly and we can remove this somewhat hackish
> > check here (e.g. this won't catch drm drivers if fbdev emulation isn't
> > enabled).
> >
> 
> Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
> of platform devices matched with fbdev drivers to be properly unregistered if
> a DRM driver attempts to remove all the conflicting framebuffers.
> 
> But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
> a FB is already registered") worked around is different. It happens when the
> DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
> kicking out of conflicting framebuffers already happened and these drivers
> will be allowed to probe even when a DRM driver is already present.
> 
> We need a clearer way to prevent it, but can't revert fb561bf9abde until that.

Yeah that entire area is a mess still, ideally we'd have something else
creating the platform devices, and efifb/offb and all these would just
bind against them.

Hm one idea that just crossed my mind: Could we have a flag in fb_info for
fw drivers, and check this in framebuffer_register? Then at least all the
logic would be in the fbdev core.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05  8:36     ` Daniel Vetter
@ 2022-04-05  8:40       ` Daniel Vetter
  2022-04-05  9:19         ` Javier Martinez Canillas
  0 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05  8:40 UTC (permalink / raw)
  To: Javier Martinez Canillas, DRI Development,
	Intel Graphics Development, linux-fbdev, LKML, Thomas Zimmermann,
	Zack Rusin, Hans de Goede, Ilya Trukhanov, Daniel Vetter,
	Peter Jones

On Tue, Apr 05, 2022 at 10:36:35AM +0200, Daniel Vetter wrote:
> On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
> > On 2/8/22 22:08, Daniel Vetter wrote:
> > > This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
> > > 
> > > With
> > > 
> > > commit 27599aacbaefcbf2af7b06b0029459bbf682000d
> > > Author: Thomas Zimmermann <tzimmermann@suse.de>
> > > Date:   Tue Jan 25 10:12:18 2022 +0100
> > > 
> > >     fbdev: Hot-unplug firmware fb devices on forced removal
> > > 
> > > this should be fixed properly and we can remove this somewhat hackish
> > > check here (e.g. this won't catch drm drivers if fbdev emulation isn't
> > > enabled).
> > >
> > 
> > Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
> > of platform devices matched with fbdev drivers to be properly unregistered if
> > a DRM driver attempts to remove all the conflicting framebuffers.
> > 
> > But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
> > a FB is already registered") worked around is different. It happens when the
> > DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
> > kicking out of conflicting framebuffers already happened and these drivers
> > will be allowed to probe even when a DRM driver is already present.
> > 
> > We need a clearer way to prevent it, but can't revert fb561bf9abde until that.
> 
> Yeah that entire area is a mess still, ideally we'd have something else
> creating the platform devices, and efifb/offb and all these would just
> bind against them.
> 
> Hm one idea that just crossed my mind: Could we have a flag in fb_info for
> fw drivers, and check this in framebuffer_register? Then at least all the
> logic would be in the fbdev core.

Ok coffee just kicked in, how exactly does your scenario work?

This code I'm reverting here is in the platform_dev->probe function.
Thomas' patch removes the platform_dev. How exactly can you still probe
against a platform dev if that platform dev is gone?

Iow, now that I reponder your case after a few weeks I'm no longer sure
things work like you claim.

There is the issue that offb still bidns without a platform_dev, but
that's not affected by this patch here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers
  2022-02-10 11:46   ` Thomas Zimmermann
@ 2022-04-05  8:45     ` Daniel Vetter
  0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05  8:45 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: Daniel Vetter, DRI Development, linux-fbdev, Du Cheng,
	Tetsuo Handa, Intel Graphics Development, LKML, Claudio Suarez,
	Greg Kroah-Hartman, Daniel Vetter, Sam Ravnborg

On Thu, Feb 10, 2022 at 12:46:32PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.02.22 um 22:08 schrieb Daniel Vetter:
> > There's two minor behaviour changes in here:
> > - in error paths we now consistently call fb_ops->fb_release
> > - fb_release really can't fail (fbmem.c ignores it too) and there's no
> >    reasonable cleanup we can do anyway.
> > 
> > Note that everything in fbcon.c is protected by the big console_lock()
> > lock (especially all the global variables), so the minor changes in
> > ordering of setup/cleanup do not matter.
> > 
> > v2: Explain a bit better why this is all correct (Sam)
> > 
> > Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Claudio Suarez <cssk@net-c.es>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Cc: Du Cheng <ducheng2@gmail.com>
> > ---
> >   drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++----------------
> >   1 file changed, 53 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index 058e885d24f6..3e1a3e7bf527 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount)
> >   #endif /* CONFIG_MISC_TILEBLITTING */
> > +static int fbcon_open(struct fb_info *info)
> > +{
> > +	if (!try_module_get(info->fbops->owner))
> > +		return -ENODEV;
> > +
> > +	if (info->fbops->fb_open &&
> > +	    info->fbops->fb_open(info, 0)) {
> > +		module_put(info->fbops->owner);
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void fbcon_release(struct fb_info *info)
> > +{
> > +	if (info->fbops->fb_release)
> > +		info->fbops->fb_release(info, 0);
> > +
> > +	module_put(info->fbops->owner);
> > +}
> >   static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
> >   				  int unit, int oldidx)
> >   {
> >   	struct fbcon_ops *ops = NULL;
> > -	int err = 0;
> > -
> > -	if (!try_module_get(info->fbops->owner))
> > -		err = -ENODEV;
> > +	int err;
> > -	if (!err && info->fbops->fb_open &&
> > -	    info->fbops->fb_open(info, 0))
> > -		err = -ENODEV;
> > +	err = fbcon_open(info);
> > +	if (err)
> > +		return err;
> >   	if (!err) {
> >   		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
> >   	if (err) {
> >   		con2fb_map[unit] = oldidx;
> > -		module_put(info->fbops->owner);
> > +		fbcon_release(info);
> >   	}
> >   	return err;
> > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
> >   				  int oldidx, int found)
> >   {
> >   	struct fbcon_ops *ops = oldinfo->fbcon_par;
> > -	int err = 0, ret;
> > +	int ret;
> > -	if (oldinfo->fbops->fb_release &&
> > -	    oldinfo->fbops->fb_release(oldinfo, 0)) {
> > -		con2fb_map[unit] = oldidx;
> 
> We don't need oldidx any longer?
> 
> There's some logic wrt to the parameter 'found' here and in set_con2fb_map()
> that appears to be relevant.

Yeah further patches clean this up more. Or did you see a potential bug
here? I did ditch the fb_release error handling, simply because there's
not really much we can do anyway, it shouldn't ever fail (that's a driver
bug) and it was convoluting the code for no gain. But I might have missed
something in this cargo-cult.
-Daniel

> 
> Best regards
> Thomas
> 
> 
> > -		if (!found && newinfo->fbops->fb_release)
> > -			newinfo->fbops->fb_release(newinfo, 0);
> > -		if (!found)
> > -			module_put(newinfo->fbops->owner);
> > -		err = -ENODEV;
> > -	}
> > +	fbcon_release(oldinfo);
> > -	if (!err) {
> > -		fbcon_del_cursor_work(oldinfo);
> > -		kfree(ops->cursor_state.mask);
> > -		kfree(ops->cursor_data);
> > -		kfree(ops->cursor_src);
> > -		kfree(ops->fontbuffer);
> > -		kfree(oldinfo->fbcon_par);
> > -		oldinfo->fbcon_par = NULL;
> > -		module_put(oldinfo->fbops->owner);
> > -		/*
> > -		  If oldinfo and newinfo are driving the same hardware,
> > -		  the fb_release() method of oldinfo may attempt to
> > -		  restore the hardware state.  This will leave the
> > -		  newinfo in an undefined state. Thus, a call to
> > -		  fb_set_par() may be needed for the newinfo.
> > -		*/
> > -		if (newinfo && newinfo->fbops->fb_set_par) {
> > -			ret = newinfo->fbops->fb_set_par(newinfo);
> > +	fbcon_del_cursor_work(oldinfo);
> > +	kfree(ops->cursor_state.mask);
> > +	kfree(ops->cursor_data);
> > +	kfree(ops->cursor_src);
> > +	kfree(ops->fontbuffer);
> > +	kfree(oldinfo->fbcon_par);
> > +	oldinfo->fbcon_par = NULL;
> > +	/*
> > +	  If oldinfo and newinfo are driving the same hardware,
> > +	  the fb_release() method of oldinfo may attempt to
> > +	  restore the hardware state.  This will leave the
> > +	  newinfo in an undefined state. Thus, a call to
> > +	  fb_set_par() may be needed for the newinfo.
> > +	*/
> > +	if (newinfo && newinfo->fbops->fb_set_par) {
> > +		ret = newinfo->fbops->fb_set_par(newinfo);
> > -			if (ret)
> > -				printk(KERN_ERR "con2fb_release_oldinfo: "
> > -					"detected unhandled fb_set_par error, "
> > -					"error code %d\n", ret);
> > -		}
> > +		if (ret)
> > +			printk(KERN_ERR "con2fb_release_oldinfo: "
> > +				"detected unhandled fb_set_par error, "
> > +				"error code %d\n", ret);
> >   	}
> > -	return err;
> > +	return 0;
> >   }
> >   static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
> > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void)
> >   	struct fbcon_display *p = &fb_display[fg_console];
> >   	struct vc_data *vc = vc_cons[fg_console].d;
> >   	const struct font_desc *font = NULL;
> > -	struct module *owner;
> >   	struct fb_info *info = NULL;
> >   	struct fbcon_ops *ops;
> >   	int rows, cols;
> > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void)
> >   	if (!info)
> >   		return NULL;
> >   	
> > -	owner = info->fbops->owner;
> > -	if (!try_module_get(owner))
> > +	if (fbcon_open(info))
> >   		return NULL;
> > -	if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) {
> > -		module_put(owner);
> > -		return NULL;
> > -	}
> >   	ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> >   	if (!ops) {
> > -		module_put(owner);
> > +		fbcon_release(info);
> >   		return NULL;
> >   	}
> > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void)
> >   		}
> >   		if (mapped) {
> > -			if (info->fbops->fb_release)
> > -				info->fbops->fb_release(info, 0);
> > -			module_put(info->fbops->owner);
> > -
> >   			if (info->fbcon_par) {
> >   				struct fbcon_ops *ops = info->fbcon_par;
> > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void)
> >   				kfree(info->fbcon_par);
> >   				info->fbcon_par = NULL;
> >   			}
> > +
> > +			fbcon_release(info);
> >   		}
> >   	}
> >   }
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05  8:40       ` Daniel Vetter
@ 2022-04-05  9:19         ` Javier Martinez Canillas
  2022-04-05  9:24           ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-04-05  9:19 UTC (permalink / raw)
  To: DRI Development, Intel Graphics Development, linux-fbdev, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

Hello Daniel,

On 4/5/22 10:40, Daniel Vetter wrote:
> On Tue, Apr 05, 2022 at 10:36:35AM +0200, Daniel Vetter wrote:
>> On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
>>> On 2/8/22 22:08, Daniel Vetter wrote:
>>>> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
>>>>
>>>> With
>>>>
>>>> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
>>>> Author: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Date:   Tue Jan 25 10:12:18 2022 +0100
>>>>
>>>>     fbdev: Hot-unplug firmware fb devices on forced removal
>>>>
>>>> this should be fixed properly and we can remove this somewhat hackish
>>>> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
>>>> enabled).
>>>>
>>>
>>> Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
>>> of platform devices matched with fbdev drivers to be properly unregistered if
>>> a DRM driver attempts to remove all the conflicting framebuffers.
>>>
>>> But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
>>> a FB is already registered") worked around is different. It happens when the
>>> DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
>>> kicking out of conflicting framebuffers already happened and these drivers
>>> will be allowed to probe even when a DRM driver is already present.
>>>
>>> We need a clearer way to prevent it, but can't revert fb561bf9abde until that.
>>
>> Yeah that entire area is a mess still, ideally we'd have something else
>> creating the platform devices, and efifb/offb and all these would just
>> bind against them.
>>
>> Hm one idea that just crossed my mind: Could we have a flag in fb_info for
>> fw drivers, and check this in framebuffer_register? Then at least all the
>> logic would be in the fbdev core.
>

I can't answer right away since I've since forgotten this part of the code
and will require to do a detailed read to refresh my memory.

I'll answer later but preferred to mention the other question ASAP.
 
> Ok coffee just kicked in, how exactly does your scenario work?
> 
> This code I'm reverting here is in the platform_dev->probe function.
> Thomas' patch removes the platform_dev. How exactly can you still probe
> against a platform dev if that platform dev is gone?
>

Because the platform was not even registered by the time the DRM driver
probed and all the devices for the conflicting drivers were unregistered.
 
> Iow, now that I reponder your case after a few weeks I'm no longer sure
> things work like you claim.
>

This is how I think that work, please let me know if you see something
wrong in my logic:

1) A PCI device of OF device is registered for the GPU, this attempt to
   match a registered driver but no driver was registered that match yet.

2) The efifb driver is built-in, will be initialized according to the link
   order of the objects under drivers/video and the fbdev driver is registered.

   There is no platform device or PCI/OF device registered that matches.

3) The DRM driver is built-in, will be initialized according to the link
   order of the objects under drivers/gpu and the DRM driver is registered.
   
   This matches the device registered in (1) and the DRM driver probes.

4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
   before registering the DRM device.

   There are no conflicting drivers or platform device at this point.

5) Latter at some point the drivers/firmware/sysfb.c init function is
   executed, and this registers a platform device for the generic fb.

   This device matches the efifb driver registered in (2) and the fbdev
   driver probes.
   
   Since that happens *after* the DRM driver already matched, probed
   and registered the DRM device, that is a bug and what the reverted
   patch worked around.

So we need to prevent (5) if (1) and (3) already happened. Having a flag
set in the fbdev core somewhere when remove_conflicting_framebuffers()
is called could be a solution indeed.

That is, the fbdev core needs to know that a DRM driver already probed
and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE

I can attempt to write a patch for that.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05  9:19         ` Javier Martinez Canillas
@ 2022-04-05  9:24           ` Daniel Vetter
  2022-04-05  9:52             ` Javier Martinez Canillas
  0 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05  9:24 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: DRI Development, Intel Graphics Development, linux-fbdev, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Daniel,
>
> On 4/5/22 10:40, Daniel Vetter wrote:
> > On Tue, Apr 05, 2022 at 10:36:35AM +0200, Daniel Vetter wrote:
> >> On Wed, Feb 09, 2022 at 01:19:26AM +0100, Javier Martinez Canillas wrote:
> >>> On 2/8/22 22:08, Daniel Vetter wrote:
> >>>> This reverts commit fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee.
> >>>>
> >>>> With
> >>>>
> >>>> commit 27599aacbaefcbf2af7b06b0029459bbf682000d
> >>>> Author: Thomas Zimmermann <tzimmermann@suse.de>
> >>>> Date:   Tue Jan 25 10:12:18 2022 +0100
> >>>>
> >>>>     fbdev: Hot-unplug firmware fb devices on forced removal
> >>>>
> >>>> this should be fixed properly and we can remove this somewhat hackish
> >>>> check here (e.g. this won't catch drm drivers if fbdev emulation isn't
> >>>> enabled).
> >>>>
> >>>
> >>> Unfortunately this hack can't be reverted yet. Thomas' patch solves the issue
> >>> of platform devices matched with fbdev drivers to be properly unregistered if
> >>> a DRM driver attempts to remove all the conflicting framebuffers.
> >>>
> >>> But the problem that fb561bf9abde ("fbdev: Prevent probing generic drivers if
> >>> a FB is already registered") worked around is different. It happens when the
> >>> DRM driver is probed before the {efi,simple}fb and other fbdev drivers, the
> >>> kicking out of conflicting framebuffers already happened and these drivers
> >>> will be allowed to probe even when a DRM driver is already present.
> >>>
> >>> We need a clearer way to prevent it, but can't revert fb561bf9abde until that.
> >>
> >> Yeah that entire area is a mess still, ideally we'd have something else
> >> creating the platform devices, and efifb/offb and all these would just
> >> bind against them.
> >>
> >> Hm one idea that just crossed my mind: Could we have a flag in fb_info for
> >> fw drivers, and check this in framebuffer_register? Then at least all the
> >> logic would be in the fbdev core.
> >
>
> I can't answer right away since I've since forgotten this part of the code
> and will require to do a detailed read to refresh my memory.
>
> I'll answer later but preferred to mention the other question ASAP.
>
> > Ok coffee just kicked in, how exactly does your scenario work?
> >
> > This code I'm reverting here is in the platform_dev->probe function.
> > Thomas' patch removes the platform_dev. How exactly can you still probe
> > against a platform dev if that platform dev is gone?
> >
>
> Because the platform was not even registered by the time the DRM driver
> probed and all the devices for the conflicting drivers were unregistered.
>
> > Iow, now that I reponder your case after a few weeks I'm no longer sure
> > things work like you claim.
> >
>
> This is how I think that work, please let me know if you see something
> wrong in my logic:
>
> 1) A PCI device of OF device is registered for the GPU, this attempt to
>    match a registered driver but no driver was registered that match yet.
>
> 2) The efifb driver is built-in, will be initialized according to the link
>    order of the objects under drivers/video and the fbdev driver is registered.
>
>    There is no platform device or PCI/OF device registered that matches.
>
> 3) The DRM driver is built-in, will be initialized according to the link
>    order of the objects under drivers/gpu and the DRM driver is registered.
>
>    This matches the device registered in (1) and the DRM driver probes.
>
> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
>    before registering the DRM device.
>
>    There are no conflicting drivers or platform device at this point.
>
> 5) Latter at some point the drivers/firmware/sysfb.c init function is
>    executed, and this registers a platform device for the generic fb.
>
>    This device matches the efifb driver registered in (2) and the fbdev
>    driver probes.
>
>    Since that happens *after* the DRM driver already matched, probed
>    and registered the DRM device, that is a bug and what the reverted
>    patch worked around.
>
> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> is called could be a solution indeed.
>
> That is, the fbdev core needs to know that a DRM driver already probed
> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
>
> I can attempt to write a patch for that.

Ah yeah that could be an issue. I think the right fix is to replace
the platform dev unregister with a sysfb_unregister() function in
sysfb.c, which is synced with a common lock with the sysfb_init
function and a small boolean. I think I can type that up quickly for
v3.
-Daniel

>
> --
> Best regards,
>
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05  9:24           ` Daniel Vetter
@ 2022-04-05  9:52             ` Javier Martinez Canillas
  2022-04-05 10:34               ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-04-05  9:52 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, Intel Graphics Development, linux-fbdev, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On 4/5/22 11:24, Daniel Vetter wrote:
> On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas

[snip]

>>
>> This is how I think that work, please let me know if you see something
>> wrong in my logic:
>>
>> 1) A PCI device of OF device is registered for the GPU, this attempt to
>>    match a registered driver but no driver was registered that match yet.
>>
>> 2) The efifb driver is built-in, will be initialized according to the link
>>    order of the objects under drivers/video and the fbdev driver is registered.
>>
>>    There is no platform device or PCI/OF device registered that matches.
>>
>> 3) The DRM driver is built-in, will be initialized according to the link
>>    order of the objects under drivers/gpu and the DRM driver is registered.
>>
>>    This matches the device registered in (1) and the DRM driver probes.
>>
>> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
>>    before registering the DRM device.
>>
>>    There are no conflicting drivers or platform device at this point.
>>
>> 5) Latter at some point the drivers/firmware/sysfb.c init function is
>>    executed, and this registers a platform device for the generic fb.
>>
>>    This device matches the efifb driver registered in (2) and the fbdev
>>    driver probes.
>>
>>    Since that happens *after* the DRM driver already matched, probed
>>    and registered the DRM device, that is a bug and what the reverted
>>    patch worked around.
>>
>> So we need to prevent (5) if (1) and (3) already happened. Having a flag
>> set in the fbdev core somewhere when remove_conflicting_framebuffers()
>> is called could be a solution indeed.
>>
>> That is, the fbdev core needs to know that a DRM driver already probed
>> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
>>
>> I can attempt to write a patch for that.
> 
> Ah yeah that could be an issue. I think the right fix is to replace
> the platform dev unregister with a sysfb_unregister() function in
> sysfb.c, which is synced with a common lock with the sysfb_init
> function and a small boolean. I think I can type that up quickly for
> v3.

It's more complicated than that since sysfb is just *one* of the several
places where platform devices can be registered for video devices.

For instance, the vga16fb driver registers its own platform device in
its module_init() function so that can also happen after the conflicting
framebuffers (and associated devices) were removed by a DRM driver probe.

I tried to minimize the issue for that particular driver with commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f

But the point stands, it all boils down to the fact that you have two
different subsystems registering video drivers and they don't know all
about each other to take a proper decision.

Right now the drm_aperture_remove_conflicting_framebuffers() call signals
in one direction from DRM to fbdev but there isn't a communication in the
other direction, from fbdev to DRM.

I believe the correct fix would be for the fbdev core to keep a list of
the apertures struct that are passed to remove_conflicting_framebuffers(),
that way it will know what apertures are not available anymore and prevent
to register any fbdev framebuffer that conflicts with one already present.

Let me know if you think that makes sense and I can attempt to write a fix.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05  9:52             ` Javier Martinez Canillas
@ 2022-04-05 10:34               ` Daniel Vetter
  2022-04-05 13:24                 ` Geert Uytterhoeven
  2022-04-05 13:25                 ` Javier Martinez Canillas
  0 siblings, 2 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05 10:34 UTC (permalink / raw)
  To: Javier Martinez Canillas, Greg KH
  Cc: DRI Development, Intel Graphics Development, linux-fbdev, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> On 4/5/22 11:24, Daniel Vetter wrote:
> > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
>
> [snip]
>
> >>
> >> This is how I think that work, please let me know if you see something
> >> wrong in my logic:
> >>
> >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> >>    match a registered driver but no driver was registered that match yet.
> >>
> >> 2) The efifb driver is built-in, will be initialized according to the link
> >>    order of the objects under drivers/video and the fbdev driver is registered.
> >>
> >>    There is no platform device or PCI/OF device registered that matches.
> >>
> >> 3) The DRM driver is built-in, will be initialized according to the link
> >>    order of the objects under drivers/gpu and the DRM driver is registered.
> >>
> >>    This matches the device registered in (1) and the DRM driver probes.
> >>
> >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> >>    before registering the DRM device.
> >>
> >>    There are no conflicting drivers or platform device at this point.
> >>
> >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> >>    executed, and this registers a platform device for the generic fb.
> >>
> >>    This device matches the efifb driver registered in (2) and the fbdev
> >>    driver probes.
> >>
> >>    Since that happens *after* the DRM driver already matched, probed
> >>    and registered the DRM device, that is a bug and what the reverted
> >>    patch worked around.
> >>
> >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> >> is called could be a solution indeed.
> >>
> >> That is, the fbdev core needs to know that a DRM driver already probed
> >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> >>
> >> I can attempt to write a patch for that.
> >
> > Ah yeah that could be an issue. I think the right fix is to replace
> > the platform dev unregister with a sysfb_unregister() function in
> > sysfb.c, which is synced with a common lock with the sysfb_init
> > function and a small boolean. I think I can type that up quickly for
> > v3.
>
> It's more complicated than that since sysfb is just *one* of the several
> places where platform devices can be registered for video devices.
>
> For instance, the vga16fb driver registers its own platform device in
> its module_init() function so that can also happen after the conflicting
> framebuffers (and associated devices) were removed by a DRM driver probe.
>
> I tried to minimize the issue for that particular driver with commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
>
> But the point stands, it all boils down to the fact that you have two
> different subsystems registering video drivers and they don't know all
> about each other to take a proper decision.
>
> Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> in one direction from DRM to fbdev but there isn't a communication in the
> other direction, from fbdev to DRM.
>
> I believe the correct fix would be for the fbdev core to keep a list of
> the apertures struct that are passed to remove_conflicting_framebuffers(),
> that way it will know what apertures are not available anymore and prevent
> to register any fbdev framebuffer that conflicts with one already present.

Hm that still feels like reinventing a driver model, badly.

I think there's two cleaner solutions:
- move all the firmware driver platform_dev into sysfb.c, and then
just bind the special cases against that (e.g. offb, vga16fb and all
these). Then we'd have one sysfb_try_unregister(struct device *dev)
interface that fbmem.c uses.
- let fbmem.c call into each of these firmware device providers, which
means some loops most likely (like we can't call into vga16fb), so
probably need to move that into fbmem.c and it all gets a bit messy.

> Let me know if you think that makes sense and I can attempt to write a fix.

I still think unregistering the platform_dev properly makes the most
sense, and feels like the most proper linux device model solution
instead of hacks on top - if the firmware fb is unuseable because a
native driver has taken over, we should nuke that. And also the
firmware fb driver would then just bind to that platform_dev if it
exists, and only if it exists. Also I think it should be the
responsibility of whichever piece of code that registers these
platform devices to ensure that platform_dev actually still exists.
That's why I think pushing all that code into sysfb.c is probably the
cleanest solution.

fbdev predates all that stuff by a lot, hence the hand-rolling.

But maybe Greg has some more thoughts here too?
-Daniel

>
> --
> Best regards,
>
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 10:34               ` Daniel Vetter
@ 2022-04-05 13:24                 ` Geert Uytterhoeven
  2022-04-05 13:33                   ` Greg KH
  2022-04-05 13:25                 ` Javier Martinez Canillas
  1 sibling, 1 reply; 49+ messages in thread
From: Geert Uytterhoeven @ 2022-04-05 13:24 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Javier Martinez Canillas, Greg KH, DRI Development,
	Intel Graphics Development, Linux Fbdev development list, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

Hi Daniel,

On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> > On 4/5/22 11:24, Daniel Vetter wrote:
> > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > >> This is how I think that work, please let me know if you see something
> > >> wrong in my logic:
> > >>
> > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > >>    match a registered driver but no driver was registered that match yet.
> > >>
> > >> 2) The efifb driver is built-in, will be initialized according to the link
> > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > >>
> > >>    There is no platform device or PCI/OF device registered that matches.
> > >>
> > >> 3) The DRM driver is built-in, will be initialized according to the link
> > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > >>
> > >>    This matches the device registered in (1) and the DRM driver probes.
> > >>
> > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > >>    before registering the DRM device.
> > >>
> > >>    There are no conflicting drivers or platform device at this point.
> > >>
> > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > >>    executed, and this registers a platform device for the generic fb.
> > >>
> > >>    This device matches the efifb driver registered in (2) and the fbdev
> > >>    driver probes.
> > >>
> > >>    Since that happens *after* the DRM driver already matched, probed
> > >>    and registered the DRM device, that is a bug and what the reverted
> > >>    patch worked around.
> > >>
> > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > >> is called could be a solution indeed.
> > >>
> > >> That is, the fbdev core needs to know that a DRM driver already probed
> > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > >>
> > >> I can attempt to write a patch for that.
> > >
> > > Ah yeah that could be an issue. I think the right fix is to replace
> > > the platform dev unregister with a sysfb_unregister() function in
> > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > function and a small boolean. I think I can type that up quickly for
> > > v3.
> >
> > It's more complicated than that since sysfb is just *one* of the several
> > places where platform devices can be registered for video devices.
> >
> > For instance, the vga16fb driver registers its own platform device in
> > its module_init() function so that can also happen after the conflicting
> > framebuffers (and associated devices) were removed by a DRM driver probe.
> >
> > I tried to minimize the issue for that particular driver with commit:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> >
> > But the point stands, it all boils down to the fact that you have two
> > different subsystems registering video drivers and they don't know all
> > about each other to take a proper decision.
> >
> > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > in one direction from DRM to fbdev but there isn't a communication in the
> > other direction, from fbdev to DRM.
> >
> > I believe the correct fix would be for the fbdev core to keep a list of
> > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > that way it will know what apertures are not available anymore and prevent
> > to register any fbdev framebuffer that conflicts with one already present.
>
> Hm that still feels like reinventing a driver model, badly.
>
> I think there's two cleaner solutions:
> - move all the firmware driver platform_dev into sysfb.c, and then
> just bind the special cases against that (e.g. offb, vga16fb and all
> these). Then we'd have one sysfb_try_unregister(struct device *dev)
> interface that fbmem.c uses.
> - let fbmem.c call into each of these firmware device providers, which
> means some loops most likely (like we can't call into vga16fb), so
> probably need to move that into fbmem.c and it all gets a bit messy.
>
> > Let me know if you think that makes sense and I can attempt to write a fix.
>
> I still think unregistering the platform_dev properly makes the most

That doesn't sound very driver-model-aware to me. The device is what
the driver binds to; it does not cease to exist.

> sense, and feels like the most proper linux device model solution
> instead of hacks on top - if the firmware fb is unuseable because a
> native driver has taken over, we should nuke that. And also the
> firmware fb driver would then just bind to that platform_dev if it
> exists, and only if it exists. Also I think it should be the
> responsibility of whichever piece of code that registers these
> platform devices to ensure that platform_dev actually still exists.
> That's why I think pushing all that code into sysfb.c is probably the
> cleanest solution.

Can't you unbind the generic driver first, and bind the specific driver
afterwards? Alike writing to sysfs unbind/driver_override/bind,
but from code?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 10:34               ` Daniel Vetter
  2022-04-05 13:24                 ` Geert Uytterhoeven
@ 2022-04-05 13:25                 ` Javier Martinez Canillas
  1 sibling, 0 replies; 49+ messages in thread
From: Javier Martinez Canillas @ 2022-04-05 13:25 UTC (permalink / raw)
  To: Daniel Vetter, Greg KH
  Cc: DRI Development, Intel Graphics Development, linux-fbdev, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On 4/5/22 12:34, Daniel Vetter wrote:
> On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> <javierm@redhat.com> wrote:

[snip]

>>
>> I believe the correct fix would be for the fbdev core to keep a list of
>> the apertures struct that are passed to remove_conflicting_framebuffers(),
>> that way it will know what apertures are not available anymore and prevent
>> to register any fbdev framebuffer that conflicts with one already present.
> 
> Hm that still feels like reinventing a driver model, badly.
>

Yeah, you are correct.
 
> I think there's two cleaner solutions:
> - move all the firmware driver platform_dev into sysfb.c, and then
> just bind the special cases against that (e.g. offb, vga16fb and all
> these). Then we'd have one sysfb_try_unregister(struct device *dev)
> interface that fbmem.c uses.

I think this is the cleaner option. And makes sense to consolidate all
the firmware drivers platform device registration to sysfb.c.

Already does for VIDEO_TYPE_EFI ("efi-framebuffer") and VIDEO_TYPE_VLFB
("vesa-framebuffer"), so need to also make it cope with VIDEO_TYPE_EGAC
and VIDEO_TYPE_VGAC ("vga16fb").

For offb is less clear since currently the offb driver does not really
use the Linux device model, that is the driver does not match a device
that's registered, there's no device which is the bug that was reported
to Thomas in the other thread.

It's unclear how to properly fix that since we will need to convert the
offb driver to register a platform driver and match against a device that
is registered by some platform code that parses the OF...

> - let fbmem.c call into each of these firmware device providers, which
> means some loops most likely (like we can't call into vga16fb), so
> probably need to move that into fbmem.c and it all gets a bit messy.
> 

Yup, that would get messy indeed so not a good option.

>> Let me know if you think that makes sense and I can attempt to write a fix.
> 
> I still think unregistering the platform_dev properly makes the most
> sense, and feels like the most proper linux device model solution
> instead of hacks on top - if the firmware fb is unuseable because a
> native driver has taken over, we should nuke that. And also the
> firmware fb driver would then just bind to that platform_dev if it
> exists, and only if it exists. Also I think it should be the
> responsibility of whichever piece of code that registers these
> platform devices to ensure that platform_dev actually still exists.
> That's why I think pushing all that code into sysfb.c is probably the
> cleanest solution.
>

Agreed. Not registering the platform devices if there is already a DRM
driver for the same device is what makes the most sense. What I don't
understand is how sysfb would know that if run after a DRM registration.

The only way that could know is if sysfb would keep a list of apertures
for all the DRM drivers registered or if the DRM core somewhat notifies
to sysfb that a native driver was already registered.

Another option and probably the cleanest although the harder solution is
to finally bite the bullet and make all the DRM drivers to request their
memory region.

Or as you mentioned in the past, to move that logic into the device model
and then not allow to register devices that require an overlapping region.

And there could be a request_mem_region_remove_conflicting() or something
that real DRM drivers could use to force a memory region request and make
the device model to unregister any device that may already have that mem.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat


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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 13:24                 ` Geert Uytterhoeven
@ 2022-04-05 13:33                   ` Greg KH
  2022-04-05 16:12                     ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Greg KH @ 2022-04-05 13:33 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Daniel Vetter, Javier Martinez Canillas, DRI Development,
	Intel Graphics Development, Linux Fbdev development list, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, Apr 05, 2022 at 03:24:40PM +0200, Geert Uytterhoeven wrote:
> Hi Daniel,
> 
> On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> > > On 4/5/22 11:24, Daniel Vetter wrote:
> > > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > > >> This is how I think that work, please let me know if you see something
> > > >> wrong in my logic:
> > > >>
> > > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > > >>    match a registered driver but no driver was registered that match yet.
> > > >>
> > > >> 2) The efifb driver is built-in, will be initialized according to the link
> > > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > > >>
> > > >>    There is no platform device or PCI/OF device registered that matches.
> > > >>
> > > >> 3) The DRM driver is built-in, will be initialized according to the link
> > > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > > >>
> > > >>    This matches the device registered in (1) and the DRM driver probes.
> > > >>
> > > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > > >>    before registering the DRM device.
> > > >>
> > > >>    There are no conflicting drivers or platform device at this point.
> > > >>
> > > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > > >>    executed, and this registers a platform device for the generic fb.
> > > >>
> > > >>    This device matches the efifb driver registered in (2) and the fbdev
> > > >>    driver probes.
> > > >>
> > > >>    Since that happens *after* the DRM driver already matched, probed
> > > >>    and registered the DRM device, that is a bug and what the reverted
> > > >>    patch worked around.
> > > >>
> > > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > > >> is called could be a solution indeed.
> > > >>
> > > >> That is, the fbdev core needs to know that a DRM driver already probed
> > > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > > >>
> > > >> I can attempt to write a patch for that.
> > > >
> > > > Ah yeah that could be an issue. I think the right fix is to replace
> > > > the platform dev unregister with a sysfb_unregister() function in
> > > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > > function and a small boolean. I think I can type that up quickly for
> > > > v3.
> > >
> > > It's more complicated than that since sysfb is just *one* of the several
> > > places where platform devices can be registered for video devices.
> > >
> > > For instance, the vga16fb driver registers its own platform device in
> > > its module_init() function so that can also happen after the conflicting
> > > framebuffers (and associated devices) were removed by a DRM driver probe.
> > >
> > > I tried to minimize the issue for that particular driver with commit:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> > >
> > > But the point stands, it all boils down to the fact that you have two
> > > different subsystems registering video drivers and they don't know all
> > > about each other to take a proper decision.
> > >
> > > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > > in one direction from DRM to fbdev but there isn't a communication in the
> > > other direction, from fbdev to DRM.
> > >
> > > I believe the correct fix would be for the fbdev core to keep a list of
> > > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > > that way it will know what apertures are not available anymore and prevent
> > > to register any fbdev framebuffer that conflicts with one already present.
> >
> > Hm that still feels like reinventing a driver model, badly.
> >
> > I think there's two cleaner solutions:
> > - move all the firmware driver platform_dev into sysfb.c, and then
> > just bind the special cases against that (e.g. offb, vga16fb and all
> > these). Then we'd have one sysfb_try_unregister(struct device *dev)
> > interface that fbmem.c uses.
> > - let fbmem.c call into each of these firmware device providers, which
> > means some loops most likely (like we can't call into vga16fb), so
> > probably need to move that into fbmem.c and it all gets a bit messy.
> >
> > > Let me know if you think that makes sense and I can attempt to write a fix.
> >
> > I still think unregistering the platform_dev properly makes the most
> 
> That doesn't sound very driver-model-aware to me. The device is what
> the driver binds to; it does not cease to exist.

I agree, that sounds odd.

The device should always stick around (as the bus creates it), it's up
to the driver to bind to the device as needed.

> > sense, and feels like the most proper linux device model solution
> > instead of hacks on top - if the firmware fb is unuseable because a
> > native driver has taken over, we should nuke that. And also the
> > firmware fb driver would then just bind to that platform_dev if it
> > exists, and only if it exists. Also I think it should be the
> > responsibility of whichever piece of code that registers these
> > platform devices to ensure that platform_dev actually still exists.
> > That's why I think pushing all that code into sysfb.c is probably the
> > cleanest solution.
> 
> Can't you unbind the generic driver first, and bind the specific driver
> afterwards? Alike writing to sysfs unbind/driver_override/bind,
> but from code?

That too feels odd, what is so special about the fbdev code that the
normal driver functions do not work for them?  It shouldn't matter if
multiple subsystems register video devices, why can't we handle more
than one fb device?

thanks,

greg k-h

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 13:33                   ` Greg KH
@ 2022-04-05 16:12                     ` Daniel Vetter
  2022-04-05 16:44                       ` Greg KH
  0 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05 16:12 UTC (permalink / raw)
  To: Greg KH
  Cc: Geert Uytterhoeven, Daniel Vetter, Javier Martinez Canillas,
	DRI Development, Intel Graphics Development,
	Linux Fbdev development list, LKML, Thomas Zimmermann,
	Zack Rusin, Hans de Goede, Ilya Trukhanov, Daniel Vetter,
	Peter Jones

On Tue, Apr 05, 2022 at 03:33:17PM +0200, Greg KH wrote:
> On Tue, Apr 05, 2022 at 03:24:40PM +0200, Geert Uytterhoeven wrote:
> > Hi Daniel,
> > 
> > On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> > > <javierm@redhat.com> wrote:
> > > > On 4/5/22 11:24, Daniel Vetter wrote:
> > > > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > > > >> This is how I think that work, please let me know if you see something
> > > > >> wrong in my logic:
> > > > >>
> > > > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > > > >>    match a registered driver but no driver was registered that match yet.
> > > > >>
> > > > >> 2) The efifb driver is built-in, will be initialized according to the link
> > > > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > > > >>
> > > > >>    There is no platform device or PCI/OF device registered that matches.
> > > > >>
> > > > >> 3) The DRM driver is built-in, will be initialized according to the link
> > > > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > > > >>
> > > > >>    This matches the device registered in (1) and the DRM driver probes.
> > > > >>
> > > > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > > > >>    before registering the DRM device.
> > > > >>
> > > > >>    There are no conflicting drivers or platform device at this point.
> > > > >>
> > > > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > > > >>    executed, and this registers a platform device for the generic fb.
> > > > >>
> > > > >>    This device matches the efifb driver registered in (2) and the fbdev
> > > > >>    driver probes.
> > > > >>
> > > > >>    Since that happens *after* the DRM driver already matched, probed
> > > > >>    and registered the DRM device, that is a bug and what the reverted
> > > > >>    patch worked around.
> > > > >>
> > > > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > > > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > > > >> is called could be a solution indeed.
> > > > >>
> > > > >> That is, the fbdev core needs to know that a DRM driver already probed
> > > > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > > > >>
> > > > >> I can attempt to write a patch for that.
> > > > >
> > > > > Ah yeah that could be an issue. I think the right fix is to replace
> > > > > the platform dev unregister with a sysfb_unregister() function in
> > > > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > > > function and a small boolean. I think I can type that up quickly for
> > > > > v3.
> > > >
> > > > It's more complicated than that since sysfb is just *one* of the several
> > > > places where platform devices can be registered for video devices.
> > > >
> > > > For instance, the vga16fb driver registers its own platform device in
> > > > its module_init() function so that can also happen after the conflicting
> > > > framebuffers (and associated devices) were removed by a DRM driver probe.
> > > >
> > > > I tried to minimize the issue for that particular driver with commit:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> > > >
> > > > But the point stands, it all boils down to the fact that you have two
> > > > different subsystems registering video drivers and they don't know all
> > > > about each other to take a proper decision.
> > > >
> > > > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > > > in one direction from DRM to fbdev but there isn't a communication in the
> > > > other direction, from fbdev to DRM.
> > > >
> > > > I believe the correct fix would be for the fbdev core to keep a list of
> > > > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > > > that way it will know what apertures are not available anymore and prevent
> > > > to register any fbdev framebuffer that conflicts with one already present.
> > >
> > > Hm that still feels like reinventing a driver model, badly.
> > >
> > > I think there's two cleaner solutions:
> > > - move all the firmware driver platform_dev into sysfb.c, and then
> > > just bind the special cases against that (e.g. offb, vga16fb and all
> > > these). Then we'd have one sysfb_try_unregister(struct device *dev)
> > > interface that fbmem.c uses.
> > > - let fbmem.c call into each of these firmware device providers, which
> > > means some loops most likely (like we can't call into vga16fb), so
> > > probably need to move that into fbmem.c and it all gets a bit messy.
> > >
> > > > Let me know if you think that makes sense and I can attempt to write a fix.
> > >
> > > I still think unregistering the platform_dev properly makes the most
> > 
> > That doesn't sound very driver-model-aware to me. The device is what
> > the driver binds to; it does not cease to exist.
> 
> I agree, that sounds odd.
> 
> The device should always stick around (as the bus creates it), it's up
> to the driver to bind to the device as needed.

The device actually disappears when the real driver takes over.

The firmware fb is a special thing which only really exists as long as the
firmware is in charge of the display hardware. As soon as a real driver
takes over, it stops being a thing.

And since a driver without a device is a bit a funny thing, we have been
pushing towards a model where the firmware code sets up a platform_device
for this fw interface, and the fw driver (efifb, simplefb and others like
that) bind against it. And then we started to throw out that
platform_device (which unbinds the fw driver and prevents it from ever
rebinding), except in the wrong layer so there's a few races.

Should we throw out all that code and replace it with something else? What
would that be like?

Note that the fw side generally has not much clue which real device on
some bus it corresponds to, that part is done through a bunch of magic
tricks. Some of them are simply "I'm taking over a display, pls through
out all fw drivers just to be sure".

> > > sense, and feels like the most proper linux device model solution
> > > instead of hacks on top - if the firmware fb is unuseable because a
> > > native driver has taken over, we should nuke that. And also the
> > > firmware fb driver would then just bind to that platform_dev if it
> > > exists, and only if it exists. Also I think it should be the
> > > responsibility of whichever piece of code that registers these
> > > platform devices to ensure that platform_dev actually still exists.
> > > That's why I think pushing all that code into sysfb.c is probably the
> > > cleanest solution.
> > 
> > Can't you unbind the generic driver first, and bind the specific driver
> > afterwards? Alike writing to sysfs unbind/driver_override/bind,
> > but from code?
> 
> That too feels odd, what is so special about the fbdev code that the
> normal driver functions do not work for them?  It shouldn't matter if
> multiple subsystems register video devices, why can't we handle more
> than one fb device?

The specific driver binds to a completely different device (this one is
more real), and sometimes has not much clue about what exactly the
fw/legacy driver is doing.

The special thing is that in fbdev we have "drivers" which are extremely
thin shims around the fw driver, which has done all the real display setup
for us. I don't think any other subsystem bothers with this, e.g. input
just tells the fw to get lost and never tries to use the fw input support
(stuff like the old horrors of emulating usb kbd as a ps/2 device and
things like that which fw tended to do). Only with display drivers do we
have this world where fairly often a fw driver is loaded first, and then
quite a bit later in the boot process, the real driver loads. It's a bit
like early serial console perhaps, to reduce the gap between when the
kernel loads and when the real display driver is ready.

Cheers, Daniel


> 
> thanks,
> 
> greg k-h

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 16:12                     ` Daniel Vetter
@ 2022-04-05 16:44                       ` Greg KH
  2022-04-05 17:29                         ` Daniel Vetter
  0 siblings, 1 reply; 49+ messages in thread
From: Greg KH @ 2022-04-05 16:44 UTC (permalink / raw)
  To: Geert Uytterhoeven, Javier Martinez Canillas, DRI Development,
	Intel Graphics Development, Linux Fbdev development list, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, Apr 05, 2022 at 06:12:59PM +0200, Daniel Vetter wrote:
> On Tue, Apr 05, 2022 at 03:33:17PM +0200, Greg KH wrote:
> > On Tue, Apr 05, 2022 at 03:24:40PM +0200, Geert Uytterhoeven wrote:
> > > Hi Daniel,
> > > 
> > > On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> > > > <javierm@redhat.com> wrote:
> > > > > On 4/5/22 11:24, Daniel Vetter wrote:
> > > > > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > > > > >> This is how I think that work, please let me know if you see something
> > > > > >> wrong in my logic:
> > > > > >>
> > > > > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > > > > >>    match a registered driver but no driver was registered that match yet.
> > > > > >>
> > > > > >> 2) The efifb driver is built-in, will be initialized according to the link
> > > > > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > > > > >>
> > > > > >>    There is no platform device or PCI/OF device registered that matches.
> > > > > >>
> > > > > >> 3) The DRM driver is built-in, will be initialized according to the link
> > > > > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > > > > >>
> > > > > >>    This matches the device registered in (1) and the DRM driver probes.
> > > > > >>
> > > > > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > > > > >>    before registering the DRM device.
> > > > > >>
> > > > > >>    There are no conflicting drivers or platform device at this point.
> > > > > >>
> > > > > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > > > > >>    executed, and this registers a platform device for the generic fb.
> > > > > >>
> > > > > >>    This device matches the efifb driver registered in (2) and the fbdev
> > > > > >>    driver probes.
> > > > > >>
> > > > > >>    Since that happens *after* the DRM driver already matched, probed
> > > > > >>    and registered the DRM device, that is a bug and what the reverted
> > > > > >>    patch worked around.
> > > > > >>
> > > > > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > > > > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > > > > >> is called could be a solution indeed.
> > > > > >>
> > > > > >> That is, the fbdev core needs to know that a DRM driver already probed
> > > > > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > > > > >>
> > > > > >> I can attempt to write a patch for that.
> > > > > >
> > > > > > Ah yeah that could be an issue. I think the right fix is to replace
> > > > > > the platform dev unregister with a sysfb_unregister() function in
> > > > > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > > > > function and a small boolean. I think I can type that up quickly for
> > > > > > v3.
> > > > >
> > > > > It's more complicated than that since sysfb is just *one* of the several
> > > > > places where platform devices can be registered for video devices.
> > > > >
> > > > > For instance, the vga16fb driver registers its own platform device in
> > > > > its module_init() function so that can also happen after the conflicting
> > > > > framebuffers (and associated devices) were removed by a DRM driver probe.
> > > > >
> > > > > I tried to minimize the issue for that particular driver with commit:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> > > > >
> > > > > But the point stands, it all boils down to the fact that you have two
> > > > > different subsystems registering video drivers and they don't know all
> > > > > about each other to take a proper decision.
> > > > >
> > > > > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > > > > in one direction from DRM to fbdev but there isn't a communication in the
> > > > > other direction, from fbdev to DRM.
> > > > >
> > > > > I believe the correct fix would be for the fbdev core to keep a list of
> > > > > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > > > > that way it will know what apertures are not available anymore and prevent
> > > > > to register any fbdev framebuffer that conflicts with one already present.
> > > >
> > > > Hm that still feels like reinventing a driver model, badly.
> > > >
> > > > I think there's two cleaner solutions:
> > > > - move all the firmware driver platform_dev into sysfb.c, and then
> > > > just bind the special cases against that (e.g. offb, vga16fb and all
> > > > these). Then we'd have one sysfb_try_unregister(struct device *dev)
> > > > interface that fbmem.c uses.
> > > > - let fbmem.c call into each of these firmware device providers, which
> > > > means some loops most likely (like we can't call into vga16fb), so
> > > > probably need to move that into fbmem.c and it all gets a bit messy.
> > > >
> > > > > Let me know if you think that makes sense and I can attempt to write a fix.
> > > >
> > > > I still think unregistering the platform_dev properly makes the most
> > > 
> > > That doesn't sound very driver-model-aware to me. The device is what
> > > the driver binds to; it does not cease to exist.
> > 
> > I agree, that sounds odd.
> > 
> > The device should always stick around (as the bus creates it), it's up
> > to the driver to bind to the device as needed.
> 
> The device actually disappears when the real driver takes over.
> 
> The firmware fb is a special thing which only really exists as long as the
> firmware is in charge of the display hardware. As soon as a real driver
> takes over, it stops being a thing.
> 
> And since a driver without a device is a bit a funny thing, we have been
> pushing towards a model where the firmware code sets up a platform_device
> for this fw interface, and the fw driver (efifb, simplefb and others like
> that) bind against it. And then we started to throw out that
> platform_device (which unbinds the fw driver and prevents it from ever
> rebinding), except in the wrong layer so there's a few races.
> 
> Should we throw out all that code and replace it with something else? What
> would that be like?

Ah, no, sorry, I didn't know that at all.

That sounds semi-sane, just fix the races by moving the layer elsewhere?


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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 16:44                       ` Greg KH
@ 2022-04-05 17:29                         ` Daniel Vetter
  2022-04-07 17:26                           ` Greg KH
  0 siblings, 1 reply; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05 17:29 UTC (permalink / raw)
  To: Greg KH
  Cc: Geert Uytterhoeven, Javier Martinez Canillas, DRI Development,
	Intel Graphics Development, Linux Fbdev development list, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, 5 Apr 2022 at 18:45, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 05, 2022 at 06:12:59PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 05, 2022 at 03:33:17PM +0200, Greg KH wrote:
> > > On Tue, Apr 05, 2022 at 03:24:40PM +0200, Geert Uytterhoeven wrote:
> > > > Hi Daniel,
> > > >
> > > > On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> > > > > <javierm@redhat.com> wrote:
> > > > > > On 4/5/22 11:24, Daniel Vetter wrote:
> > > > > > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > > > > > >> This is how I think that work, please let me know if you see something
> > > > > > >> wrong in my logic:
> > > > > > >>
> > > > > > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > > > > > >>    match a registered driver but no driver was registered that match yet.
> > > > > > >>
> > > > > > >> 2) The efifb driver is built-in, will be initialized according to the link
> > > > > > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > > > > > >>
> > > > > > >>    There is no platform device or PCI/OF device registered that matches.
> > > > > > >>
> > > > > > >> 3) The DRM driver is built-in, will be initialized according to the link
> > > > > > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > > > > > >>
> > > > > > >>    This matches the device registered in (1) and the DRM driver probes.
> > > > > > >>
> > > > > > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > > > > > >>    before registering the DRM device.
> > > > > > >>
> > > > > > >>    There are no conflicting drivers or platform device at this point.
> > > > > > >>
> > > > > > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > > > > > >>    executed, and this registers a platform device for the generic fb.
> > > > > > >>
> > > > > > >>    This device matches the efifb driver registered in (2) and the fbdev
> > > > > > >>    driver probes.
> > > > > > >>
> > > > > > >>    Since that happens *after* the DRM driver already matched, probed
> > > > > > >>    and registered the DRM device, that is a bug and what the reverted
> > > > > > >>    patch worked around.
> > > > > > >>
> > > > > > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > > > > > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > > > > > >> is called could be a solution indeed.
> > > > > > >>
> > > > > > >> That is, the fbdev core needs to know that a DRM driver already probed
> > > > > > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > > > > > >>
> > > > > > >> I can attempt to write a patch for that.
> > > > > > >
> > > > > > > Ah yeah that could be an issue. I think the right fix is to replace
> > > > > > > the platform dev unregister with a sysfb_unregister() function in
> > > > > > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > > > > > function and a small boolean. I think I can type that up quickly for
> > > > > > > v3.
> > > > > >
> > > > > > It's more complicated than that since sysfb is just *one* of the several
> > > > > > places where platform devices can be registered for video devices.
> > > > > >
> > > > > > For instance, the vga16fb driver registers its own platform device in
> > > > > > its module_init() function so that can also happen after the conflicting
> > > > > > framebuffers (and associated devices) were removed by a DRM driver probe.
> > > > > >
> > > > > > I tried to minimize the issue for that particular driver with commit:
> > > > > >
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> > > > > >
> > > > > > But the point stands, it all boils down to the fact that you have two
> > > > > > different subsystems registering video drivers and they don't know all
> > > > > > about each other to take a proper decision.
> > > > > >
> > > > > > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > > > > > in one direction from DRM to fbdev but there isn't a communication in the
> > > > > > other direction, from fbdev to DRM.
> > > > > >
> > > > > > I believe the correct fix would be for the fbdev core to keep a list of
> > > > > > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > > > > > that way it will know what apertures are not available anymore and prevent
> > > > > > to register any fbdev framebuffer that conflicts with one already present.
> > > > >
> > > > > Hm that still feels like reinventing a driver model, badly.
> > > > >
> > > > > I think there's two cleaner solutions:
> > > > > - move all the firmware driver platform_dev into sysfb.c, and then
> > > > > just bind the special cases against that (e.g. offb, vga16fb and all
> > > > > these). Then we'd have one sysfb_try_unregister(struct device *dev)
> > > > > interface that fbmem.c uses.
> > > > > - let fbmem.c call into each of these firmware device providers, which
> > > > > means some loops most likely (like we can't call into vga16fb), so
> > > > > probably need to move that into fbmem.c and it all gets a bit messy.
> > > > >
> > > > > > Let me know if you think that makes sense and I can attempt to write a fix.
> > > > >
> > > > > I still think unregistering the platform_dev properly makes the most
> > > >
> > > > That doesn't sound very driver-model-aware to me. The device is what
> > > > the driver binds to; it does not cease to exist.
> > >
> > > I agree, that sounds odd.
> > >
> > > The device should always stick around (as the bus creates it), it's up
> > > to the driver to bind to the device as needed.
> >
> > The device actually disappears when the real driver takes over.
> >
> > The firmware fb is a special thing which only really exists as long as the
> > firmware is in charge of the display hardware. As soon as a real driver
> > takes over, it stops being a thing.
> >
> > And since a driver without a device is a bit a funny thing, we have been
> > pushing towards a model where the firmware code sets up a platform_device
> > for this fw interface, and the fw driver (efifb, simplefb and others like
> > that) bind against it. And then we started to throw out that
> > platform_device (which unbinds the fw driver and prevents it from ever
> > rebinding), except in the wrong layer so there's a few races.
> >
> > Should we throw out all that code and replace it with something else? What
> > would that be like?
>
> Ah, no, sorry, I didn't know that at all.
>
> That sounds semi-sane, just fix the races by moving the layer elsewhere?

Yeah essentially move it all into drivers/firmware/sysfb.c, for all
drivers, both the registering and the nuking, and warp that into a
local mutex. Currently parts is in there, parts is in fbmem.c, parts
in some of the drivers like vga16fb, and some drivers (iirc only offb)
still don't even have any platform_dev underneath their driver. So
ideally the drivers would all just have their platform_driver probe
functions, and that's it. It does mean though that some of that stuff
needs to be moved to sysfb.c or into the relevant fw code that sets
stuff up.

It'll take some, so really just a direction check before we move
further. You should get cc'ed on the patches (like with the sysfb
stuff) anyway. Sounds roughly right?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 06/19] fbcon: Use delayed work for cursor
  2022-02-10 11:43   ` Tetsuo Handa
@ 2022-04-05 20:54     ` Daniel Vetter
  0 siblings, 0 replies; 49+ messages in thread
From: Daniel Vetter @ 2022-04-05 20:54 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Daniel Vetter, DRI Development, Intel Graphics Development,
	linux-fbdev, LKML, Daniel Vetter, Daniel Vetter, Claudio Suarez,
	Du Cheng, Thomas Zimmermann, Greg Kroah-Hartman

On Thu, Feb 10, 2022 at 08:43:36PM +0900, Tetsuo Handa wrote:
> On 2022/02/09 6:08, Daniel Vetter wrote:
> > @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
> >  		ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> >  		if (!ops)
> >  			err = -ENOMEM;
> > +
> > +		INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor);
> >  	}
> >  
> >  	if (!err) {
> 
> Memory allocation fault injection will hit NULL pointer dereference.

The error handling here is convoluted and I got this wrong, but a later
patch to extract an fbcon_open() helper fixes it. I'll fix this small
bisect issue for v3 anyway, thanks for taking a look at the patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

* Re: [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered"
  2022-04-05 17:29                         ` Daniel Vetter
@ 2022-04-07 17:26                           ` Greg KH
  0 siblings, 0 replies; 49+ messages in thread
From: Greg KH @ 2022-04-07 17:26 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Geert Uytterhoeven, Javier Martinez Canillas, DRI Development,
	Intel Graphics Development, Linux Fbdev development list, LKML,
	Thomas Zimmermann, Zack Rusin, Hans de Goede, Ilya Trukhanov,
	Daniel Vetter, Peter Jones

On Tue, Apr 05, 2022 at 07:29:22PM +0200, Daniel Vetter wrote:
> On Tue, 5 Apr 2022 at 18:45, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 05, 2022 at 06:12:59PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 05, 2022 at 03:33:17PM +0200, Greg KH wrote:
> > > > On Tue, Apr 05, 2022 at 03:24:40PM +0200, Geert Uytterhoeven wrote:
> > > > > Hi Daniel,
> > > > >
> > > > > On Tue, Apr 5, 2022 at 1:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > On Tue, 5 Apr 2022 at 11:52, Javier Martinez Canillas
> > > > > > <javierm@redhat.com> wrote:
> > > > > > > On 4/5/22 11:24, Daniel Vetter wrote:
> > > > > > > > On Tue, 5 Apr 2022 at 11:19, Javier Martinez Canillas
> > > > > > > >> This is how I think that work, please let me know if you see something
> > > > > > > >> wrong in my logic:
> > > > > > > >>
> > > > > > > >> 1) A PCI device of OF device is registered for the GPU, this attempt to
> > > > > > > >>    match a registered driver but no driver was registered that match yet.
> > > > > > > >>
> > > > > > > >> 2) The efifb driver is built-in, will be initialized according to the link
> > > > > > > >>    order of the objects under drivers/video and the fbdev driver is registered.
> > > > > > > >>
> > > > > > > >>    There is no platform device or PCI/OF device registered that matches.
> > > > > > > >>
> > > > > > > >> 3) The DRM driver is built-in, will be initialized according to the link
> > > > > > > >>    order of the objects under drivers/gpu and the DRM driver is registered.
> > > > > > > >>
> > > > > > > >>    This matches the device registered in (1) and the DRM driver probes.
> > > > > > > >>
> > > > > > > >> 4) The DRM driver .probe kicks out any conflicting DRM drivers and pdev
> > > > > > > >>    before registering the DRM device.
> > > > > > > >>
> > > > > > > >>    There are no conflicting drivers or platform device at this point.
> > > > > > > >>
> > > > > > > >> 5) Latter at some point the drivers/firmware/sysfb.c init function is
> > > > > > > >>    executed, and this registers a platform device for the generic fb.
> > > > > > > >>
> > > > > > > >>    This device matches the efifb driver registered in (2) and the fbdev
> > > > > > > >>    driver probes.
> > > > > > > >>
> > > > > > > >>    Since that happens *after* the DRM driver already matched, probed
> > > > > > > >>    and registered the DRM device, that is a bug and what the reverted
> > > > > > > >>    patch worked around.
> > > > > > > >>
> > > > > > > >> So we need to prevent (5) if (1) and (3) already happened. Having a flag
> > > > > > > >> set in the fbdev core somewhere when remove_conflicting_framebuffers()
> > > > > > > >> is called could be a solution indeed.
> > > > > > > >>
> > > > > > > >> That is, the fbdev core needs to know that a DRM driver already probed
> > > > > > > >> and make register_framebuffer() fail if info->flag & FBINFO_MISC_FIRMWARE
> > > > > > > >>
> > > > > > > >> I can attempt to write a patch for that.
> > > > > > > >
> > > > > > > > Ah yeah that could be an issue. I think the right fix is to replace
> > > > > > > > the platform dev unregister with a sysfb_unregister() function in
> > > > > > > > sysfb.c, which is synced with a common lock with the sysfb_init
> > > > > > > > function and a small boolean. I think I can type that up quickly for
> > > > > > > > v3.
> > > > > > >
> > > > > > > It's more complicated than that since sysfb is just *one* of the several
> > > > > > > places where platform devices can be registered for video devices.
> > > > > > >
> > > > > > > For instance, the vga16fb driver registers its own platform device in
> > > > > > > its module_init() function so that can also happen after the conflicting
> > > > > > > framebuffers (and associated devices) were removed by a DRM driver probe.
> > > > > > >
> > > > > > > I tried to minimize the issue for that particular driver with commit:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0499f419b76f
> > > > > > >
> > > > > > > But the point stands, it all boils down to the fact that you have two
> > > > > > > different subsystems registering video drivers and they don't know all
> > > > > > > about each other to take a proper decision.
> > > > > > >
> > > > > > > Right now the drm_aperture_remove_conflicting_framebuffers() call signals
> > > > > > > in one direction from DRM to fbdev but there isn't a communication in the
> > > > > > > other direction, from fbdev to DRM.
> > > > > > >
> > > > > > > I believe the correct fix would be for the fbdev core to keep a list of
> > > > > > > the apertures struct that are passed to remove_conflicting_framebuffers(),
> > > > > > > that way it will know what apertures are not available anymore and prevent
> > > > > > > to register any fbdev framebuffer that conflicts with one already present.
> > > > > >
> > > > > > Hm that still feels like reinventing a driver model, badly.
> > > > > >
> > > > > > I think there's two cleaner solutions:
> > > > > > - move all the firmware driver platform_dev into sysfb.c, and then
> > > > > > just bind the special cases against that (e.g. offb, vga16fb and all
> > > > > > these). Then we'd have one sysfb_try_unregister(struct device *dev)
> > > > > > interface that fbmem.c uses.
> > > > > > - let fbmem.c call into each of these firmware device providers, which
> > > > > > means some loops most likely (like we can't call into vga16fb), so
> > > > > > probably need to move that into fbmem.c and it all gets a bit messy.
> > > > > >
> > > > > > > Let me know if you think that makes sense and I can attempt to write a fix.
> > > > > >
> > > > > > I still think unregistering the platform_dev properly makes the most
> > > > >
> > > > > That doesn't sound very driver-model-aware to me. The device is what
> > > > > the driver binds to; it does not cease to exist.
> > > >
> > > > I agree, that sounds odd.
> > > >
> > > > The device should always stick around (as the bus creates it), it's up
> > > > to the driver to bind to the device as needed.
> > >
> > > The device actually disappears when the real driver takes over.
> > >
> > > The firmware fb is a special thing which only really exists as long as the
> > > firmware is in charge of the display hardware. As soon as a real driver
> > > takes over, it stops being a thing.
> > >
> > > And since a driver without a device is a bit a funny thing, we have been
> > > pushing towards a model where the firmware code sets up a platform_device
> > > for this fw interface, and the fw driver (efifb, simplefb and others like
> > > that) bind against it. And then we started to throw out that
> > > platform_device (which unbinds the fw driver and prevents it from ever
> > > rebinding), except in the wrong layer so there's a few races.
> > >
> > > Should we throw out all that code and replace it with something else? What
> > > would that be like?
> >
> > Ah, no, sorry, I didn't know that at all.
> >
> > That sounds semi-sane, just fix the races by moving the layer elsewhere?
> 
> Yeah essentially move it all into drivers/firmware/sysfb.c, for all
> drivers, both the registering and the nuking, and warp that into a
> local mutex. Currently parts is in there, parts is in fbmem.c, parts
> in some of the drivers like vga16fb, and some drivers (iirc only offb)
> still don't even have any platform_dev underneath their driver. So
> ideally the drivers would all just have their platform_driver probe
> functions, and that's it. It does mean though that some of that stuff
> needs to be moved to sysfb.c or into the relevant fw code that sets
> stuff up.
> 
> It'll take some, so really just a direction check before we move
> further. You should get cc'ed on the patches (like with the sysfb
> stuff) anyway. Sounds roughly right?

That's fine with me, thanks.

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

end of thread, other threads:[~2022-04-07 17:29 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 21:08 [PATCH v2 00/19] fbcon patches, take two Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 01/19] fbcon: delete a few unneeded forward decl Daniel Vetter
2022-02-10 11:17   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 02/19] fbcon: Move fbcon_bmove(_rec) functions Daniel Vetter
2022-02-08 23:06   ` Javier Martinez Canillas
2022-02-10 11:17   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 03/19] fbcon: Introduce wrapper for console->fb_info lookup Daniel Vetter
2022-02-10 11:18   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 04/19] fbcon: delete delayed loading code Daniel Vetter
2022-02-10 11:20   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 05/19] fbdev/sysfs: Fix locking Daniel Vetter
2022-02-10 11:22   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 06/19] fbcon: Use delayed work for cursor Daniel Vetter
2022-02-08 23:59   ` Javier Martinez Canillas
2022-02-10 11:37   ` Thomas Zimmermann
2022-02-10 11:43   ` Tetsuo Handa
2022-04-05 20:54     ` Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 07/19] fbcon: Replace FBCON_FLAGS_INIT with a boolean Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 08/19] fb: Delete fb_info->queue Daniel Vetter
2022-02-10 11:38   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Daniel Vetter
2022-02-10 11:46   ` Thomas Zimmermann
2022-04-05  8:45     ` Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 10/19] fbcon: Ditch error handling for con2fb_release_oldinfo Daniel Vetter
2022-02-10 14:14   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 11/19] fbcon: move more common code into fb_open() Daniel Vetter
2022-02-10 14:16   ` Thomas Zimmermann
2022-02-08 21:08 ` [PATCH v2 12/19] fbcon: use lock_fb_info in fbcon_open/release Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 13/19] fbcon: Consistently protect deferred_takeover with console_lock() Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 14/19] fbcon: Move console_lock for register/unlink/unregister Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 15/19] fbcon: Move more code into fbcon_release Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 16/19] fbcon: untangle fbcon_exit Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 17/19] fbcon: Maintain a private array of fb_info Daniel Vetter
2022-02-08 21:08 ` [PATCH v2 18/19] Revert "fbdev: Prevent probing generic drivers if a FB is already registered" Daniel Vetter
2022-02-09  0:19   ` Javier Martinez Canillas
2022-04-05  8:36     ` Daniel Vetter
2022-04-05  8:40       ` Daniel Vetter
2022-04-05  9:19         ` Javier Martinez Canillas
2022-04-05  9:24           ` Daniel Vetter
2022-04-05  9:52             ` Javier Martinez Canillas
2022-04-05 10:34               ` Daniel Vetter
2022-04-05 13:24                 ` Geert Uytterhoeven
2022-04-05 13:33                   ` Greg KH
2022-04-05 16:12                     ` Daniel Vetter
2022-04-05 16:44                       ` Greg KH
2022-04-05 17:29                         ` Daniel Vetter
2022-04-07 17:26                           ` Greg KH
2022-04-05 13:25                 ` Javier Martinez Canillas
2022-02-08 21:08 ` [PATCH v2 19/19] fbdev: Make registered_fb[] private to fbmem.c Daniel Vetter

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).