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
