Date:   Tue, 30 Nov 2021 13:34:22 -0600
From:   Steve Wahl <steve.wahl@hpe.com>
To:     Colin Ian King <colin.i.king@googlemail.com>
Cc:     Steve Wahl <steve.wahl@hpe.com>, Mike Travis <mike.travis@hpe.com>,
        Dimitri Sivanich <dimitri.sivanich@hpe.com>,
        Russ Anderson <russ.anderson@hpe.com>,
        Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H . Peter Anvin" <hpa@zytor.com>,
        platform-driver-x86@vger.kernel.org,
        kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/platform/uv: make const pointer dots a static const
 array
Message-ID: <YaZ8vtajDKUVDCne@swahl-home.5wahls.com>
References: <20211127170320.77963-1-colin.i.king@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211127170320.77963-1-colin.i.king@gmail.com>
Precedence: bulk

On Sat, Nov 27, 2021 at 05:03:20PM +0000, Colin Ian King wrote:
> Don't populate the const array dots on the stack

That is a misunderstanding of what the original code does.  The
original code has a constant char array (string constant) that is
placed in an initialized data section of memory, the address off which
would be assigned to the pointer "dots" on the stack -- to be clear,
stack contents would not be a full array, but a pointer to it.  Then
that pointer would be passed to the pr_info function (which boils down
to a call to printk).

Examination of the disassembly shows that the compiler actually
eliminates the creation of the pointer "dots" on the stack and just
passes the address of the string constant to the printk function.

So this change should not have any actual effect (I don't know where
you got the "shrinks object code" from), and in my humble opinion
makes the code less clear.

As such, unless there's something here I don't understand, I vote to
reject this patch.

--> Steve Wahl <steve.wahl@hpe.com>

> but make it static
> const and make the pointer an array to remove a dereference. Shrinks
> object code a few bytes too.
> 
> Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> ---
>  arch/x86/platform/uv/uv_nmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 1e9ff28bc2e0..2c69a0c30632 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -725,7 +725,7 @@ static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
>   */
>  static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
>  {
> -	const char *dots = " ................................. ";
> +	static const char dots[] = " ................................. ";
>  
>  	if (cpu == 0)
>  		uv_nmi_dump_cpu_ip_hdr();
> -- 
> 2.33.1
> 

-- 
Steve Wahl, Hewlett Packard Enterprise