From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751616AbdAVOmA (ORCPT <rfc822;w@1wt.eu>);
        Sun, 22 Jan 2017 09:42:00 -0500
Received: from mail.skyhub.de ([78.46.96.112]:60168 "EHLO mail.skyhub.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750907AbdAVOlv (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Sun, 22 Jan 2017 09:41:51 -0500
Date: Sun, 22 Jan 2017 15:41:44 +0100
From: Borislav Petkov <bp@alien8.de>
To: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, linux-edac@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] EDAC: always return an initialized value in
 knl_show_interleave_mode
Message-ID: <20170122144144.bwgkh3esdcveim3q@pd.tnic>
References: <20170122135115.13256-1-nicolas.iooss_linux@m4x.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170122135115.13256-1-nicolas.iooss_linux@m4x.org>
User-Agent: NeoMutt/20161014 (1.7.1)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Jan 22, 2017 at 02:51:15PM +0100, Nicolas Iooss wrote:
> When building drivers/edac/sb_edac.c with compiler warning flags which
> aim to detect the use of uninitialized values at compile time, the
> compiler reports that knl_show_interleave_mode() may return an
> uninitialized value. This function indeed uses a switch statement to set
> a local variable ("s"), which is not set to anything in the default
> statement. Anyway this would be never reached as
> knl_interleave_mode(reg) always returns a value between 0 and 3, but the
> compiler has no way of knowing this.
> 
> Silent the compiler warning by initializing variable "s" too in the
> default case. While at it, make knl_show_interleave_mode() and
> show_interleave_mode() return const char* values as the returned
> pointers refer to read-only memory.

No, this is trying to make pretty an already ugly pile of crap.

That ->show_interleave_mode() is called only once in a debug printk. Big
deal. But it is a function pointer which points to the same function
except for KNL.

Then, the default implementation returns bit slices: "8:6" :
"[8:6]XOR[18:16]" but the KNL one says "use address bits" like this is
the SDM. Yeah, right. I should've caught that when the KNL pile was
added.

So here's what I'd like you to do, instead:

Kill that ->show_interleave_mode() thing and use the default
show_interleave_mode() at the only call site. You can pass in a second
bool argument is_knl which is "pvt->info.type == KNIGHTS_LANDING".

And then add the KNL functionality from knl_show_interleave_mode() to
the default show_interleave_mode().

And get rid of that default: case - it won't be reached anyway.

You can even define a string array:

struct pair intlv_mode[] = {
	"[8:6]", "[10:8]", ...;
};

and then do:

	if (!is_knl && !interleave_mode(reg))
		return "[8:6]XOR[18:16]";
	else
		return intlv_mode[knl_interleave_mode()];

and be done with it.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.