Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210322164308.827846-1-arnd@kernel.org>
In-Reply-To: <20210322164308.827846-1-arnd@kernel.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 22 Mar 2021 10:04:57 -0700
Message-ID: <CAD=FV=WY6yxx+vkH+UU4VYei29xBftdnyRBE1OpEELmJ-kLfFg@mail.gmail.com>
Subject: Re: [PATCH] kgdb: fix gcc-11 warning on indentation
To:     Arnd Bergmann <arnd@kernel.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ingo Molnar <mingo@elte.hu>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        kgdb-bugreport@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> -#define v1printk(a...) do { \
> -       if (verbose) \
> -               printk(KERN_INFO a); \
> -       } while (0)
> -#define v2printk(a...) do { \
> -       if (verbose > 1) \
> -               printk(KERN_INFO a); \
> -               touch_nmi_watchdog();   \
> -       } while (0)
> -#define eprintk(a...) do { \
> -               printk(KERN_ERR a); \
> -               WARN_ON(1); \
> -       } while (0)
> +#define v1printk(a...) do {            \

nit: In addition to the indentation change you're also lining up the
backslashes. Is that just personal preference, or is there some
official recommendation in the kernel? I don't really have a strong
opinion either way (IMO each style has its advantages).


> +       if (verbose)                    \
> +               printk(KERN_INFO a);    \
> +} while (0)
> +#define v2printk(a...) do {            \
> +       if (verbose > 1)                \
> +               printk(KERN_INFO a);    \
> +       touch_nmi_watchdog();           \

This touch_nmi_watchdog() is pretty wonky. I guess maybe the
assumption is that the "verbose level 2" prints are so chatty that the
printing might prevent us from touching the NMI watchdog in the way
that we normally do and thus we need an extra one here?

...but, in that case, I think the old code was _wrong_ and that the
intention was that the touch_nmi_watchdog() should only be if "verose
> 1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.

-Doug