* [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
@ 2017-01-22 13:51 Nicolas Iooss
2017-01-22 14:41 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2017-01-22 13:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Borislav Petkov, linux-edac
Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
2017-01-22 13:51 [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode Nicolas Iooss
@ 2017-01-22 14:41 ` Borislav Petkov
2017-01-22 15:50 ` Nicolas Iooss
0 siblings, 1 reply; 4+ messages in thread
From: Borislav Petkov @ 2017-01-22 14:41 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
2017-01-22 14:41 ` Borislav Petkov
@ 2017-01-22 15:50 ` Nicolas Iooss
2017-01-22 16:10 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2017-01-22 15:50 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
2017-01-22 15:50 ` Nicolas Iooss
@ 2017-01-22 16:10 ` Borislav Petkov
0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2017-01-22 16:10 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: Mauro Carvalho Chehab, linux-edac, linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-22 16:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-22 13:51 [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode Nicolas Iooss
2017-01-22 14:41 ` Borislav Petkov
2017-01-22 15:50 ` Nicolas Iooss
2017-01-22 16:10 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).