[PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode

[PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode

[Nicolas Iooss]

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.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/edac/sb_edac.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 54ae6dc45ab2..15a068744c25 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -304,7 +304,7 @@ struct sbridge_info {
 	u64		(*rir_limit)(u32 reg);
 	u64		(*sad_limit)(u32 reg);
 	u32		(*interleave_mode)(u32 reg);
-	char*		(*show_interleave_mode)(u32 reg);
+	const char*	(*show_interleave_mode)(u32 reg);
 	u32		(*dram_attr)(u32 reg);
 	const u32	*dram_rule;
 	const u32	*interleave_list;
@@ -811,7 +811,7 @@ static u32 interleave_mode(u32 reg)
 	return GET_BITFIELD(reg, 1, 1);
 }
 
-char *show_interleave_mode(u32 reg)
+static const char *show_interleave_mode(u32 reg)
 {
 	return interleave_mode(reg) ? "8:6" : "[8:6]XOR[18:16]";
 }
@@ -831,9 +831,9 @@ static u32 knl_interleave_mode(u32 reg)
 	return GET_BITFIELD(reg, 1, 2);
 }
 
-static char *knl_show_interleave_mode(u32 reg)
+static const char *knl_show_interleave_mode(u32 reg)
 {
-	char *s;
+	const char *s;
 
 	switch (knl_interleave_mode(reg)) {
 	case 0:
@@ -850,6 +850,7 @@ static char *knl_show_interleave_mode(u32 reg)
 		break;
 	default:
 		WARN_ON(1);
+		s = "";
 		break;
 	}
 
-- 
2.11.0

Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode

[Borislav Petkov]

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.

Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode

[Nicolas Iooss]

On 22/01/17 15:41, Borislav Petkov wrote:
> 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.

Thanks for your quick reply! I agree with your proposal and will send an
other patch which implements it.

In your reply, there is one point I have not understood. When doing "if
(!is_knl && !interleave_mode(reg))", the condition assumes that
interleave mode 1 has the same meaning on all kinds of CPUs. However the
current code prints this mode as "[10:8]" on KNL and "8:6" anywhere
else. So I would rather modify the code this way:

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

Would this be good for you?

Thanks

Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode

[Borislav Petkov]

On Sun, Jan 22, 2017 at 04:50:31PM +0100, Nicolas Iooss wrote:
>  	if (!is_knl)
>  		return interleave_mode(reg) ?
> 			"[8:6]" : "[8:6]XOR[18:16]";
>  	else
>  		return knl_intlv_mode[knl_interleave_mode(reg)];
> 
> Would this be good for you?

Ah, yes we interleave on address bits [8:6] when the bitield is 0 on KNL
while bitfield 0 for the rest of the families gives interleave mode for
the XOR of bits [8:6] and [18:16]. Good catch.

Thanks.

-- 
Regards/Gruss,
    Boris.

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