Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[David Rientjes]

On Tue, 18 Oct 2011, Borislav Petkov wrote:

> > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > > index 8af6fa4..ad8d897 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > > @@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
> > >  	 * Note this output is parsed by external tools and old fields
> > >  	 * should not be changed.
> > >  	 */
> > > -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
> > > +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
> > >  		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
> > >  		cpu_data(m->extcpu).microcode);
> > >  
> > 
> > Any reason why this isn't prefixed with "0x"?
> 
> Well, no strong reason except that APIC is without '0x' and I leaned
> towards the same for 'microcode'. And since this output format is legacy
> and MCE stanzas are being parsed by scripts, keeping the format for new
> fields sounded like the right thing to do, IMHO.
> 

Anytime there's a string that prints decimal, then hex, then decimal, then 
decimal, then hex, then hex, I think it's always better to include a 
prefix where it's not clear.  It's printed here without the prefix and in 
other places with the prefix, so I think it would be better to just be as 
explicit as possible.

And, the argument that scripts are parsing this is actually bogus since it 
would be expecting decimal there and you'd actually be doing them a favor 
by breaking if they can't handle the "0x" since you've changed it to hex.  
I know the comment says not to change old fields, but the microcode field 
hasn't hit Linus' tree yet, either.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/