diff mbox series

[v2,4/4] vt: coherence validation code for the unicode screen buffer

Message ID 20180617190706.14614-5-nicolas.pitre@linaro.org
State New
Headers show
Series have the vt console preserve unicode characters | expand

Commit Message

Nicolas Pitre June 17, 2018, 7:07 p.m. UTC 
Make sure the unicode screen buffer matches the video screen content.
This is provided for debugging convenience and disabled by default.

Signed-off-by: Nicolas Pitre <nico@linaro.org>

---
 drivers/tty/vt/vt.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

-- 
2.17.1

Comments

Andy Shevchenko June 18, 2018, 10:01 p.m. UTC | #1 
On Sun, Jun 17, 2018 at 10:07 PM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:
> Make sure the unicode screen buffer matches the video screen content.

> This is provided for debugging convenience and disabled by default.


> +#define VC_UNI_SCREEN_DEBUG 0


> +       if (!VC_UNI_SCREEN_DEBUG || !uniscr)

> +               return;


Hmm... Interesting. I would rather go with
#ifdef ..._DEBUG
...
#else
return;
#endif

It will relax requirement on how to define _DEBUG. I don't recall I
see something like you proposing in the kernel for the same purpose.

> +

> +       WARN_CONSOLE_UNLOCKED();

> +

> +       /*

> +        * Make sure our unicode screen translates into the same glyphs

> +        * as the actual screen. This is brutal indeed.

> +        */

> +       p = (unsigned short *)vc->vc_origin;

> +       mask = vc->vc_hi_font_mask | 0xff;

> +       for (y = 0; y < vc->vc_rows; y++) {

> +               char32_t *line = uniscr->lines[y];

> +               for (x = 0; x < vc->vc_cols; x++) {

> +                       u16 glyph = scr_readw(p++) & mask;

> +                       char32_t uc = line[x];

> +                       int tc = conv_uni_to_pc(vc, uc);

> +                       if (tc == -4)

> +                               tc = conv_uni_to_pc(vc, 0xfffd);

> +                       if (tc == -4)

> +                               tc = conv_uni_to_pc(vc, '?');

> +                       if (tc != glyph)


> +                               pr_notice("%s: mismatch at %d,%d: "

> +                                         "glyph=%#x tc=%#x\n", __func__,

> +                                         x, y, glyph, tc);


Don't split format string in printk(). checkpatch will not warn on longer lines.

> +               }

> +       }


-- 
With Best Regards,
Andy Shevchenko
Nicolas Pitre June 19, 2018, 1:50 a.m. UTC | #2 
On Tue, 19 Jun 2018, Andy Shevchenko wrote:

> On Sun, Jun 17, 2018 at 10:07 PM, Nicolas Pitre

> <nicolas.pitre@linaro.org> wrote:

> > Make sure the unicode screen buffer matches the video screen content.

> > This is provided for debugging convenience and disabled by default.

> 

> > +#define VC_UNI_SCREEN_DEBUG 0

> 

> > +       if (!VC_UNI_SCREEN_DEBUG || !uniscr)

> > +               return;

> 

> Hmm... Interesting. I would rather go with

> #ifdef ..._DEBUG

> ...

> #else

> return;

> #endif

> 

> It will relax requirement on how to define _DEBUG. I don't recall I

> see something like you proposing in the kernel for the same purpose.


Some random examples:

include/crypto/scatterwalk.h:106;
                if (ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE && !PageSlab(page))

include/math-emu/single.h:68:
    if (!FP_INHIBIT_RESULTS)

This form also allows for the compiler to parse and validate the code 
whether or not the feature is enabled, while still optimizing it away in 
the end if not enabled.

> > +

> > +       WARN_CONSOLE_UNLOCKED();

> > +

> > +       /*

> > +        * Make sure our unicode screen translates into the same glyphs

> > +        * as the actual screen. This is brutal indeed.

> > +        */

> > +       p = (unsigned short *)vc->vc_origin;

> > +       mask = vc->vc_hi_font_mask | 0xff;

