* [PATCH 01/11] fbcon: move update_attr() into separate source file
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 02/11] fbcon: move bit_putcs() " David Herrmann
` (10 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
If we want to use update_attr() independently from fbcon, we need to split
it off from bitblit.c and fbcon.h. Therefore, introduce a new header and
source file (fbdraw.[ch]) which does not depende on vc_* and fbcon_*
structures in any way.
This does not introduce any new code nor does it make the paths deeper,
it simply splits the function off.
The other update_attr() functions (inside the rotation sources) seem
similar but are significantly different and I haven't found a way to merge
them efficiently (which is probably the reason why they are split off
now). Furthermore, I do not intend to use them in the coming code so there
is no need to split them off.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/Makefile | 3 ++-
drivers/video/console/bitblit.c | 26 +++--------------------
drivers/video/console/fbcon.h | 5 +----
drivers/video/console/fbdraw.c | 46 +++++++++++++++++++++++++++++++++++++++++
drivers/video/console/fbdraw.h | 26 +++++++++++++++++++++++
5 files changed, 78 insertions(+), 28 deletions(-)
create mode 100644 drivers/video/console/fbdraw.c
create mode 100644 drivers/video/console/fbdraw.h
diff --git a/drivers/video/console/Makefile b/drivers/video/console/Makefile
index a862e91..9a52226 100644
--- a/drivers/video/console/Makefile
+++ b/drivers/video/console/Makefile
@@ -25,7 +25,8 @@ obj-$(CONFIG_SGI_NEWPORT_CONSOLE) += newport_con.o font.o
obj-$(CONFIG_STI_CONSOLE) += sticon.o sticore.o font.o
obj-$(CONFIG_VGA_CONSOLE) += vgacon.o
obj-$(CONFIG_MDA_CONSOLE) += mdacon.o
-obj-$(CONFIG_FRAMEBUFFER_CONSOLE) += fbcon.o bitblit.o font.o softcursor.o
+obj-$(CONFIG_FRAMEBUFFER_CONSOLE) += fbcon.o bitblit.o font.o softcursor.o \
+ fbdraw.o
ifeq ($(CONFIG_FB_TILEBLITTING),y)
obj-$(CONFIG_FRAMEBUFFER_CONSOLE) += tileblit.o
endif
diff --git a/drivers/video/console/bitblit.c b/drivers/video/console/bitblit.c
index 28b1a83..6ec2905 100644
--- a/drivers/video/console/bitblit.c
+++ b/drivers/video/console/bitblit.c
@@ -22,26 +22,6 @@
/*
* Accelerated handlers.
*/
-static void update_attr(u8 *dst, u8 *src, int attribute,
- struct vc_data *vc)
-{
- int i, offset = (vc->vc_font.height < 10) ? 1 : 2;
- int width = DIV_ROUND_UP(vc->vc_font.width, 8);
- unsigned int cellsize = vc->vc_font.height * width;
- u8 c;
-
- offset = cellsize - (offset * width);
- for (i = 0; i < cellsize; i++) {
- c = src[i];
- if (attribute & FBCON_ATTRIBUTE_UNDERLINE && i >= offset)
- c = 0xff;
- if (attribute & FBCON_ATTRIBUTE_BOLD)
- c |= c >> 1;
- if (attribute & FBCON_ATTRIBUTE_REVERSE)
- c = ~c;
- dst[i] = c;
- }
-}
static void bit_bmove(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int dy, int dx, int height, int width)
@@ -88,7 +68,7 @@ static inline void bit_putcs_aligned(struct vc_data *vc, struct fb_info *info,
charmask)*cellsize;
if (attr) {
- update_attr(buf, src, attr, vc);
+ fbdraw_update_attr(buf, src, attr, &vc->vc_font);
src = buf;
}
@@ -123,7 +103,7 @@ static inline void bit_putcs_unaligned(struct vc_data *vc,
charmask)*cellsize;
if (attr) {
- update_attr(buf, src, attr, vc);
+ fbdraw_update_attr(buf, src, attr, &vc->vc_font);
src = buf;
}
@@ -275,7 +255,7 @@ static void bit_cursor(struct vc_data *vc, struct fb_info *info, int mode,
return;
kfree(ops->cursor_data);
ops->cursor_data = dst;
- update_attr(dst, src, attribute, vc);
+ fbdraw_update_attr(dst, src, attribute, &vc->vc_font);
src = dst;
}
diff --git a/drivers/video/console/fbcon.h b/drivers/video/console/fbcon.h
index 6bd2e0c..8623bac 100644
--- a/drivers/video/console/fbcon.h
+++ b/drivers/video/console/fbcon.h
@@ -16,6 +16,7 @@
#include <linux/vt_kern.h>
#include <asm/io.h>
+#include "fbdraw.h"
#define FBCON_FLAGS_INIT 1
#define FBCON_FLAGS_CURSOR_TIMER 2
@@ -219,10 +220,6 @@ extern void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info);
extern void fbcon_set_bitops(struct fbcon_ops *ops);
extern int soft_cursor(struct fb_info *info, struct fb_cursor *cursor);
-#define FBCON_ATTRIBUTE_UNDERLINE 1
-#define FBCON_ATTRIBUTE_REVERSE 2
-#define FBCON_ATTRIBUTE_BOLD 4
-
static inline int real_y(struct display *p, int ypos)
{
int rows = p->vrows;
diff --git a/drivers/video/console/fbdraw.c b/drivers/video/console/fbdraw.c
new file mode 100644
index 0000000..aa18f7e
--- /dev/null
+++ b/drivers/video/console/fbdraw.c
@@ -0,0 +1,46 @@
+/*
+ * Framebuffer helpers for image draw-operations
+ *
+ * Copyright (c) 2004 Antonino Daplas <adaplas @pol.net>
+ * Copyright (c) 2012 David Herrmann <dh.herrmann@googlemail.com>
+ *
+ * Originally from drivers/video/console/bitblit.c which itself originally is
+ * from the 'accel_*' routines in drivers/video/console/fbcon.c.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/console.h>
+#include <linux/fb.h>
+#include <linux/kd.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include "fbdraw.h"
+
+void fbdraw_update_attr(u8 *dst, const u8 *src, int attribute,
+ struct console_font *font)
+{
+ int i, offset = (font->height < 10) ? 1 : 2;
+ int width = DIV_ROUND_UP(font->width, 8);
+ unsigned int cellsize = font->height * width;
+ u8 c;
+
+ offset = cellsize - (offset * width);
+ for (i = 0; i < cellsize; i++) {
+ c = src[i];
+ if (attribute & FBCON_ATTRIBUTE_UNDERLINE && i >= offset)
+ c = 0xff;
+ if (attribute & FBCON_ATTRIBUTE_BOLD)
+ c |= c >> 1;
+ if (attribute & FBCON_ATTRIBUTE_REVERSE)
+ c = ~c;
+ dst[i] = c;
+ }
+}
+EXPORT_SYMBOL_GPL(fbdraw_update_attr);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@googlemail.com>");
+MODULE_DESCRIPTION("Framebuffer helpers for image draw-operations");
diff --git a/drivers/video/console/fbdraw.h b/drivers/video/console/fbdraw.h
new file mode 100644
index 0000000..77edd7f
--- /dev/null
+++ b/drivers/video/console/fbdraw.h
@@ -0,0 +1,26 @@
+/*
+ * Framebuffer helpers for image draw-operations
+ *
+ * Copyright (c) 2012 David Herrmann <dh.herrmann@googlemail.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#ifndef _VIDEO_FBDRAW_H
+#define _VIDEO_FBDRAW_H
+
+#include <linux/console.h>
+#include <linux/fb.h>
+#include <linux/kd.h>
+
+/* fbcon character attributes */
+#define FBCON_ATTRIBUTE_UNDERLINE 1
+#define FBCON_ATTRIBUTE_REVERSE 2
+#define FBCON_ATTRIBUTE_BOLD 4
+
+void fbdraw_update_attr(u8 *dst, const u8 *src, int attribute,
+ struct console_font *font);
+
+#endif /* _VIDEO_FBDRAW_H */
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 02/11] fbcon: move bit_putcs() into separate source file
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
2012-08-12 14:53 ` [PATCH 01/11] fbcon: move update_attr() into separate source file David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 03/11] fblog: new framebuffer kernel log dummy driver David Herrmann
` (9 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
If we want to use font-draw-operations in other modules than fbcon, we
need to split this function off of fbcon headers and sources. This also
makes bit_putcs() totally independent of vc_* and fbcon_* structures.
As scr_read() cannot be called inside of non-fbcon/vt functions, we need
to assemble the buffer before passing it to fbdraw_font(). This slows down
this operations a little bit but my rough benchmark showed that it didn't
really matter. Anyway, if it does, we can still put it into VT_BUF_HAVE_RW
conditions so platforms that don't use it won't be affected. And for other
platforms we can add the buffer to the vc-struct so we can reuse it.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/bitblit.c | 127 ++++------------------------------------
drivers/video/console/fbdraw.c | 125 +++++++++++++++++++++++++++++++++++++++
drivers/video/console/fbdraw.h | 4 ++
3 files changed, 139 insertions(+), 117 deletions(-)
diff --git a/drivers/video/console/bitblit.c b/drivers/video/console/bitblit.c
index 6ec2905..c5d897b 100644
--- a/drivers/video/console/bitblit.c
+++ b/drivers/video/console/bitblit.c
@@ -54,132 +54,25 @@ static void bit_clear(struct vc_data *vc, struct fb_info *info, int sy,
info->fbops->fb_fillrect(info, ®ion);
}
-static inline void bit_putcs_aligned(struct vc_data *vc, struct fb_info *info,
- const u16 *s, u32 attr, u32 cnt,
- u32 d_pitch, u32 s_pitch, u32 cellsize,
- struct fb_image *image, u8 *buf, u8 *dst)
-{
- u16 charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
- u32 idx = vc->vc_font.width >> 3;
- u8 *src;
-
- while (cnt--) {
- src = vc->vc_font.data + (scr_readw(s++)&
- charmask)*cellsize;
-
- if (attr) {
- fbdraw_update_attr(buf, src, attr, &vc->vc_font);
- src = buf;
- }
-
- if (likely(idx == 1))
- __fb_pad_aligned_buffer(dst, d_pitch, src, idx,
- image->height);
- else
- fb_pad_aligned_buffer(dst, d_pitch, src, idx,
- image->height);
-
- dst += s_pitch;
- }
-
- info->fbops->fb_imageblit(info, image);
-}
-
-static inline void bit_putcs_unaligned(struct vc_data *vc,
- struct fb_info *info, const u16 *s,
- u32 attr, u32 cnt, u32 d_pitch,
- u32 s_pitch, u32 cellsize,
- struct fb_image *image, u8 *buf,
- u8 *dst)
-{
- u16 charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
- u32 shift_low = 0, mod = vc->vc_font.width % 8;
- u32 shift_high = 8;
- u32 idx = vc->vc_font.width >> 3;
- u8 *src;
-
- while (cnt--) {
- src = vc->vc_font.data + (scr_readw(s++)&
- charmask)*cellsize;
-
- if (attr) {
- fbdraw_update_attr(buf, src, attr, &vc->vc_font);
- src = buf;
- }
-
- fb_pad_unaligned_buffer(dst, d_pitch, src, idx,
- image->height, shift_high,
- shift_low, mod);
- shift_low += mod;
- dst += (shift_low >= 8) ? s_pitch : s_pitch - 1;
- shift_low &= 7;
- shift_high = 8 - shift_low;
- }
-
- info->fbops->fb_imageblit(info, image);
-
-}
-
static void bit_putcs(struct vc_data *vc, struct fb_info *info,
const unsigned short *s, int count, int yy, int xx,
int fg, int bg)
{
- struct fb_image image;
- u32 width = DIV_ROUND_UP(vc->vc_font.width, 8);
- u32 cellsize = width * vc->vc_font.height;
- u32 maxcnt = info->pixmap.size/cellsize;
- u32 scan_align = info->pixmap.scan_align - 1;
- u32 buf_align = info->pixmap.buf_align - 1;
- u32 mod = vc->vc_font.width % 8, cnt, pitch, size;
+ u16 *buf;
+ int i;
u32 attribute = get_attribute(info, scr_readw(s));
- u8 *dst, *buf = NULL;
- image.fg_color = fg;
- image.bg_color = bg;
- image.dx = xx * vc->vc_font.width;
- image.dy = yy * vc->vc_font.height;
- image.height = vc->vc_font.height;
- image.depth = 1;
+ buf = kmalloc(sizeof(*buf) * count, GFP_KERNEL);
+ if (!buf)
+ return;
- if (attribute) {
- buf = kmalloc(cellsize, GFP_KERNEL);
- if (!buf)
- return;
- }
-
- while (count) {
- if (count > maxcnt)
- cnt = maxcnt;
- else
- cnt = count;
-
- image.width = vc->vc_font.width * cnt;
- pitch = DIV_ROUND_UP(image.width, 8) + scan_align;
- pitch &= ~scan_align;
- size = pitch * image.height + buf_align;
- size &= ~buf_align;
- dst = fb_get_buffer_offset(info, &info->pixmap, size);
- image.data = dst;
-
- if (!mod)
- bit_putcs_aligned(vc, info, s, attribute, cnt, pitch,
- width, cellsize, &image, buf, dst);
- else
- bit_putcs_unaligned(vc, info, s, attribute, cnt,
- pitch, width, cellsize, &image,
- buf, dst);
-
- image.dx += cnt * vc->vc_font.width;
- count -= cnt;
- s += cnt;
- }
+ for (i = 0; i < count; ++i)
+ buf[i] = scr_readw(s++);
- /* buf is always NULL except when in monochrome mode, so in this case
- it's a gain to check buf against NULL even though kfree() handles
- NULL pointers just fine */
- if (unlikely(buf))
- kfree(buf);
+ fbdraw_font(info, &vc->vc_font, vc->vc_hi_font_mask, xx, yy, fg, bg,
+ attribute, buf, count);
+ kfree(buf);
}
static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
diff --git a/drivers/video/console/fbdraw.c b/drivers/video/console/fbdraw.c
index aa18f7e..f7c3da7 100644
--- a/drivers/video/console/fbdraw.c
+++ b/drivers/video/console/fbdraw.c
@@ -41,6 +41,131 @@ void fbdraw_update_attr(u8 *dst, const u8 *src, int attribute,
}
EXPORT_SYMBOL_GPL(fbdraw_update_attr);
+static inline void bit_putcs_aligned(struct fb_info *info, bool hi_font,
+ struct console_font *font, u32 attribute,
+ u32 d_pitch, u32 s_pitch, u32 cellsize,
+ struct fb_image *image, u8 *buf, u8 *dst,
+ const u16 *chars, size_t cnt)
+{
+ u16 charmask = hi_font ? 0x1ff : 0xff;
+ u32 idx = font->width >> 3;
+ u8 *src;
+
+ while (cnt--) {
+ src = font->data + ((*chars++) & charmask) * cellsize;
+
+ if (attribute) {
+ fbdraw_update_attr(buf, src, attribute, font);
+ src = buf;
+ }
+
+ if (likely(idx == 1))
+ __fb_pad_aligned_buffer(dst, d_pitch, src, idx,
+ image->height);
+ else
+ fb_pad_aligned_buffer(dst, d_pitch, src, idx,
+ image->height);
+
+ dst += s_pitch;
+ }
+
+ info->fbops->fb_imageblit(info, image);
+}
+
+static inline void bit_putcs_unaligned(struct fb_info *info, bool hi_font,
+ struct console_font *font, u32 attribute,
+ u32 d_pitch, u32 s_pitch, u32 cellsize,
+ struct fb_image *image, u8 *buf, u8 *dst,
+ const u16 *chars, size_t cnt)
+{
+ u16 charmask = hi_font ? 0x1ff : 0xff;
+ u32 shift_low = 0, mod = font->width % 8;
+ u32 shift_high = 8;
+ u32 idx = font->width >> 3;
+ u8 *src;
+
+ while (cnt--) {
+ src = font->data + ((*chars++) & charmask) * cellsize;
+
+ if (attribute) {
+ fbdraw_update_attr(buf, src, attribute, font);
+ src = buf;
+ }
+
+ fb_pad_unaligned_buffer(dst, d_pitch, src, idx,
+ image->height, shift_high,
+ shift_low, mod);
+ shift_low += mod;
+ dst += (shift_low >= 8) ? s_pitch : s_pitch - 1;
+ shift_low &= 7;
+ shift_high = 8 - shift_low;
+ }
+
+ info->fbops->fb_imageblit(info, image);
+}
+
+void fbdraw_font(struct fb_info *info, struct console_font *font, bool hi_font,
+ unsigned int xpos, unsigned int ypos, int fg, int bg,
+ u32 attribute, const u16 *chars, size_t count)
+{
+ struct fb_image image;
+ u32 width = DIV_ROUND_UP(font->width, 8);
+ u32 cellsize = width * font->height;
+ u32 maxcnt = info->pixmap.size / cellsize;
+ u32 scan_align = info->pixmap.scan_align - 1;
+ u32 buf_align = info->pixmap.buf_align - 1;
+ u32 mod = font->width % 8, cnt, pitch, size;
+ u8 *dst, *buf = NULL;
+
+ image.fg_color = fg;
+ image.bg_color = bg;
+ image.dx = xpos * font->width;
+ image.dy = ypos * font->height;
+ image.height = font->height;
+ image.depth = 1;
+
+ if (attribute) {
+ buf = kmalloc(cellsize, GFP_KERNEL);
+ if (!buf)
+ return;
+ }
+
+ while (count) {
+ if (count > maxcnt)
+ cnt = maxcnt;
+ else
+ cnt = count;
+
+ image.width = font->width * cnt;
+ pitch = DIV_ROUND_UP(image.width, 8) + scan_align;
+ pitch &= ~scan_align;
+ size = pitch * image.height + buf_align;
+ size &= ~buf_align;
+ dst = fb_get_buffer_offset(info, &info->pixmap, size);
+ image.data = dst;
+
+ if (!mod)
+ bit_putcs_aligned(info, hi_font, font, attribute,
+ pitch, width, cellsize, &image, buf,
+ dst, chars, cnt);
+ else
+ bit_putcs_unaligned(info, hi_font, font, attribute,
+ pitch, width, cellsize, &image, buf,
+ dst, chars, cnt);
+
+ image.dx += cnt * font->width;
+ count -= cnt;
+ chars += cnt;
+ }
+
+ /* buf is always NULL except when in monochrome mode, so in this case
+ it's a gain to check buf against NULL even though kfree() handles
+ NULL pointers just fine */
+ if (unlikely(buf))
+ kfree(buf);
+}
+EXPORT_SYMBOL_GPL(fbdraw_font);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Herrmann <dh.herrmann@googlemail.com>");
MODULE_DESCRIPTION("Framebuffer helpers for image draw-operations");
diff --git a/drivers/video/console/fbdraw.h b/drivers/video/console/fbdraw.h
index 77edd7f..b9f1ffa 100644
--- a/drivers/video/console/fbdraw.h
+++ b/drivers/video/console/fbdraw.h
@@ -23,4 +23,8 @@
void fbdraw_update_attr(u8 *dst, const u8 *src, int attribute,
struct console_font *font);
+void fbdraw_font(struct fb_info *info, struct console_font *font, bool hi_font,
+ unsigned int xpos, unsigned int ypos, int fg, int bg,
+ u32 attribute, const u16 *chars, size_t count);
+
#endif /* _VIDEO_FBDRAW_H */
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
2012-08-12 14:53 ` [PATCH 01/11] fbcon: move update_attr() into separate source file David Herrmann
2012-08-12 14:53 ` [PATCH 02/11] fbcon: move bit_putcs() " David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 23:34 ` Ryan Mallon
2012-08-12 14:53 ` [PATCH 04/11] fbdev: export get_fb_info()/put_fb_info() David Herrmann
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
Fblog displays all kernel log messages on all connected framebuffers. It
replaces fbcon when CONFIG_VT=n is selected. Its main purpose is to debug
boot problems by displaying the whole boot log on the screen. This patch
provides the first dummy module-init/deinit functions.
As it uses all the font and fb functions I placed it in
drivers/video/console. However, this means that we need to move the check
for CONFIG_VT in Makefile/Kconfig from drivers/video into
drivers/video/console as fblog does not depend on CONFIG_VT.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/Kconfig | 5 +----
drivers/video/Makefile | 2 +-
drivers/video/console/Kconfig | 37 +++++++++++++++++++++++++++++--------
drivers/video/console/Makefile | 1 +
drivers/video/console/fblog.c | 41 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 73 insertions(+), 13 deletions(-)
create mode 100644 drivers/video/console/fblog.c
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0217f74..e8fd53d 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2448,10 +2448,7 @@ source "drivers/video/omap/Kconfig"
source "drivers/video/omap2/Kconfig"
source "drivers/video/exynos/Kconfig"
source "drivers/video/backlight/Kconfig"
-
-if VT
- source "drivers/video/console/Kconfig"
-endif
+source "drivers/video/console/Kconfig"
if FB || SGI_NEWPORT_CONSOLE
source "drivers/video/logo/Kconfig"
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index ee8dafb..9f8a7f0 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -11,7 +11,7 @@ fb-y := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
modedb.o fbcvt.o
fb-objs := $(fb-y)
-obj-$(CONFIG_VT) += console/
+obj-y += console/
obj-$(CONFIG_LOGO) += logo/
obj-y += backlight/
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index e2c96d0..7374362 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -6,7 +6,7 @@ menu "Console display driver support"
config VGA_CONSOLE
bool "VGA text console" if EXPERT || !X86
- depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
+ depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
default y
help
Saying Y here will allow you to use Linux in text mode through a
@@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
screenfuls of scrollback buffer
config MDA_CONSOLE
- depends on !M68K && !PARISC && ISA
+ depends on VT && !M68K && !PARISC && ISA
tristate "MDA text console (dual-headed) (EXPERIMENTAL)"
---help---
Say Y here if you have an old MDA or monochrome Hercules graphics
@@ -61,14 +61,14 @@ config MDA_CONSOLE
config SGI_NEWPORT_CONSOLE
tristate "SGI Newport Console support"
- depends on SGI_IP22
+ depends on VT && SGI_IP22
help
Say Y here if you want the console on the Newport aka XL graphics
card of your Indy. Most people say Y here.
config DUMMY_CONSOLE
bool
- depends on VGA_CONSOLE!=y || SGI_NEWPORT_CONSOLE!=y
+ depends on VT && (VGA_CONSOLE!=y || SGI_NEWPORT_CONSOLE!=y)
default y
config DUMMY_CONSOLE_COLUMNS
@@ -89,7 +89,7 @@ config DUMMY_CONSOLE_ROWS
config FRAMEBUFFER_CONSOLE
tristate "Framebuffer Console support"
- depends on FB
+ depends on VT && FB
select CRC32
help
Low-level framebuffer-based console driver.
@@ -122,16 +122,37 @@ config FRAMEBUFFER_CONSOLE_ROTATION
config STI_CONSOLE
bool "STI text console"
- depends on PARISC
+ depends on VT && PARISC
default y
help
The STI console is the builtin display/keyboard on HP-PARISC
machines. Say Y here to build support for it into your kernel.
The alternative is to use your primary serial port as a console.
+config FBLOG
+ tristate "Framebuffer Kernel Log Driver"
+ depends on !VT && FB
+ default n
+ help
+ This driver displays all kernel log messages on all connected
+ framebuffers. It is mutually exclusive with CONFIG_FRAMEBUFFER_CONSOLE
+ and CONFIG_VT. It was mainly created for debugging purposes when
+ CONFIG_VT is not selected but you still want kernel boot messages on
+ the screen.
+
+ This driver overwrites all other graphics output on the framebuffer as
+ long as it is active so the kernel log will always be visible. You
+ need to disable this driver via sysfs to be able to start another
+ graphics application.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module will
+ be called fblog.
+
config FONTS
bool "Select compiled-in fonts"
- depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE
+ depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || FBLOG
help
Say Y here if you would like to use fonts other than the default
your frame buffer console usually use.
@@ -158,7 +179,7 @@ config FONT_8x8
config FONT_8x16
bool "VGA 8x16 font" if FONTS
- depends on FRAMEBUFFER_CONSOLE || SGI_NEWPORT_CONSOLE || STI_CONSOLE || USB_SISUSBVGA_CON
+ depends on FRAMEBUFFER_CONSOLE || SGI_NEWPORT_CONSOLE || STI_CONSOLE || USB_SISUSBVGA_CON || FBLOG
default y if !SPARC && !FONTS
help
This is the "high resolution" font for the VGA frame buffer (the one
diff --git a/drivers/video/console/Makefile b/drivers/video/console/Makefile
index 9a52226..ec0e155 100644
--- a/drivers/video/console/Makefile
+++ b/drivers/video/console/Makefile
@@ -20,6 +20,7 @@ font-objs += $(font-objs-y)
# Each configuration option enables a list of files.
+obj-$(CONFIG_FBLOG) += fblog.o font.o fbdraw.o
obj-$(CONFIG_DUMMY_CONSOLE) += dummycon.o
obj-$(CONFIG_SGI_NEWPORT_CONSOLE) += newport_con.o font.o
obj-$(CONFIG_STI_CONSOLE) += sticon.o sticore.o font.o
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
new file mode 100644
index 0000000..fb39737
--- /dev/null
+++ b/drivers/video/console/fblog.c
@@ -0,0 +1,41 @@
+/*
+ * Framebuffer Kernel Log Driver
+ * Copyright (c) 2012 David Herrmann <dh.herrmann@googlemail.com>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+/*
+ * Framebuffer Kernel Log
+ * This driver prints the kernel log to all connected display devices. It
+ * replaces CONFIG_VT and cannot run simultaneously with it. It does not provide
+ * any virtual-terminal, though. It should only be used to get kernel boot
+ * messages to debug kernel errors.
+ * Hence, this driver is neither optimized for speed, nor does it provide any
+ * fancy features like colored text output.
+ * This driver forcibly writes to the framebuffer while active, therefore, you
+ * cannot run other graphics applications simultaneously. You need to disable
+ * all fblog instances before running other graphics applications.
+ */
+
+#include <linux/module.h>
+
+static int __init fblog_init(void)
+{
+ return 0;
+}
+
+static void __exit fblog_exit(void)
+{
+}
+
+module_init(fblog_init);
+module_exit(fblog_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@googlemail.com>");
+MODULE_DESCRIPTION("Framebuffer Kernel Log Driver");
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
2012-08-12 14:53 ` [PATCH 03/11] fblog: new framebuffer kernel log dummy driver David Herrmann
@ 2012-08-12 23:34 ` Ryan Mallon
2012-08-14 9:42 ` David Herrmann
0 siblings, 1 reply; 22+ messages in thread
From: Ryan Mallon @ 2012-08-12 23:34 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
On 13/08/12 00:53, David Herrmann wrote:
> Fblog displays all kernel log messages on all connected framebuffers. It
> replaces fbcon when CONFIG_VT=n is selected. Its main purpose is to debug
> boot problems by displaying the whole boot log on the screen. This patch
> provides the first dummy module-init/deinit functions.
>
> As it uses all the font and fb functions I placed it in
> drivers/video/console. However, this means that we need to move the check
> for CONFIG_VT in Makefile/Kconfig from drivers/video into
> drivers/video/console as fblog does not depend on CONFIG_VT.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Hi David,
Couple of comments below.
~Ryan
> config VGA_CONSOLE
> bool "VGA text console" if EXPERT || !X86
> - depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> + depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> default y
> help
> Saying Y here will allow you to use Linux in text mode through a
> @@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> screenfuls of scrollback buffer
You could just place all of the CONFIG_VT options inside an if VT and
then the new fblog options in the else case rather than modifying all of
the (already overly long) depends statements.
> +config FBLOG
> + tristate "Framebuffer Kernel Log Driver"
> + depends on !VT && FB
> + default n
Default n is not required, all options are default n unless otherwise
specified. I don't think you need the dependency on FB either, since
this file should only get included if CONFIG_FB is set?
> + help
> + This driver displays all kernel log messages on all connected
> + framebuffers. It is mutually exclusive with CONFIG_FRAMEBUFFER_CONSOLE
> + and CONFIG_VT. It was mainly created for debugging purposes when
> + CONFIG_VT is not selected but you still want kernel boot messages on
> + the screen.
Do command line options exist to specify screens to write the debug log
to? It might be a useful feature to have.
~Ryan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
2012-08-12 23:34 ` Ryan Mallon
@ 2012-08-14 9:42 ` David Herrmann
0 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-14 9:42 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
Hi Ryan
On Mon, Aug 13, 2012 at 1:34 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> > config VGA_CONSOLE
> > bool "VGA text console" if EXPERT || !X86
> > - depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV &&
> > !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE ||
> > ARCH_INTEGRATOR || ARCH_NETWINDER)
> > + depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC &&
> > !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM ||
> > ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> > default y
> > help
> > Saying Y here will allow you to use Linux in text mode through a
> > @@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> > screenfuls of scrollback buffer
>
> You could just place all of the CONFIG_VT options inside an if VT and
> then the new fblog options in the else case rather than modifying all of
> the (already overly long) depends statements.
Indeed, that sounds better.
> > +config FBLOG
> > + tristate "Framebuffer Kernel Log Driver"
> > + depends on !VT && FB
> > + default n
>
> Default n is not required, all options are default n unless otherwise
> specified. I don't think you need the dependency on FB either, since
> this file should only get included if CONFIG_FB is set?
I will remove the default-line, but the FB-dependency is required. The
"console/" directory includes also drivers like vgacon which do not
depend on FB so it gets always included.
> > + help
> > + This driver displays all kernel log messages on all connected
> > + framebuffers. It is mutually exclusive with
> > CONFIG_FRAMEBUFFER_CONSOLE
> > + and CONFIG_VT. It was mainly created for debugging purposes when
> > + CONFIG_VT is not selected but you still want kernel boot
> > messages on
> > + the screen.
>
> Do command line options exist to specify screens to write the debug log
> to? It might be a useful feature to have.
See patch-7. However, I am not entirely happy with it. I will probably
have to rethink the parameters.
Thanks for reviewing. I will fix all these in the next series.
Regards
David
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 04/11] fbdev: export get_fb_info()/put_fb_info()
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (2 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 03/11] fblog: new framebuffer kernel log dummy driver David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 05/11] fblog: register one fblog object per framebuffer David Herrmann
` (7 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
When adding other internal users of the framebuffer subsystem, we need a
way to get references to framebuffers. These two functions already exist
so export them.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/fbmem.c | 6 ++++--
include/linux/fb.h | 3 +++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 0dff12a..1312ba2 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -46,7 +46,7 @@ static DEFINE_MUTEX(registration_lock);
struct fb_info *registered_fb[FB_MAX] __read_mostly;
int num_registered_fb __read_mostly;
-static struct fb_info *get_fb_info(unsigned int idx)
+struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -61,14 +61,16 @@ static struct fb_info *get_fb_info(unsigned int idx)
return fb_info;
}
+EXPORT_SYMBOL_GPL(get_fb_info);
-static void put_fb_info(struct fb_info *fb_info)
+void put_fb_info(struct fb_info *fb_info)
{
if (!atomic_dec_and_test(&fb_info->count))
return;
if (fb_info->fbops->fb_destroy)
fb_info->fbops->fb_destroy(fb_info);
}
+EXPORT_SYMBOL_GPL(put_fb_info);
int lock_fb_info(struct fb_info *info)
{
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ac3f1c6..2d51c0e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -1033,6 +1033,9 @@ static inline void unlock_fb_info(struct fb_info *info)
mutex_unlock(&info->lock);
}
+extern struct fb_info *get_fb_info(unsigned int idx);
+extern void put_fb_info(struct fb_info *fb_info);
+
static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
u8 *src, u32 s_pitch, u32 height)
{
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 05/11] fblog: register one fblog object per framebuffer
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (3 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 04/11] fbdev: export get_fb_info()/put_fb_info() David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 23:54 ` Ryan Mallon
2012-08-12 14:53 ` [PATCH 06/11] fblog: open fb on registration David Herrmann
` (6 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
One fblog object is associated to each registered framebuffer. This way,
we can draw the console to each framebuffer. When a framebuffer driver
unregisters a framebuffer, we also unregister our fblog object. That is,
our lifetime is coupled to the lifetime of the framebuffer. However, this
does not mean that we are always active. On the contrary, we do not even
own a reference to the framebuffer. We don't need it as we are notified
_before_ the last reference is dropped.
However, if other users have a reference to our object, we simply mark it
as dead when the associated framebuffer dies and leave it alone. When the
last reference is dropped, it will be automatically freed.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 195 insertions(+)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index fb39737..279f4d8 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -23,15 +23,210 @@
* all fblog instances before running other graphics applications.
*/
+#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
+
+#include <linux/device.h>
+#include <linux/fb.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+
+enum fblog_flags {
+ FBLOG_KILLED,
+};
+
+struct fblog_fb {
+ unsigned long flags;
+ struct fb_info *info;
+ struct device dev;
+};
+
+static DEFINE_MUTEX(fblog_registration_lock);
+static struct fblog_fb *fblog_fbs[FB_MAX];
+
+#define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
+
+/*
+ * fblog framebuffer list
+ * The fblog_fbs[] array contains all currently registered framebuffers. If a
+ * framebuffer is in that list, we always must make sure that we own a reference
+ * to it. If it is added through the notifier callbacks, then this is always
+ * guaranteed.
+ * We are only interested in registered framebuffers. That is, if a driver calls
+ * unregister_framebuffer() we directly unlink it from our list. This guarantees
+ * that the associated fb_info is always valid. However, we might still have
+ * pending users so we mark it as dead so no further framebuffer actions are
+ * done. If the last user then drops a reference, the memory gets freed
+ * automatically.
+ */
+
+static void fblog_release(struct device *dev)
+{
+ struct fblog_fb *fb = to_fblog_dev(dev);
+
+ kfree(fb);
+ module_put(THIS_MODULE);
+}
+
+static void fblog_do_unregister(struct fb_info *info)
+{
+ struct fblog_fb *fb;
+
+ fb = fblog_fbs[info->node];
+ if (!fb || fb->info != info)
+ return;
+
+ fblog_fbs[info->node] = NULL;
+
+ device_del(&fb->dev);
+ put_device(&fb->dev);
+}
+
+static void fblog_do_register(struct fb_info *info, bool force)
+{
+ struct fblog_fb *fb;
+ int ret;
+
+ fb = fblog_fbs[info->node];
+ if (fb && fb->info != info) {
+ if (!force)
+ return;
+
+ fblog_do_unregister(fb->info);
+ }
+
+ fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+ if (!fb)
+ return;
+
+ fb->info = info;
+ __module_get(THIS_MODULE);
+ device_initialize(&fb->dev);
+ fb->dev.class = fb_class;
+ fb->dev.release = fblog_release;
+ dev_set_name(&fb->dev, "fblog%d", info->node);
+ fblog_fbs[info->node] = fb;
+
+ ret = device_add(&fb->dev);
+ if (ret) {
+ fblog_fbs[info->node] = NULL;
+ set_bit(FBLOG_KILLED, &fb->flags);
+ put_device(&fb->dev);
+ return;
+ }
+}
+
+static void fblog_register(struct fb_info *info, bool force)
+{
+ mutex_lock(&fblog_registration_lock);
+ fblog_do_register(info, force);
+ mutex_unlock(&fblog_registration_lock);
+}
+
+static void fblog_unregister(struct fb_info *info)
+{
+ mutex_lock(&fblog_registration_lock);
+ fblog_do_unregister(info);
+ mutex_unlock(&fblog_registration_lock);
+}
+
+static int fblog_event(struct notifier_block *self, unsigned long action,
+ void *data)
+{
+ struct fb_event *event = data;
+ struct fb_info *info = event->info;
+
+ switch(action) {
+ case FB_EVENT_FB_REGISTERED:
+ /* This is called when a low-level system driver registers a new
+ * framebuffer. The registration lock is held but the console
+ * lock might not be held when this is called. */
+ fblog_register(info, true);
+ break;
+ case FB_EVENT_FB_UNREGISTERED:
+ /* This is called when a low-level system driver unregisters a
+ * framebuffer. The registration lock is held but the console
+ * lock might not be held. */
+ fblog_unregister(info);
+ break;
+ }
+
+ return 0;
+}
+
+static void fblog_scan(void)
+{
+ unsigned int i;
+ struct fb_info *info, *tmp;
+
+ for (i = 0; i < FB_MAX; ++i) {
+ info = get_fb_info(i);
+ if (!info || IS_ERR(info))
+ continue;
+
+ fblog_register(info, false);
+
+ /* There is a very subtle race-condition. Even though we might
+ * own a reference to the fb, it may still get unregistered
+ * between our call from get_fb_info() and fblog_register().
+ * Therefore, we simply check whether the same fb still is
+ * registered by calling get_fb_info() again. Only if they
+ * differ we know that it got unregistered, therefore, we
+ * call fblog_unregister() with the old pointer. */
+
+ tmp = get_fb_info(i);
+ if (tmp && !IS_ERR(tmp))
+ put_fb_info(tmp);
+ if (tmp != info)
+ fblog_unregister(info);
+
+ /* Here we either called fblog_unregister() and therefore do not
+ * need any reference to the fb, or we can be sure that the FB
+ * is registered and FB_EVENT_FB_UNREGISTERED will be called
+ * before the last reference is dropped. Hence, we can drop our
+ * reference here. */
+
+ put_fb_info(info);
+ }
+}
+
+static struct notifier_block fblog_notifier = {
+ .notifier_call = fblog_event,
+};
static int __init fblog_init(void)
{
+ int ret;
+
+ ret = fb_register_client(&fblog_notifier);
+ if (ret) {
+ pr_err("cannot register framebuffer notifier\n");
+ return ret;
+ }
+
+ fblog_scan();
+
return 0;
}
static void __exit fblog_exit(void)
{
+ unsigned int i;
+ struct fb_info *info;
+
+ fb_unregister_client(&fblog_notifier);
+
+ /* We scan through the whole registered_fb array here instead of
+ * fblog_fbs because we need to get the device lock _before_ the
+ * fblog-registration-lock. */
+
+ for (i = 0; i < FB_MAX; ++i) {
+ info = get_fb_info(i);
+ if (!info || IS_ERR(info))
+ continue;
+
+ fblog_unregister(info);
+ put_fb_info(info);
+ }
}
module_init(fblog_init);
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
2012-08-12 14:53 ` [PATCH 05/11] fblog: register one fblog object per framebuffer David Herrmann
@ 2012-08-12 23:54 ` Ryan Mallon
2012-08-14 11:01 ` David Herrmann
0 siblings, 1 reply; 22+ messages in thread
From: Ryan Mallon @ 2012-08-12 23:54 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
On 13/08/12 00:53, David Herrmann wrote:
> One fblog object is associated to each registered framebuffer. This way,
> we can draw the console to each framebuffer. When a framebuffer driver
> unregisters a framebuffer, we also unregister our fblog object. That is,
> our lifetime is coupled to the lifetime of the framebuffer. However, this
> does not mean that we are always active. On the contrary, we do not even
> own a reference to the framebuffer. We don't need it as we are notified
> _before_ the last reference is dropped.
>
> However, if other users have a reference to our object, we simply mark it
> as dead when the associated framebuffer dies and leave it alone. When the
> last reference is dropped, it will be automatically freed.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Hi David,
Quick review below.
Thanks,
~Ryan
> ---
> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 195 insertions(+)
>
> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> index fb39737..279f4d8 100644
> --- a/drivers/video/console/fblog.c
> +++ b/drivers/video/console/fblog.c
> @@ -23,15 +23,210 @@
> * all fblog instances before running other graphics applications.
> */
>
> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
> +
> +#include <linux/device.h>
> +#include <linux/fb.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +enum fblog_flags {
> + FBLOG_KILLED,
> +};
> +
> +struct fblog_fb {
> + unsigned long flags;
Are more flags added in later patches? If not, why not just have:
bool is_killed;
?
> + struct fb_info *info;
> + struct device dev;
> +};
> +
> +static DEFINE_MUTEX(fblog_registration_lock);
> +static struct fblog_fb *fblog_fbs[FB_MAX];
> +
> +#define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
> +
> +/*
> + * fblog framebuffer list
> + * The fblog_fbs[] array contains all currently registered framebuffers. If a
> + * framebuffer is in that list, we always must make sure that we own a reference
> + * to it. If it is added through the notifier callbacks, then this is always
> + * guaranteed.
> + * We are only interested in registered framebuffers. That is, if a driver calls
> + * unregister_framebuffer() we directly unlink it from our list. This guarantees
> + * that the associated fb_info is always valid. However, we might still have
> + * pending users so we mark it as dead so no further framebuffer actions are
> + * done. If the last user then drops a reference, the memory gets freed
> + * automatically.
> + */
> +
> +static void fblog_release(struct device *dev)
> +{
> + struct fblog_fb *fb = to_fblog_dev(dev);
> +
> + kfree(fb);
> + module_put(THIS_MODULE);
> +}
> +
> +static void fblog_do_unregister(struct fb_info *info)
> +{
> + struct fblog_fb *fb;
> +
> + fb = fblog_fbs[info->node];
> + if (!fb || fb->info != info)
> + return;
> +
> + fblog_fbs[info->node] = NULL;
> +
> + device_del(&fb->dev);
> + put_device(&fb->dev);
device_unregister?
> +}
> +
> +static void fblog_do_register(struct fb_info *info, bool force)
> +{
> + struct fblog_fb *fb;
> + int ret;
> +
> + fb = fblog_fbs[info->node];
> + if (fb && fb->info != info) {
> + if (!force)
> + return;
> +
> + fblog_do_unregister(fb->info);
> + }
> +
> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> + if (!fb)
> + return;
> +
> + fb->info = info;
> + __module_get(THIS_MODULE);
> + device_initialize(&fb->dev);
> + fb->dev.class = fb_class;
> + fb->dev.release = fblog_release;
> + dev_set_name(&fb->dev, "fblog%d", info->node);
> + fblog_fbs[info->node] = fb;
> +
> + ret = device_add(&fb->dev);
> + if (ret) {
> + fblog_fbs[info->node] = NULL;
> + set_bit(FBLOG_KILLED, &fb->flags);
> + put_device(&fb->dev);
kfree(fb); ?
> + return;
> + }
> +}
> +
> +static void fblog_register(struct fb_info *info, bool force)
> +{
> + mutex_lock(&fblog_registration_lock);
> + fblog_do_register(info, force);
> + mutex_unlock(&fblog_registration_lock);
> +}
> +
> +static void fblog_unregister(struct fb_info *info)
> +{
> + mutex_lock(&fblog_registration_lock);
> + fblog_do_unregister(info);
> + mutex_unlock(&fblog_registration_lock);
> +}
This locking is needlessly heavy, and could easily pushed down into the
fb_do_(un)register functions. It would also help make it clear exactly
what the lock is protecting.
> +static int fblog_event(struct notifier_block *self, unsigned long action,
> + void *data)
> +{
> + struct fb_event *event = data;
> + struct fb_info *info = event->info;
> +
> + switch(action) {
> + case FB_EVENT_FB_REGISTERED:
> + /* This is called when a low-level system driver registers a new
> + * framebuffer. The registration lock is held but the console
> + * lock might not be held when this is called. */
Nitpick:
/*
* The Linux kernel multi-line
* comment style looks like
* this.
*/
> + fblog_register(info, true);
> + break;
> + case FB_EVENT_FB_UNREGISTERED:
> + /* This is called when a low-level system driver unregisters a
> + * framebuffer. The registration lock is held but the console
> + * lock might not be held. */
> + fblog_unregister(info);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void fblog_scan(void)
> +{
> + unsigned int i;
> + struct fb_info *info, *tmp;
> +
> + for (i = 0; i < FB_MAX; ++i) {
> + info = get_fb_info(i);
> + if (!info || IS_ERR(info))
Nitpick:
if (IS_ERR_OR_NULL(info))
> + continue;
> +
> + fblog_register(info, false);
This function should really return a value to indicate if it failed.
There is no point continuing if it didn't register anything.
> + /* There is a very subtle race-condition. Even though we might
> + * own a reference to the fb, it may still get unregistered
> + * between our call from get_fb_info() and fblog_register().
> + * Therefore, we simply check whether the same fb still is
> + * registered by calling get_fb_info() again. Only if they
> + * differ we know that it got unregistered, therefore, we
> + * call fblog_unregister() with the old pointer. */
> +
> + tmp = get_fb_info(i);
> + if (tmp && !IS_ERR(tmp))
> + put_fb_info(tmp);
> + if (tmp != info)
> + fblog_unregister(info);
It would be better to fix this issue properly. Calling fblog_unregister
here also looks unsafe if the call to fblog_register above failed.
> + /* Here we either called fblog_unregister() and therefore do not
> + * need any reference to the fb, or we can be sure that the FB
> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
> + * before the last reference is dropped. Hence, we can drop our
> + * reference here. */
This seems a slightly odd reasoning. Why would you not hold a reference
to something you are using?
> + put_fb_info(info);
> + }
> +}
> +
> +static struct notifier_block fblog_notifier = {
> + .notifier_call = fblog_event,
> +};
>
> static int __init fblog_init(void)
> {
> + int ret;
> +
> + ret = fb_register_client(&fblog_notifier);
> + if (ret) {
> + pr_err("cannot register framebuffer notifier\n");
> + return ret;
> + }
> +
> + fblog_scan();
> +
> return 0;
> }
>
> static void __exit fblog_exit(void)
> {
> + unsigned int i;
> + struct fb_info *info;
> +
> + fb_unregister_client(&fblog_notifier);
> +
> + /* We scan through the whole registered_fb array here instead of
> + * fblog_fbs because we need to get the device lock _before_ the
> + * fblog-registration-lock. */
> +
> + for (i = 0; i < FB_MAX; ++i) {
> + info = get_fb_info(i);
> + if (!info || IS_ERR(info))
> + continue;
> +
> + fblog_unregister(info);
Given the description of the get_fb_info/fblog_register race above, can
this unregister the wrong framebuffer?
> + put_fb_info(info);
> + }
> }
>
> module_init(fblog_init);
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
2012-08-12 23:54 ` Ryan Mallon
@ 2012-08-14 11:01 ` David Herrmann
2012-08-15 0:17 ` Ryan Mallon
0 siblings, 1 reply; 22+ messages in thread
From: David Herrmann @ 2012-08-14 11:01 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
Hi Ryan
On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 195 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index fb39737..279f4d8 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -23,15 +23,210 @@
>> * all fblog instances before running other graphics applications.
>> */
>>
>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/fb.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> +
>> +enum fblog_flags {
>> + FBLOG_KILLED,
>> +};
>> +
>> +struct fblog_fb {
>> + unsigned long flags;
>
> Are more flags added in later patches? If not, why not just have:
>
> bool is_killed;
>
> ?
Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
FBLOG_SUSPENDED, FBLOG_BLANKED.
>> +static void fblog_release(struct device *dev)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + kfree(fb);
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static void fblog_do_unregister(struct fb_info *info)
>> +{
>> + struct fblog_fb *fb;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (!fb || fb->info != info)
>> + return;
>> +
>> + fblog_fbs[info->node] = NULL;
>> +
>> + device_del(&fb->dev);
>> + put_device(&fb->dev);
>
> device_unregister?
Right, I will replace it.
>> +}
>> +
>> +static void fblog_do_register(struct fb_info *info, bool force)
>> +{
>> + struct fblog_fb *fb;
>> + int ret;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (fb && fb->info != info) {
>> + if (!force)
>> + return;
>> +
>> + fblog_do_unregister(fb->info);
>> + }
>> +
>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>> + if (!fb)
>> + return;
>> +
>> + fb->info = info;
>> + __module_get(THIS_MODULE);
>> + device_initialize(&fb->dev);
>> + fb->dev.class = fb_class;
>> + fb->dev.release = fblog_release;
>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>> + fblog_fbs[info->node] = fb;
>> +
>> + ret = device_add(&fb->dev);
>> + if (ret) {
>> + fblog_fbs[info->node] = NULL;
>> + set_bit(FBLOG_KILLED, &fb->flags);
>> + put_device(&fb->dev);
>
> kfree(fb); ?
No. See device_initialize() in ./drivers/base/core.c. After a call to
device_initialize() the object is ref-counted so put_device() will
invoke the fblog_release() callback which will call kfree(fb) itself.
>> + return;
>> + }
>> +}
>> +
>> +static void fblog_register(struct fb_info *info, bool force)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_register(info, force);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>> +
>> +static void fblog_unregister(struct fb_info *info)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_unregister(info);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>
> This locking is needlessly heavy, and could easily pushed down into the
> fb_do_(un)register functions. It would also help make it clear exactly
> what the lock is protecting.
I need to call fblog_do_unregister() from within fblog_do_register().
I cannot release the locks while calling fblog_do_unregister() so I
need the unlocked fblog_do_unregister() function. So the locking must
be in a wrapper function.
See below for an explanation of the locks.
>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>> + void *data)
>> +{
>> + struct fb_event *event = data;
>> + struct fb_info *info = event->info;
>> +
>> + switch(action) {
>> + case FB_EVENT_FB_REGISTERED:
>> + /* This is called when a low-level system driver registers a new
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held when this is called. */
>
> Nitpick:
>
> /*
> * The Linux kernel multi-line
> * comment style looks like
> * this.
> */
I was confused by a recent discussion on the LKML:
http://comments.gmane.org/gmane.linux.kernel/1282421
However, turns out they didn't add this to CodingStyle so I will adopt
the old style again. Will be fixed in the next revision, thanks.
>> + fblog_register(info, true);
>> + break;
>> + case FB_EVENT_FB_UNREGISTERED:
>> + /* This is called when a low-level system driver unregisters a
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held. */
>> + fblog_unregister(info);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void fblog_scan(void)
>> +{
>> + unsigned int i;
>> + struct fb_info *info, *tmp;
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>
> Nitpick:
>
> if (IS_ERR_OR_NULL(info))
Didn't know of this macro, thanks.
>> + continue;
>> +
>> + fblog_register(info, false);
>
> This function should really return a value to indicate if it failed.
> There is no point continuing if it didn't register anything.
Indeed.
>> + /* There is a very subtle race-condition. Even though we might
>> + * own a reference to the fb, it may still get unregistered
>> + * between our call from get_fb_info() and fblog_register().
>> + * Therefore, we simply check whether the same fb still is
>> + * registered by calling get_fb_info() again. Only if they
>> + * differ we know that it got unregistered, therefore, we
>> + * call fblog_unregister() with the old pointer. */
>> +
>> + tmp = get_fb_info(i);
>> + if (tmp && !IS_ERR(tmp))
>> + put_fb_info(tmp);
>> + if (tmp != info)
>> + fblog_unregister(info);
>
> It would be better to fix this issue properly. Calling fblog_unregister
> here also looks unsafe if the call to fblog_register above failed.
fblog_unregister() does nothing if the passed "info" does not match
the found "info" so this is safe. And as we are holding a reference to
"info" here, the pointer is always valid and cannot be used by other
FBs.
Fixing this properly means changing the locking of fbdev. This can
either be done by exporting the fbdev-registration-lock (which I want
to avoid as locking should never be exported in an API) or by changing
fbdev to provide an scan/enumeration function itself. However, in my
opinion both would be uglier than using this race-condition-check
here.
The best way would be redesigning the fbdev in-kernel API. But that
means checking that fbcon will not break and this is something I
really don't want to touch.
>> + /* Here we either called fblog_unregister() and therefore do not
>> + * need any reference to the fb, or we can be sure that the FB
>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>> + * before the last reference is dropped. Hence, we can drop our
>> + * reference here. */
>
> This seems a slightly odd reasoning. Why would you not hold a reference
> to something you are using?
It's because get_fb_info() takes the fbdev-registration-lock so we
cannot call it from fblog_event(). And fblog_event() calls
fblog_register() for hotplugged framebuffers so we cannot get a refcnt
for hotplugged framebuffers.
Now I can either increase the fbdev-refcount manually without calling
get_fb_info() in fblog_register() or I can simply drop the framebuffer
when the fbdev-core notifies me that it is gone. I chose the latter
one.
>> + put_fb_info(info);
>> + }
>> +}
>> +
>> +static struct notifier_block fblog_notifier = {
>> + .notifier_call = fblog_event,
>> +};
>>
>> static int __init fblog_init(void)
>> {
>> + int ret;
>> +
>> + ret = fb_register_client(&fblog_notifier);
>> + if (ret) {
>> + pr_err("cannot register framebuffer notifier\n");
>> + return ret;
>> + }
>> +
>> + fblog_scan();
>> +
>> return 0;
>> }
>>
>> static void __exit fblog_exit(void)
>> {
>> + unsigned int i;
>> + struct fb_info *info;
>> +
>> + fb_unregister_client(&fblog_notifier);
>> +
>> + /* We scan through the whole registered_fb array here instead of
>> + * fblog_fbs because we need to get the device lock _before_ the
>> + * fblog-registration-lock. */
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>> + continue;
>> +
>> + fblog_unregister(info);
>
> Given the description of the get_fb_info/fblog_register race above, can
> this unregister the wrong framebuffer?
No. fblog_unregister() will do nothing if the "info" pointers do not
match. Moreover, this unregisters _all_ framebuffers, so I don't
understand how this can unregister the "wrong" framebuffer?
But I just noticed a race here. If the fbdev core unregisters a
framebuffer during my loop, I will not call fblog_unregister() on it,
as it does not exist anymore. Therefore, I will not free it in my
fblog_fbs array as the fblog_notifier has already been unregistered.
So I need to set a global "exiting" flag and fblog_event() shall only
handle the UNBIND/UNREGISTER events during exit. Then I simply move
the fb_unregister_client() call below this loop.
>> + put_fb_info(info);
>> + }
>> }
>>
>> module_init(fblog_init);
>>
>
A short explanation for all locks:
First of all, I spent hours getting this right. The easy way would be
removing all locks and relying on the fbdev locking (fbcon does this).
But obviously, that doesn't work as fbdev has no safe way of
scanning/enumerating all existing devices. And this is needed as fblog
can be compiled as a module.
fbdev has a core registration-lock and each framebuffer has its own
fb-lock. fblog_event() is sometimes called with these locks held,
sometimes without any locks held (see the comments in this function)
and I need to work around this.
So fblog_event() as entry point for hotplugging is called with the
fbdev-registration-lock held. And it calls fblog_(un)register() which
uses its own locks. So when calling this from fblog_scan(), I need to
make sure to guarantee that the locks are acquired in the same order
as in fblog_event(), otherwise I might have deadlocks. But I have no
outside access to the fbdev-registration-lock, therefore, the
fblog-registration-lock is needed and fblog_scan() needs to check for
those ugly races.
As you mentioned that this locking is needlessly complex, I have to
disagree. I use one lock to protect the registration
(fblog_registration_lock) and one lock to protect each registered
framebuffer from concurrent access (struct fblog_fb->lock). This is
the most common way to protect hotplugged devices and I don't see how
this can be done with less locks? (these are the only locks that are
added by fblog).
Normally, this would be all locks I have to access. However, the
fbdev-core wasn't designed as in-kernel API and thus has very
inconsistent locking. And all entry points to fblog that are not
through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
acquire the same locks as the fbdev-core to avoid races with the
fbdev-core. As this is not possible, because the fbdev-locks are not
exported, I need to carefully use fbdev-functions that guarantee that
I have no races. And I think I found the only way to guarantee this.
If anyone has other ideas, I would be glad to hear them.
Thanks for reviewing the code!
Regards
David
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
2012-08-14 11:01 ` David Herrmann
@ 2012-08-15 0:17 ` Ryan Mallon
0 siblings, 0 replies; 22+ messages in thread
From: Ryan Mallon @ 2012-08-15 0:17 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.
I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:
static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}
static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}
__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}
static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}
__fblog_unregister(fb);
}
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;
fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);
ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}
fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);
return 0;
}
Functions which do:
foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}
can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.
>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + case FB_EVENT_FB_UNREGISTERED:
>>> + /* This is called when a low-level system driver unregisters a
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.
Fair enough. It might be something to come back to once this gets merged.
>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().
That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.
> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.
Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.
> As you mentioned that this locking is needlessly complex, I have to
> disagree.
Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.
> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).
I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.
> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.
Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.
~Ryan
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 06/11] fblog: open fb on registration
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (4 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 05/11] fblog: register one fblog object per framebuffer David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-13 0:00 ` Ryan Mallon
2012-08-12 14:53 ` [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters David Herrmann
` (5 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
This opens the framebuffer upon registration so we can use it for
drawing-operations. On unregistration we close it again.
While opening/closing or accessing the fb in any other way, we must hold
the fb-mutex. However, since the notifiers are often called with the mutex
already held, we cannot lock it _after_ taking the
fblog_registration_lock. Therefore, we require the caller to make sure the
fb-mutex is held.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 94 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 93 insertions(+), 1 deletion(-)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 279f4d8..1c526c5 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -32,12 +32,14 @@
enum fblog_flags {
FBLOG_KILLED,
+ FBLOG_OPEN,
};
struct fblog_fb {
unsigned long flags;
struct fb_info *info;
struct device dev;
+ struct mutex lock;
};
static DEFINE_MUTEX(fblog_registration_lock);
@@ -46,6 +48,74 @@ static struct fblog_fb *fblog_fbs[FB_MAX];
#define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
/*
+ * fblog_open/close()
+ * These functions manage access to the underlying framebuffer. While opened, we
+ * have a valid reference to the fb and can use it for drawing operations. When
+ * the fb is unregistered, we drop our reference and close the fb so it can get
+ * deleted properly. We also mark it as dead so no further fblog_open() call
+ * will succeed.
+ * Both functions must be called with the fb->info->lock mutex held! But make
+ * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
+ * will dead-lock with fb-registration.
+ */
+
+static int fblog_open(struct fblog_fb *fb)
+{
+ int ret;
+
+ mutex_lock(&fb->lock);
+
+ if (test_bit(FBLOG_KILLED, &fb->flags)) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
+ if (test_bit(FBLOG_OPEN, &fb->flags)) {
+ ret = 0;
+ goto unlock;
+ }
+
+ if (!try_module_get(fb->info->fbops->owner)) {
+ ret = -ENODEV;
+ goto out_killed;
+ }
+
+ if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
+ ret = -EIO;
+ goto out_unref;
+ }
+
+ set_bit(FBLOG_OPEN, &fb->flags);
+ mutex_unlock(&fb->lock);
+ return 0;
+
+out_unref:
+ module_put(fb->info->fbops->owner);
+out_killed:
+ set_bit(FBLOG_KILLED, &fb->flags);
+unlock:
+ mutex_unlock(&fb->lock);
+ return ret;
+}
+
+static void fblog_close(struct fblog_fb *fb, bool kill_dev)
+{
+ mutex_lock(&fb->lock);
+
+ if (test_bit(FBLOG_OPEN, &fb->flags)) {
+ if (fb->info->fbops->fb_release)
+ fb->info->fbops->fb_release(fb->info, 0);
+ module_put(fb->info->fbops->owner);
+ clear_bit(FBLOG_OPEN, &fb->flags);
+ }
+
+ if (kill_dev)
+ set_bit(FBLOG_KILLED, &fb->flags);
+
+ mutex_unlock(&fb->lock);
+}
+
+/*
* fblog framebuffer list
* The fblog_fbs[] array contains all currently registered framebuffers. If a
* framebuffer is in that list, we always must make sure that we own a reference
@@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
fblog_fbs[info->node] = NULL;
+ fblog_close(fb, true);
device_del(&fb->dev);
put_device(&fb->dev);
}
@@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
return;
fb->info = info;
+ mutex_init(&fb->lock);
__module_get(THIS_MODULE);
device_initialize(&fb->dev);
fb->dev.class = fb_class;
@@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
put_device(&fb->dev);
return;
}
+
+ fblog_open(fb);
}
static void fblog_register(struct fb_info *info, bool force)
@@ -134,6 +208,7 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
{
struct fb_event *event = data;
struct fb_info *info = event->info;
+ struct fblog_fb *fb;
switch(action) {
case FB_EVENT_FB_REGISTERED:
@@ -145,8 +220,21 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
case FB_EVENT_FB_UNREGISTERED:
/* This is called when a low-level system driver unregisters a
* framebuffer. The registration lock is held but the console
- * lock might not be held. */
+ * lock might not be held. The fb-lock is not held, either! */
+ mutex_lock(&info->lock);
fblog_unregister(info);
+ mutex_unlock(&info->lock);
+ break;
+ case FB_EVENT_FB_UNBIND:
+ /* Called directly before unregistering an FB. The FB is still
+ * valid here and the registration lock is held but the console
+ * lock might not be held (really?). */
+ mutex_lock(&fblog_registration_lock);
+ fb = fblog_fbs[info->node];
+ mutex_unlock(&fblog_registration_lock);
+
+ if (fb)
+ fblog_close(fb, true);
break;
}
@@ -163,7 +251,9 @@ static void fblog_scan(void)
if (!info || IS_ERR(info))
continue;
+ mutex_lock(&info->lock);
fblog_register(info, false);
+ mutex_unlock(&info->lock);
/* There is a very subtle race-condition. Even though we might
* own a reference to the fb, it may still get unregistered
@@ -224,7 +314,9 @@ static void __exit fblog_exit(void)
if (!info || IS_ERR(info))
continue;
+ mutex_lock(&info->lock);
fblog_unregister(info);
+ mutex_unlock(&info->lock);
put_fb_info(info);
}
}
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 06/11] fblog: open fb on registration
2012-08-12 14:53 ` [PATCH 06/11] fblog: open fb on registration David Herrmann
@ 2012-08-13 0:00 ` Ryan Mallon
2012-08-14 11:05 ` David Herrmann
0 siblings, 1 reply; 22+ messages in thread
From: Ryan Mallon @ 2012-08-13 0:00 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
On 13/08/12 00:53, David Herrmann wrote:
> This opens the framebuffer upon registration so we can use it for
> drawing-operations. On unregistration we close it again.
>
> While opening/closing or accessing the fb in any other way, we must hold
> the fb-mutex. However, since the notifiers are often called with the mutex
> already held, we cannot lock it _after_ taking the
> fblog_registration_lock. Therefore, we require the caller to make sure the
> fb-mutex is held.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Some comments below,
~Ryan
> ---
> drivers/video/console/fblog.c | 94 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> index 279f4d8..1c526c5 100644
> --- a/drivers/video/console/fblog.c
> +++ b/drivers/video/console/fblog.c
> @@ -32,12 +32,14 @@
>
> enum fblog_flags {
> FBLOG_KILLED,
> + FBLOG_OPEN,
> };
>
> struct fblog_fb {
> unsigned long flags;
> struct fb_info *info;
> struct device dev;
> + struct mutex lock;
> };
>
> static DEFINE_MUTEX(fblog_registration_lock);
> @@ -46,6 +48,74 @@ static struct fblog_fb *fblog_fbs[FB_MAX];
> #define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
>
> /*
> + * fblog_open/close()
> + * These functions manage access to the underlying framebuffer. While opened, we
> + * have a valid reference to the fb and can use it for drawing operations. When
> + * the fb is unregistered, we drop our reference and close the fb so it can get
> + * deleted properly. We also mark it as dead so no further fblog_open() call
> + * will succeed.
> + * Both functions must be called with the fb->info->lock mutex held! But make
> + * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
> + * will dead-lock with fb-registration.
> + */
> +
> +static int fblog_open(struct fblog_fb *fb)
> +{
> + int ret;
> +
> + mutex_lock(&fb->lock);
> +
> + if (test_bit(FBLOG_KILLED, &fb->flags)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
> + ret = 0;
> + goto unlock;
> + }
> +
> + if (!try_module_get(fb->info->fbops->owner)) {
> + ret = -ENODEV;
> + goto out_killed;
> + }
> +
> + if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
Should propagate the error here:
if (fb->info->fbops->fb_open) {
ret = fb->info->fbops->fb_open(fb->info, 0);
if (ret)
gotou out_unref;
}
> + ret = -EIO;
> + goto out_unref;
> + }
> +
> + set_bit(FBLOG_OPEN, &fb->flags);
> + mutex_unlock(&fb->lock);
> + return 0;
> +
> +out_unref:
> + module_put(fb->info->fbops->owner);
> +out_killed:
> + set_bit(FBLOG_KILLED, &fb->flags);
> +unlock:
> + mutex_unlock(&fb->lock);
> + return ret;
> +}
> +
> +static void fblog_close(struct fblog_fb *fb, bool kill_dev)
> +{
> + mutex_lock(&fb->lock);
> +
> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
> + if (fb->info->fbops->fb_release)
> + fb->info->fbops->fb_release(fb->info, 0);
> + module_put(fb->info->fbops->owner);
> + clear_bit(FBLOG_OPEN, &fb->flags);
> + }
> +
> + if (kill_dev)
> + set_bit(FBLOG_KILLED, &fb->flags);
> +
> + mutex_unlock(&fb->lock);
> +}
> +
> +/*
> * fblog framebuffer list
> * The fblog_fbs[] array contains all currently registered framebuffers. If a
> * framebuffer is in that list, we always must make sure that we own a reference
> @@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
>
> fblog_fbs[info->node] = NULL;
>
> + fblog_close(fb, true);
> device_del(&fb->dev);
> put_device(&fb->dev);
device_unregister?
> }
> @@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
> return;
>
> fb->info = info;
> + mutex_init(&fb->lock);
> __module_get(THIS_MODULE);
> device_initialize(&fb->dev);
> fb->dev.class = fb_class;
> @@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
> put_device(&fb->dev);
> return;
> }
> +
> + fblog_open(fb);
> }
This function should really return an errno value.
>
> static void fblog_register(struct fb_info *info, bool force)
> @@ -134,6 +208,7 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
> {
> struct fb_event *event = data;
> struct fb_info *info = event->info;
> + struct fblog_fb *fb;
>
> switch(action) {
> case FB_EVENT_FB_REGISTERED:
> @@ -145,8 +220,21 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
> case FB_EVENT_FB_UNREGISTERED:
> /* This is called when a low-level system driver unregisters a
> * framebuffer. The registration lock is held but the console
> - * lock might not be held. */
> + * lock might not be held. The fb-lock is not held, either! */
> + mutex_lock(&info->lock);
> fblog_unregister(info);
> + mutex_unlock(&info->lock);
> + break;
> + case FB_EVENT_FB_UNBIND:
> + /* Called directly before unregistering an FB. The FB is still
> + * valid here and the registration lock is held but the console
> + * lock might not be held (really?). */
> + mutex_lock(&fblog_registration_lock);
> + fb = fblog_fbs[info->node];
> + mutex_unlock(&fblog_registration_lock);
> +
> + if (fb)
> + fblog_close(fb, true);
> break;
> }
>
> @@ -163,7 +251,9 @@ static void fblog_scan(void)
> if (!info || IS_ERR(info))
> continue;
>
> + mutex_lock(&info->lock);
> fblog_register(info, false);
> + mutex_unlock(&info->lock);
>
> /* There is a very subtle race-condition. Even though we might
> * own a reference to the fb, it may still get unregistered
> @@ -224,7 +314,9 @@ static void __exit fblog_exit(void)
> if (!info || IS_ERR(info))
> continue;
>
> + mutex_lock(&info->lock);
> fblog_unregister(info);
> + mutex_unlock(&info->lock);
> put_fb_info(info);
> }
> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 06/11] fblog: open fb on registration
2012-08-13 0:00 ` Ryan Mallon
@ 2012-08-14 11:05 ` David Herrmann
0 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-14 11:05 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
Hi Ryan
On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> /*
>> + * fblog_open/close()
>> + * These functions manage access to the underlying framebuffer. While opened, we
>> + * have a valid reference to the fb and can use it for drawing operations. When
>> + * the fb is unregistered, we drop our reference and close the fb so it can get
>> + * deleted properly. We also mark it as dead so no further fblog_open() call
>> + * will succeed.
>> + * Both functions must be called with the fb->info->lock mutex held! But make
>> + * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
>> + * will dead-lock with fb-registration.
>> + */
>> +
>> +static int fblog_open(struct fblog_fb *fb)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&fb->lock);
>> +
>> + if (test_bit(FBLOG_KILLED, &fb->flags)) {
>> + ret = -ENODEV;
>> + goto unlock;
>> + }
>> +
>> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
>> + ret = 0;
>> + goto unlock;
>> + }
>> +
>> + if (!try_module_get(fb->info->fbops->owner)) {
>> + ret = -ENODEV;
>> + goto out_killed;
>> + }
>> +
>> + if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
>
> Should propagate the error here:
>
> if (fb->info->fbops->fb_open) {
> ret = fb->info->fbops->fb_open(fb->info, 0);
> if (ret)
> gotou out_unref;
> }
Nice catch, thanks.
>> +/*
>> * fblog framebuffer list
>> * The fblog_fbs[] array contains all currently registered framebuffers. If a
>> * framebuffer is in that list, we always must make sure that we own a reference
>> @@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
>>
>> fblog_fbs[info->node] = NULL;
>>
>> + fblog_close(fb, true);
>> device_del(&fb->dev);
>> put_device(&fb->dev);
>
> device_unregister?
Heh, you already mentioned this in the previous patch. I definitely
need to fix that.
>> }
>> @@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>>
>> fb->info = info;
>> + mutex_init(&fb->lock);
>> __module_get(THIS_MODULE);
>> device_initialize(&fb->dev);
>> fb->dev.class = fb_class;
>> @@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> put_device(&fb->dev);
>> return;
>> }
>> +
>> + fblog_open(fb);
>> }
>
> This function should really return an errno value.
I never used the return value in my code but as mentioned in the
previous patches, fblog_scan() should check them. So yes, I will add
this.
Thanks!
David
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (5 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 06/11] fblog: open fb on registration David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-13 0:04 ` Ryan Mallon
2012-08-12 14:53 ` [PATCH 08/11] fblog: cache framebuffer BLANK and SUSPEND states David Herrmann
` (4 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
fblog is mainly useful during boot, reboot, panics and maintenance. In all
cases you often want to control which monitors are used for console
output. Moreover, in multi-seat environments it is desireable to reduce
system-overhead by not drawing the console to all framebuffers. Four
mechanisms to select framebuffers for fblog are added:
1) "active" module parameter: This parameter selects whether fblog has
access to available framebuffer devices. If it is true, then fblog will
open devices following the rules described below and rendering will take
place. If it is false, new hotplugged devices will not be activated and no
more rendering to currently active devices takes place. However, active
devices will continue rendering after this is set to true again.
2) "active" sysfs attribute for each fblog object. Reading this value
returns whether a framebuffer is currently active. Writing it opens/closes
the framebuffer. This allows runtime control which fbs are used. For
instance, init can set these to 0 after bootup.
Note that a framebuffer is only active if this is 1 _and_ the "active"
module parameter is set to "true".
3) "activate_on_hotplug" module parameter: This selects whether a device
is activated by default when hotplugged. This is true by default so new
devices will be automatically activated.
4) "main_only" module parameter: This selects what devices are activated
on hotplug. This has no effect if "activate_on_hotplug" is false.
Otherwise, if this is true then only fb0 will be activated on hotplug.
This is false by default.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 1c526c5..aed77dc 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -44,6 +44,9 @@ struct fblog_fb {
static DEFINE_MUTEX(fblog_registration_lock);
static struct fblog_fb *fblog_fbs[FB_MAX];
+static bool active = true;
+static bool activate_on_hotplug = true;
+static bool main_only = false;
#define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
@@ -63,6 +66,9 @@ static int fblog_open(struct fblog_fb *fb)
{
int ret;
+ if (!active)
+ return -EPERM;
+
mutex_lock(&fb->lock);
if (test_bit(FBLOG_KILLED, &fb->flags)) {
@@ -115,6 +121,40 @@ static void fblog_close(struct fblog_fb *fb, bool kill_dev)
mutex_unlock(&fb->lock);
}
+static ssize_t fblog_dev_active_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct fblog_fb *fb = to_fblog_dev(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n",
+ !!test_bit(FBLOG_OPEN, &fb->flags));
+}
+
+static ssize_t fblog_dev_active_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct fblog_fb *fb = to_fblog_dev(dev);
+ unsigned long num;
+ int ret = 0;
+
+ num = simple_strtoul(buf, NULL, 10);
+
+ mutex_lock(&fb->info->lock);
+ if (num)
+ ret = fblog_open(fb);
+ else
+ fblog_close(fb, false);
+ mutex_unlock(&fb->info->lock);
+
+ return ret ? ret : count;
+}
+
+static DEVICE_ATTR(active, S_IRUGO | S_IWUSR | S_IWGRP, fblog_dev_active_show,
+ fblog_dev_active_store);
+
/*
* fblog framebuffer list
* The fblog_fbs[] array contains all currently registered framebuffers. If a
@@ -148,6 +188,7 @@ static void fblog_do_unregister(struct fb_info *info)
fblog_fbs[info->node] = NULL;
fblog_close(fb, true);
+ device_remove_file(&fb->dev, &dev_attr_active);
device_del(&fb->dev);
put_device(&fb->dev);
}
@@ -156,6 +197,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
+ bool do_open = true;
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
@@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
return;
}
- fblog_open(fb);
+ ret = device_create_file(&fb->dev, &dev_attr_active);
+ if (ret) {
+ pr_err("fblog: cannot create sysfs entry");
+ /* do not open fb if we cannot create control file */
+ do_open = false;
+ }
+
+ if (!activate_on_hotplug)
+ do_open = false;
+ if (main_only && info->node != 0)
+ do_open = false;
+
+ if (do_open)
+ fblog_open(fb);
}
static void fblog_register(struct fb_info *info, bool force)
@@ -321,6 +376,15 @@ static void __exit fblog_exit(void)
}
}
+module_param(active, bool, S_IRUGO | S_IWUSR | S_IWGRP);
+MODULE_PARM_DESC(active, "Activate fblog rendering");
+
+module_param(activate_on_hotplug, bool, S_IRUGO | S_IWUSR | S_IWGRP);
+MODULE_PARM_DESC(activate_on_hotplug, "Activate fblog on hotplugged devices");
+
+module_param(main_only, bool, S_IRUGO);
+MODULE_PARM_DESC(main_only, "Activate fblog by default only on main devices");
+
module_init(fblog_init);
module_exit(fblog_exit);
MODULE_LICENSE("GPL");
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
2012-08-12 14:53 ` [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters David Herrmann
@ 2012-08-13 0:04 ` Ryan Mallon
2012-08-14 11:07 ` David Herrmann
0 siblings, 1 reply; 22+ messages in thread
From: Ryan Mallon @ 2012-08-13 0:04 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
On 13/08/12 00:53, David Herrmann wrote:
> fblog is mainly useful during boot, reboot, panics and maintenance. In all
> cases you often want to control which monitors are used for console
> output. Moreover, in multi-seat environments it is desireable to reduce
> system-overhead by not drawing the console to all framebuffers. Four
> mechanisms to select framebuffers for fblog are added:
>
> 1) "active" module parameter: This parameter selects whether fblog has
> access to available framebuffer devices. If it is true, then fblog will
> open devices following the rules described below and rendering will take
> place. If it is false, new hotplugged devices will not be activated and no
> more rendering to currently active devices takes place. However, active
> devices will continue rendering after this is set to true again.
>
> 2) "active" sysfs attribute for each fblog object. Reading this value
> returns whether a framebuffer is currently active. Writing it opens/closes
> the framebuffer. This allows runtime control which fbs are used. For
> instance, init can set these to 0 after bootup.
> Note that a framebuffer is only active if this is 1 _and_ the "active"
> module parameter is set to "true".
>
> 3) "activate_on_hotplug" module parameter: This selects whether a device
> is activated by default when hotplugged. This is true by default so new
> devices will be automatically activated.
>
> 4) "main_only" module parameter: This selects what devices are activated
> on hotplug. This has no effect if "activate_on_hotplug" is false.
> Otherwise, if this is true then only fb0 will be activated on hotplug.
> This is false by default.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> drivers/video/console/fblog.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> index 1c526c5..aed77dc 100644
> --- a/drivers/video/console/fblog.c
> +++ b/drivers/video/console/fblog.c
> @@ -44,6 +44,9 @@ struct fblog_fb {
>
> static DEFINE_MUTEX(fblog_registration_lock);
> static struct fblog_fb *fblog_fbs[FB_MAX];
> +static bool active = true;
> +static bool activate_on_hotplug = true;
> +static bool main_only = false;
>
> #define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
>
> @@ -63,6 +66,9 @@ static int fblog_open(struct fblog_fb *fb)
> {
> int ret;
>
> + if (!active)
> + return -EPERM;
> +
> mutex_lock(&fb->lock);
>
> if (test_bit(FBLOG_KILLED, &fb->flags)) {
> @@ -115,6 +121,40 @@ static void fblog_close(struct fblog_fb *fb, bool kill_dev)
> mutex_unlock(&fb->lock);
> }
>
> +static ssize_t fblog_dev_active_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct fblog_fb *fb = to_fblog_dev(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + !!test_bit(FBLOG_OPEN, &fb->flags));
Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
> +}
> +
> +static ssize_t fblog_dev_active_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct fblog_fb *fb = to_fblog_dev(dev);
> + unsigned long num;
> + int ret = 0;
> +
> + num = simple_strtoul(buf, NULL, 10);
kstrtoul is preferred these days I think, it also catches errors.
> +
> + mutex_lock(&fb->info->lock);
> + if (num)
> + ret = fblog_open(fb);
> + else
> + fblog_close(fb, false);
> + mutex_unlock(&fb->info->lock);
> +
> + return ret ? ret : count;
Nitpick, you can use gcc's shortcut form of the ? operator here:
return ret ?: count;
> +}
> +
> +static DEVICE_ATTR(active, S_IRUGO | S_IWUSR | S_IWGRP, fblog_dev_active_show,
> + fblog_dev_active_store);
> +
> /*
> * fblog framebuffer list
> * The fblog_fbs[] array contains all currently registered framebuffers. If a
> @@ -148,6 +188,7 @@ static void fblog_do_unregister(struct fb_info *info)
> fblog_fbs[info->node] = NULL;
>
> fblog_close(fb, true);
> + device_remove_file(&fb->dev, &dev_attr_active);
> device_del(&fb->dev);
> put_device(&fb->dev);
> }
> @@ -156,6 +197,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
> {
> struct fblog_fb *fb;
> int ret;
> + bool do_open = true;
>
> fb = fblog_fbs[info->node];
> if (fb && fb->info != info) {
> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
> return;
> }
>
> - fblog_open(fb);
> + ret = device_create_file(&fb->dev, &dev_attr_active);
> + if (ret) {
> + pr_err("fblog: cannot create sysfs entry");
Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
> + /* do not open fb if we cannot create control file */
> + do_open = false;
> + }
> +
> + if (!activate_on_hotplug)
> + do_open = false;
> + if (main_only && info->node != 0)
> + do_open = false;
> +
> + if (do_open)
> + fblog_open(fb);
> }
>
> static void fblog_register(struct fb_info *info, bool force)
> @@ -321,6 +376,15 @@ static void __exit fblog_exit(void)
> }
> }
>
> +module_param(active, bool, S_IRUGO | S_IWUSR | S_IWGRP);
> +MODULE_PARM_DESC(active, "Activate fblog rendering");
> +
> +module_param(activate_on_hotplug, bool, S_IRUGO | S_IWUSR | S_IWGRP);
> +MODULE_PARM_DESC(activate_on_hotplug, "Activate fblog on hotplugged devices");
> +
> +module_param(main_only, bool, S_IRUGO);
> +MODULE_PARM_DESC(main_only, "Activate fblog by default only on main devices");
> +
> module_init(fblog_init);
> module_exit(fblog_exit);
> MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
2012-08-13 0:04 ` Ryan Mallon
@ 2012-08-14 11:07 ` David Herrmann
0 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-14 11:07 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
Hi Ryan
On Mon, Aug 13, 2012 at 2:04 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> +static ssize_t fblog_dev_active_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + !!test_bit(FBLOG_OPEN, &fb->flags));
>
> Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
>
>> +}
>> +
>> +static ssize_t fblog_dev_active_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> + unsigned long num;
>> + int ret = 0;
>> +
>> + num = simple_strtoul(buf, NULL, 10);
>
> kstrtoul is preferred these days I think, it also catches errors.
>
>> +
>> + mutex_lock(&fb->info->lock);
>> + if (num)
>> + ret = fblog_open(fb);
>> + else
>> + fblog_close(fb, false);
>> + mutex_unlock(&fb->info->lock);
>> +
>> + return ret ? ret : count;
>
> Nitpick, you can use gcc's shortcut form of the ? operator here:
>
> return ret ?: count;
>
>> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>> }
>>
>> - fblog_open(fb);
>> + ret = device_create_file(&fb->dev, &dev_attr_active);
>> + if (ret) {
>> + pr_err("fblog: cannot create sysfs entry");
>
> Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
>
>> + /* do not open fb if we cannot create control file */
>> + do_open = false;
>> + }
>> +
>> + if (!activate_on_hotplug)
>> + do_open = false;
>> + if (main_only && info->node != 0)
>> + do_open = false;
>> +
>> + if (do_open)
>> + fblog_open(fb);
>> }
>>
>> static void fblog_register(struct fb_info *info, bool force)
All four fixes make sense, thanks!
David
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 08/11] fblog: cache framebuffer BLANK and SUSPEND states
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (6 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 09/11] fblog: register console driver David Herrmann
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
We must cache these states so we will never draw to the framebuffer while
it is suspended or blanked.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index aed77dc..8bcd0ce 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -33,6 +33,8 @@
enum fblog_flags {
FBLOG_KILLED,
FBLOG_OPEN,
+ FBLOG_SUSPENDED,
+ FBLOG_BLANKED,
};
struct fblog_fb {
@@ -264,6 +266,7 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
struct fb_event *event = data;
struct fb_info *info = event->info;
struct fblog_fb *fb;
+ int *blank;
switch(action) {
case FB_EVENT_FB_REGISTERED:
@@ -291,6 +294,44 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
if (fb)
fblog_close(fb, true);
break;
+ case FB_EVENT_SUSPEND:
+ /* This is called when the low-level display driver suspends the
+ * video system. We should not access the video system while it
+ * is suspended. This is called with the console lock held. */
+ mutex_lock(&fblog_registration_lock);
+ fb = fblog_fbs[info->node];
+ mutex_unlock(&fblog_registration_lock);
+
+ if (fb)
+ set_bit(FBLOG_SUSPENDED, &fb->flags);
+ break;
+ case FB_EVENT_RESUME:
+ /* This is called when the low-level display driver resumes
+ * operating. It is called with the console lock held. */
+ mutex_lock(&fblog_registration_lock);
+ fb = fblog_fbs[info->node];
+ mutex_unlock(&fblog_registration_lock);
+
+ if (fb)
+ clear_bit(FBLOG_SUSPENDED, &fb->flags);
+ break;
+ case FB_EVENT_BLANK:
+ /* This gets called _after_ the framebuffer was successfully
+ * blanked. The console-lock is always held while fb_blank is
+ * called and during this callback. */
+ mutex_lock(&fblog_registration_lock);
+ fb = fblog_fbs[info->node];
+ mutex_unlock(&fblog_registration_lock);
+
+ if (!fb)
+ break;
+
+ blank = (int*)event->data;
+ if (*blank == FB_BLANK_UNBLANK)
+ clear_bit(FBLOG_BLANKED, &fb->flags);
+ else
+ set_bit(FBLOG_BLANKED, &fb->flags);
+ break;
}
return 0;
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 09/11] fblog: register console driver
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (7 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 08/11] fblog: cache framebuffer BLANK and SUSPEND states David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 10/11] fblog: draw console to framebuffers David Herrmann
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
We want to print the kernel log to all FBs so we need a console driver.
This registers the driver on startup and writes all messages to all
registered fblog instances.
We cannot share a console buffer between FBs because they might have
different resolutions. Therefore, we create one buffer per object. We
destroy the buffer during close() so we do not waste memory if it is not
used.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 150 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 150 insertions(+)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 8bcd0ce..2e39577 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -25,11 +25,34 @@
#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
+#include <linux/console.h>
#include <linux/device.h>
#include <linux/fb.h>
#include <linux/module.h>
#include <linux/mutex.h>
+/**
+ * struct fblog_buf: Console text buffer
+ *
+ * Each framebuffer has its own text buffer which contains all characters that
+ * are currently printed on screen. The buffers might have different sizes and
+ * can be resized during runtime. When the buffer content changes, we redraw the
+ * screen.
+ *
+ * width: Width of buffer in characters
+ * height: Height of buffer in characters
+ * lines: Array of lines
+ * pos_x: Cursor x-position
+ * pos_y: Cursor y-position
+ */
+struct fblog_buf {
+ size_t width;
+ size_t height;
+ u16 **lines;
+ size_t pos_x;
+ size_t pos_y;
+};
+
enum fblog_flags {
FBLOG_KILLED,
FBLOG_OPEN,
@@ -42,6 +65,7 @@ struct fblog_fb {
struct fb_info *info;
struct device dev;
struct mutex lock;
+ struct fblog_buf buf;
};
static DEFINE_MUTEX(fblog_registration_lock);
@@ -52,6 +76,106 @@ static bool main_only = false;
#define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
+static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
+ size_t height)
+{
+ u16 **lines = NULL;
+ size_t i, j, minw, minh;
+
+ if (buf->height == height && buf->width == width)
+ return;
+
+ if (width && height) {
+ lines = kzalloc(height * sizeof(char *), GFP_KERNEL);
+ if (!lines)
+ return;
+
+ for (i = 0; i < height; ++i) {
+ lines[i] = kzalloc(width * sizeof(u16), GFP_KERNEL);
+ if (!lines[i]) {
+ while (i--)
+ kfree(lines[i]);
+ return;
+ }
+ }
+
+ /* copy old lines */
+ minw = min(width, buf->width);
+ minh = min(height, buf->height);
+ if (height >= buf->height)
+ i = 0;
+ else
+ i = buf->height - height;
+
+ for (j = 0; j < minh; ++i, ++j)
+ memcpy(lines[j], buf->lines[i], minw * sizeof(u16));
+ } else {
+ width = 0;
+ height = 0;
+ }
+
+ for (i = 0; i < buf->height; ++i)
+ kfree(buf->lines[i]);
+ kfree(buf->lines);
+
+ buf->lines = lines;
+ buf->width = width;
+ buf->height = height;
+}
+
+static void fblog_buf_deinit(struct fblog_buf *buf)
+{
+ fblog_buf_resize(buf, 0, 0);
+}
+
+static void fblog_buf_rotate(struct fblog_buf *buf)
+{
+ u16 *line;
+
+ if (!buf->height)
+ return;
+
+ line = buf->lines[0];
+ memset(line, 0, sizeof(u16) * buf->width);
+
+ memmove(buf->lines, &buf->lines[1], sizeof(char*) * (buf->height - 1));
+ buf->lines[buf->height - 1] = line;
+}
+
+static void fblog_buf_write(struct fblog_buf *buf, const char *str, size_t len)
+{
+ char c;
+
+ if (!buf->height)
+ return;
+
+ while (len--) {
+ c = *str++;
+
+ if (c == 0)
+ c = '?';
+
+ if (c == '\n') {
+ buf->pos_x = 0;
+ if (++buf->pos_y >= buf->height) {
+ buf->pos_y = buf->height - 1;
+ fblog_buf_rotate(buf);
+ }
+ } else {
+ if (buf->pos_x >= buf->width) {
+ buf->pos_x = 0;
+ ++buf->pos_y;
+ }
+ if (buf->pos_y >= buf->height) {
+ buf->pos_y = buf->height - 1;
+ fblog_buf_rotate(buf);
+ }
+
+ buf->lines[buf->pos_y][buf->pos_x++] = c;
+ }
+ }
+}
+
/*
* fblog_open/close()
* These functions manage access to the underlying framebuffer. While opened, we
@@ -66,6 +190,7 @@ static bool main_only = false;
static int fblog_open(struct fblog_fb *fb)
{
+ static const char init_str[] = "Framebuffer log initialized\n";
int ret;
if (!active)
@@ -93,6 +218,8 @@ static int fblog_open(struct fblog_fb *fb)
goto out_unref;
}
+ fblog_buf_resize(&fb->buf, 80, 24);
+ fblog_buf_write(&fb->buf, init_str, sizeof(init_str) - 1);
set_bit(FBLOG_OPEN, &fb->flags);
mutex_unlock(&fb->lock);
return 0;
@@ -115,6 +242,7 @@ static void fblog_close(struct fblog_fb *fb, bool kill_dev)
fb->info->fbops->fb_release(fb->info, 0);
module_put(fb->info->fbops->owner);
clear_bit(FBLOG_OPEN, &fb->flags);
+ fblog_buf_deinit(&fb->buf);
}
if (kill_dev)
@@ -379,6 +507,26 @@ static struct notifier_block fblog_notifier = {
.notifier_call = fblog_event,
};
+static void fblog_con_write(struct console *con, const char *buf,
+ unsigned int len)
+{
+ int i;
+
+ mutex_lock(&fblog_registration_lock);
+ for (i = 0; i < FB_MAX; ++i) {
+ if (fblog_fbs[i]) {
+ fblog_buf_write(&fblog_fbs[i]->buf, buf, len);
+ }
+ }
+ mutex_unlock(&fblog_registration_lock);
+}
+
+static struct console fblog_con_driver = {
+ .name = "fblog",
+ .write = fblog_con_write,
+ .flags = CON_PRINTBUFFER | CON_ENABLED,
+};
+
static int __init fblog_init(void)
{
int ret;
@@ -390,6 +538,7 @@ static int __init fblog_init(void)
}
fblog_scan();
+ register_console(&fblog_con_driver);
return 0;
}
@@ -399,6 +548,7 @@ static void __exit fblog_exit(void)
unsigned int i;
struct fb_info *info;
+ unregister_console(&fblog_con_driver);
fb_unregister_client(&fblog_notifier);
/* We scan through the whole registered_fb array here instead of
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 10/11] fblog: draw console to framebuffers
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (8 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 09/11] fblog: register console driver David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 14:53 ` [PATCH 11/11] MAINTAINERS: add fblog entry David Herrmann
2012-08-12 15:28 ` [PATCH 00/11] fblog: Framebuffer kernel log driver v4 Florian Tobias Schandinat
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
If not disabled or suspended, we now blit the console data to each
framebuffer. We only redraw on changes to avoid consuming too much CPU
power.
This isn't optimized for speed, currently. However, fblog is mainly used
for debugging purposes so this can be optimized later.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/video/console/fblog.c | 110 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 1 deletion(-)
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 2e39577..25bb63d 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -28,8 +28,11 @@
#include <linux/console.h>
#include <linux/device.h>
#include <linux/fb.h>
+#include <linux/font.h>
+#include <linux/kd.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include "fbdraw.h"
/**
* struct fblog_buf: Console text buffer
@@ -66,6 +69,7 @@ struct fblog_fb {
struct device dev;
struct mutex lock;
struct fblog_buf buf;
+ struct console_font font;
};
static DEFINE_MUTEX(fblog_registration_lock);
@@ -176,6 +180,61 @@ static void fblog_buf_write(struct fblog_buf *buf, const char *str, size_t len)
}
}
+static void fblog_redraw_clear(struct fblog_fb *fb)
+{
+ struct fb_fillrect region;
+ struct fb_info *info = fb->info;
+
+ region.color = 0;
+ region.dx = 0;
+ region.dy = 0;
+ region.width = info->var.xres;
+ region.height = info->var.yres;
+ region.rop = ROP_COPY;
+
+ info->fbops->fb_fillrect(info, ®ion);
+}
+
+static void fblog_redraw(struct fblog_fb *fb)
+{
+ size_t i;
+
+ if (!active)
+ return;
+
+ mutex_lock(&fb->lock);
+ if (!test_bit(FBLOG_OPEN, &fb->flags) ||
+ test_bit(FBLOG_SUSPENDED, &fb->flags) ||
+ test_bit(FBLOG_BLANKED, &fb->flags)) {
+ mutex_unlock(&fb->lock);
+ return;
+ }
+
+ fblog_redraw_clear(fb);
+
+ for (i = 0; i < fb->buf.height; ++i) {
+ fbdraw_font(fb->info, &fb->font, false, 0, i, 7, 0, 0,
+ fb->buf.lines[i], fb->buf.width);
+ }
+
+ mutex_unlock(&fb->lock);
+}
+
+static void fblog_refresh(struct fblog_fb *fb)
+{
+ unsigned int width, height;
+
+ mutex_lock(&fb->lock);
+ if (test_bit(FBLOG_OPEN, &fb->flags)) {
+ width = fb->info->var.xres / fb->font.width;
+ height = fb->info->var.yres / fb->font.height;
+ fblog_buf_resize(&fb->buf, width, height);
+ }
+ mutex_unlock(&fb->lock);
+
+ fblog_redraw(fb);
+}
+
/*
* fblog_open/close()
* These functions manage access to the underlying framebuffer. While opened, we
@@ -192,6 +251,10 @@ static int fblog_open(struct fblog_fb *fb)
{
static const char init_str[] = "Framebuffer log initialized\n";
int ret;
+ struct fb_var_screeninfo var;
+ const struct fb_videomode *mode;
+ unsigned int width, height;
+ const struct font_desc *font;
if (!active)
return -EPERM;
@@ -208,6 +271,13 @@ static int fblog_open(struct fblog_fb *fb)
goto unlock;
}
+ font = get_default_font(var.xres, var.yres, fb->info->pixmap.blit_x,
+ fb->info->pixmap.blit_y);
+ if (!font) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
if (!try_module_get(fb->info->fbops->owner)) {
ret = -ENODEV;
goto out_killed;
@@ -218,10 +288,22 @@ static int fblog_open(struct fblog_fb *fb)
goto out_unref;
}
- fblog_buf_resize(&fb->buf, 80, 24);
+ var = fb->info->var;
+ mode = fb_find_best_mode(&var, &fb->info->modelist);
+ var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE;
+ fb_set_var(fb->info, &var);
+
+ fb->font.width = font->width;
+ fb->font.height = font->height;
+ fb->font.data = (void*)font->data;
+
+ width = var.xres / fb->font.width;
+ height = var.yres / fb->font.height;
+ fblog_buf_resize(&fb->buf, width, height);
fblog_buf_write(&fb->buf, init_str, sizeof(init_str) - 1);
set_bit(FBLOG_OPEN, &fb->flags);
mutex_unlock(&fb->lock);
+ fblog_redraw(fb);
return 0;
out_unref:
@@ -460,6 +542,31 @@ static int fblog_event(struct notifier_block *self, unsigned long action,
else
set_bit(FBLOG_BLANKED, &fb->flags);
break;
+ case FB_EVENT_MODE_DELETE:
+ /* This is sent when a video mode is removed. The current video
+ * mode is never removed! The console lock is held while this is
+ * called. */
+ /* fallthrough */
+ case FB_EVENT_NEW_MODELIST:
+ /* This is sent when the modelist got changed. The console-lock
+ * is held and we should reset the mode. */
+ /* fallthrough */
+ case FB_EVENT_MODE_CHANGE_ALL:
+ /* This is the same as below but notifies us that the user used
+ * the FB_ACTIVATE_ALL flag when setting the video mode. */
+ /* fallthrough */
+ case FB_EVENT_MODE_CHANGE:
+ /* This is called when the _user_ changes the video mode via
+ * ioctls. It is not sent, when the kernel changes the mode
+ * internally. This callback is called inside fb_set_var() so
+ * the console lock is held. */
+ mutex_lock(&fblog_registration_lock);
+ fb = fblog_fbs[info->node];
+ mutex_unlock(&fblog_registration_lock);
+
+ if (fb)
+ fblog_refresh(fb);
+ break;
}
return 0;
@@ -516,6 +623,7 @@ static void fblog_con_write(struct console *con, const char *buf,
for (i = 0; i < FB_MAX; ++i) {
if (fblog_fbs[i]) {
fblog_buf_write(&fblog_fbs[i]->buf, buf, len);
+ fblog_redraw(fblog_fbs[i]);
}
}
mutex_unlock(&fblog_registration_lock);
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 11/11] MAINTAINERS: add fblog entry
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (9 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 10/11] fblog: draw console to framebuffers David Herrmann
@ 2012-08-12 14:53 ` David Herrmann
2012-08-12 15:28 ` [PATCH 00/11] fblog: Framebuffer kernel log driver v4 Florian Tobias Schandinat
11 siblings, 0 replies; 22+ messages in thread
From: David Herrmann @ 2012-08-12 14:53 UTC (permalink / raw)
To: linux-fbdev
Cc: Florian Tobias Schandinat, Greg Kroah-Hartman, linux-serial,
Alan Cox, linux-kernel, Geert Uytterhoeven, David Herrmann
Add myself as maintainer for the fblog driver to the MAINTAINERS file.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 375b1fd..40a10b54 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2895,6 +2895,12 @@ F: drivers/video/
F: include/video/
F: include/linux/fb.h
+FRAMEBUFFER LOG DRIVER
+M: David Herrmann <dh.herrmann@googlemail.com>
+L: linux-fbdev@vger.kernel.org
+S: Maintained
+F: drivers/video/console/fblog.c
+
FREESCALE DMA DRIVER
M: Li Yang <leoli@freescale.com>
M: Zhang Wei <zw@zh-kernel.org>
--
1.7.11.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 00/11] fblog: Framebuffer kernel log driver v4
2012-08-12 14:53 [PATCH 00/11] fblog: Framebuffer kernel log driver v4 David Herrmann
` (10 preceding siblings ...)
2012-08-12 14:53 ` [PATCH 11/11] MAINTAINERS: add fblog entry David Herrmann
@ 2012-08-12 15:28 ` Florian Tobias Schandinat
11 siblings, 0 replies; 22+ messages in thread
From: Florian Tobias Schandinat @ 2012-08-12 15:28 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Greg Kroah-Hartman, linux-serial, Alan Cox,
linux-kernel, Geert Uytterhoeven
On 08/12/2012 02:53 PM, David Herrmann wrote:
> Hi
>
> This is revision 4 of the fblog driver. It is a replacement for fbcon for
> systems that do not want/need CONFIG_VT. It simply prints the kernel-log to all
> connected framebuffers. Previous versions are available here:
> v3: http://thread.gmane.org/gmane.linux.kernel/1328164
> v2: http://thread.gmane.org/gmane.linux.serial/8133
> v1: http://marc.info/?l=linux-kernel&m=133988465602225&w=2
>
> This patchset is based on linux-next.
>
> Changes from previous version:
> - Changed mailinglist to linux-fbdev instead of linux-serial
> - Added "main_only" and "activate_on_hotplug" module parameters to provide
> more fine-grained control over which framebuffers are used
>
> As always simply remove the !VT dependency if you want to test this with VTs
> enabled. Some more background information on deprecating CONFIG_VT can be found
> here:
> http://dvdhrm.wordpress.com/2012/08/12/killing-off-config_vt/
>
> I am still looking for someone who is willing to push this to linux-next through
> his or her tree. Feedback is much appreciated!
I'll take care of this. But I'm currently very busy writing a thesis, so
it could take some time. It would be great if someone would find time to
give this a complete thorough review.
Thanks,
Florian Tobias Schandinat
>
> Thanks
> David
>
> David Herrmann (11):
> fbcon: move update_attr() into separate source file
> fbcon: move bit_putcs() into separate source file
> fblog: new framebuffer kernel log dummy driver
> fbdev: export get_fb_info()/put_fb_info()
> fblog: register one fblog object per framebuffer
> fblog: open fb on registration
> fblog: allow selecting fbs via sysfs and module-parameters
> fblog: cache framebuffer BLANK and SUSPEND states
> fblog: register console driver
> fblog: draw console to framebuffers
> MAINTAINERS: add fblog entry
>
> MAINTAINERS | 6 +
> drivers/video/Kconfig | 5 +-
> drivers/video/Makefile | 2 +-
> drivers/video/console/Kconfig | 37 ++-
> drivers/video/console/Makefile | 4 +-
> drivers/video/console/bitblit.c | 149 +--------
> drivers/video/console/fbcon.h | 5 +-
> drivers/video/console/fbdraw.c | 171 ++++++++++
> drivers/video/console/fbdraw.h | 30 ++
> drivers/video/console/fblog.c | 691 ++++++++++++++++++++++++++++++++++++++++
> drivers/video/fbmem.c | 6 +-
> include/linux/fb.h | 3 +
> 12 files changed, 951 insertions(+), 158 deletions(-)
> create mode 100644 drivers/video/console/fbdraw.c
> create mode 100644 drivers/video/console/fbdraw.h
> create mode 100644 drivers/video/console/fblog.c
>
^ permalink raw reply [flat|nested] 22+ messages in thread