Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 18 Jun 2018 21:50:08 -0400 (EDT)
From:   Nicolas Pitre <nicolas.pitre@linaro.org>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dave Mielke <Dave@mielke.cc>,
        Samuel Thibault <samuel.thibault@ens-lyon.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/4] vt: coherence validation code for the unicode
 screen buffer
In-Reply-To: <CAHp75VfG_4zw+NPuHTUyH-gzdktiCKVcnWcwo0D_fAgfPbdk+A@mail.gmail.com>
Message-ID: <nycvar.YSQ.7.76.1806182110450.16670@knanqh.ubzr>
References: <20180617190706.14614-1-nicolas.pitre@linaro.org> <20180617190706.14614-5-nicolas.pitre@linaro.org> <CAHp75VfG_4zw+NPuHTUyH-gzdktiCKVcnWcwo0D_fAgfPbdk+A@mail.gmail.com>
User-Agent: Alpine 2.21 (LFD 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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