> > +       for (y = 0; y < vc->vc_rows; y++) {

> > +               char32_t *line = uniscr->lines[y];

> > +               for (x = 0; x < vc->vc_cols; x++) {

> > +                       u16 glyph = scr_readw(p++) & mask;

> > +                       char32_t uc = line[x];

> > +                       int tc = conv_uni_to_pc(vc, uc);

> > +                       if (tc == -4)

> > +                               tc = conv_uni_to_pc(vc, 0xfffd);

> > +                       if (tc == -4)

> > +                               tc = conv_uni_to_pc(vc, '?');

> > +                       if (tc != glyph)

> 

> > +                               pr_notice("%s: mismatch at %d,%d: "

> > +                                         "glyph=%#x tc=%#x\n", __func__,

> > +                                         x, y, glyph, tc);

> 

> Don't split format string in printk(). checkpatch will not warn on longer lines.


I didn't do it like that for checkpatch but to keep the code readable.
I don't particularly care either ways though.


Nicolas
Joe Perches June 19, 2018, 4:52 a.m. UTC | #3 
On Mon, 2018-06-18 at 21:50 -0400, Nicolas Pitre wrote:
> On Tue, 19 Jun 2018, Andy Shevchenko wrote:

[]
> > > +       /*

> > > +        * Make sure our unicode screen translates into the same glyphs

> > > +        * as the actual screen. This is brutal indeed.

> > > +        */

> > > +       p = (unsigned short *)vc->vc_origin;

> > > +       mask = vc->vc_hi_font_mask | 0xff;

> > > +       for (y = 0; y < vc->vc_rows; y++) {

> > > +               char32_t *line = uniscr->lines[y];

> > > +               for (x = 0; x < vc->vc_cols; x++) {

> > > +                       u16 glyph = scr_readw(p++) & mask;

> > > +                       char32_t uc = line[x];

> > > +                       int tc = conv_uni_to_pc(vc, uc);

> > > +                       if (tc == -4)

> > > +                               tc = conv_uni_to_pc(vc, 0xfffd);

> > > +                       if (tc == -4)

> > > +                               tc = conv_uni_to_pc(vc, '?');

> > > +                       if (tc != glyph)

> > > +                               pr_notice("%s: mismatch at %d,%d: "

> > > +                                         "glyph=%#x tc=%#x\n", __func__,

> > > +                                         x, y, glyph, tc);

> > 

> > Don't split format string in printk(). checkpatch will not warn on longer lines.

> 

> I didn't do it like that for checkpatch but to keep the code readable.

> I don't particularly care either ways though.


If one glyph is off, then perhaps others are off too.
Perhaps this message should be ratelimited.
Nicolas Pitre June 19, 2018, 12:14 p.m. UTC | #4 
On Mon, 18 Jun 2018, Joe Perches wrote:

> On Mon, 2018-06-18 at 21:50 -0400, Nicolas Pitre wrote:

> > On Tue, 19 Jun 2018, Andy Shevchenko wrote:

> []

> > > > +       /*

> > > > +        * Make sure our unicode screen translates into the same glyphs

> > > > +        * as the actual screen. This is brutal indeed.

> > > > +        */

> > > > +       p = (unsigned short *)vc->vc_origin;

> > > > +       mask = vc->vc_hi_font_mask | 0xff;

> > > > +       for (y = 0; y < vc->vc_rows; y++) {

> > > > +               char32_t *line = uniscr->lines[y];

> > > > +               for (x = 0; x < vc->vc_cols; x++) {

> > > > +                       u16 glyph = scr_readw(p++) & mask;

> > > > +                       char32_t uc = line[x];

> > > > +                       int tc = conv_uni_to_pc(vc, uc);

> > > > +                       if (tc == -4)

> > > > +                               tc = conv_uni_to_pc(vc, 0xfffd);

> > > > +                       if (tc == -4)

> > > > +                               tc = conv_uni_to_pc(vc, '?');

> > > > +                       if (tc != glyph)

> > > > +                               pr_notice("%s: mismatch at %d,%d: "

> > > > +                                         "glyph=%#x tc=%#x\n", __func__,

> > > > +                                         x, y, glyph, tc);

> > > 

> > > Don't split format string in printk(). checkpatch will not warn on longer lines.

> > 

> > I didn't do it like that for checkpatch but to keep the code readable.

> > I don't particularly care either ways though.

> 

> If one glyph is off, then perhaps others are off too.

> Perhaps this message should be ratelimited.


Remember that this is costly debugging code that is off by default. 
No production kernel should have it enabled.


Nicolas
diff mbox series

Patch

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 2d14bb195d..81129671c9 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -321,6 +321,8 @@  void schedule_console_callback(void)
  * Code to manage unicode-based screen buffers
  */
 
+#define VC_UNI_SCREEN_DEBUG 0
+
 #ifdef NO_VC_UNI_SCREEN
 /* this disables and optimizes related code away at compile time */
 #define get_vc_uniscr(vc) NULL
@@ -569,6 +571,42 @@  void vc_uniscr_copy_line(struct vc_data *vc, void *dest, int viewed,
 	}
 }
 
+/* this is for validation and debugging only */
+static void vc_uniscr_debug_check(struct vc_data *vc)
+{
+	struct uni_screen *uniscr = get_vc_uniscr(vc);
+	unsigned short *p;
+	int x, y, mask;
+
+	if (!VC_UNI_SCREEN_DEBUG || !uniscr)
+		return;
+
+	WARN_CONSOLE_UNLOCKED();
+
+	/*
+	 * Make sure our unicode screen translates into the same glyphs
+	 * as the actual screen. This is brutal indeed.
+	 */
+	p = (unsigned short *)vc->vc_origin;
+	mask = vc->vc_hi_font_mask | 0xff;
+	for (y = 0; y < vc->vc_rows; y++) {
+		char32_t *line = uniscr->lines[y];
+		for (x = 0; x < vc->vc_cols; x++) {
+			u16 glyph = scr_readw(p++) & mask;
+			char32_t uc = line[x];
+			int tc = conv_uni_to_pc(vc, uc);
+			if (tc == -4)
+				tc = conv_uni_to_pc(vc, 0xfffd);
+			if (tc == -4)
+				tc = conv_uni_to_pc(vc, '?');
+			if (tc != glyph)
+				pr_notice("%s: mismatch at %d,%d: "
+					  "glyph=%#x tc=%#x\n", __func__,
+					  x, y, glyph, tc);
+		}
+	}
+}
+
 
 static void con_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
 		enum con_scroll dir, unsigned int nr)
@@ -2717,6 +2755,7 @@  static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int co
 		do_con_trol(tty, vc, orig);
 	}
 	con_flush(vc, draw_from, draw_to, &draw_x);
+	vc_uniscr_debug_check(vc);
 	console_conditional_schedule();
 	console_unlock();
 	notify_update(vc);