[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Daniel Mack]

The revision field in /proc/cpuinfo is reported as hex number, so it
should have a '0x' prefix to make that clear. Otherwise, parsers
will take it as decimal number unless it contains a letter.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Sven Neumann <s.neumann@raumfeld.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/kernel/setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index c6c57b6..73eb2c8 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -860,7 +860,7 @@ static int c_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 
 	seq_printf(m, "Hardware\t: %s\n", machine_name);
-	seq_printf(m, "Revision\t: %04x\n", system_rev);
+	seq_printf(m, "Revision\t: 0x%04x\n", system_rev);
 	seq_printf(m, "Serial\t\t: %08x%08x\n",
 		   system_serial_high, system_serial_low);
 
-- 
1.6.5.2

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Alessandro Rubini]

> The revision field in /proc/cpuinfo is reported as hex number, so it
> should have a '0x' prefix to make that clear. Otherwise, parsers
> will take it as decimal number unless it contains a letter.

If the problem is real (I assume it is), why don't you fix all
fields?

> -	seq_printf(m, "Revision\t: %04x\n", system_rev);
> +	seq_printf(m, "Revision\t: 0x%04x\n", system_rev);
>  	seq_printf(m, "Serial\t\t: %08x%08x\n",
>  		   system_serial_high, system_serial_low);

"Serial", above, is hex as well.

thanks
/alessandro

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Daniel Mack]

On Thu, Nov 12, 2009 at 03:30:20PM +0100, Alessandro Rubini wrote:
> > The revision field in /proc/cpuinfo is reported as hex number, so it
> > should have a '0x' prefix to make that clear. Otherwise, parsers
> > will take it as decimal number unless it contains a letter.
> 
> If the problem is real (I assume it is), why don't you fix all
> fields?
> 
> > -	seq_printf(m, "Revision\t: %04x\n", system_rev);
> > +	seq_printf(m, "Revision\t: 0x%04x\n", system_rev);
> >  	seq_printf(m, "Serial\t\t: %08x%08x\n",
> >  		   system_serial_high, system_serial_low);
> 
> "Serial", above, is hex as well.

Yes, but the serial number is more likely to be taken as string, I
thought. Anyway, below is a new patch.

Daniel

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Russell King - ARM Linux]

On Thu, Nov 12, 2009 at 02:58:07PM +0100, Daniel Mack wrote:
> The revision field in /proc/cpuinfo is reported as hex number, so it
> should have a '0x' prefix to make that clear. Otherwise, parsers
> will take it as decimal number unless it contains a letter.

So what you're doing is possibly breaking existing parsers (which
probably already assume it is hex) to fix buggy parsers?

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Daniel Mack]

Hi Russell,

On Thu, Nov 12, 2009 at 09:52:01PM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 12, 2009 at 02:58:07PM +0100, Daniel Mack wrote:
> > The revision field in /proc/cpuinfo is reported as hex number, so it
> > should have a '0x' prefix to make that clear. Otherwise, parsers
> > will take it as decimal number unless it contains a letter.
> 
> So what you're doing is possibly breaking existing parsers (which
> probably already assume it is hex) to fix buggy parsers?

I wouldn't call that fixing a buggy parser. On my board, the revision is
0x101 which was parsed as 101. You can't know it is in hex unless you
look at the sources.

Daniel

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Russell King - ARM Linux]

On Thu, Nov 12, 2009 at 11:03:14PM +0100, Daniel Mack wrote:
> Hi Russell,
> 
> On Thu, Nov 12, 2009 at 09:52:01PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Nov 12, 2009 at 02:58:07PM +0100, Daniel Mack wrote:
> > > The revision field in /proc/cpuinfo is reported as hex number, so it
> > > should have a '0x' prefix to make that clear. Otherwise, parsers
> > > will take it as decimal number unless it contains a letter.
> > 
> > So what you're doing is possibly breaking existing parsers (which
> > probably already assume it is hex) to fix buggy parsers?
> 
> I wouldn't call that fixing a buggy parser. On my board, the revision is
> 0x101 which was parsed as 101. You can't know it is in hex unless you
> look at the sources.

However, we export lots of stuff from the kernel in hex without an '0x'
prefix.

The biggest point here, though, is that the field is already present and
has been used, created about 10 years ago for NetWinder stuff.  So we've
about 10 years of history of it being there.  Adding the '0x' prefix may
break existing parsers, which is a good enough reason not to change it.

Moreover, if you try to parse the field using strtoul, you'll generally
end up with it being interpreted as an _octal_ number due to the leading
zeros - which should be enough of a hint that it isn't a decimal number.

If you really feel strongly about it, then the only portable way of fixing
it would be to create a new entry in the file with the '0x' prefix.

[PATCH] ARM: Prefix revision number in /proc/cpuinfo

[Daniel Mack]

On Thu, Nov 12, 2009 at 10:13:07PM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 12, 2009 at 11:03:14PM +0100, Daniel Mack wrote:
> > On Thu, Nov 12, 2009 at 09:52:01PM +0000, Russell King - ARM Linux wrote:
> > > On Thu, Nov 12, 2009 at 02:58:07PM +0100, Daniel Mack wrote:
> > > > The revision field in /proc/cpuinfo is reported as hex number, so it
> > > > should have a '0x' prefix to make that clear. Otherwise, parsers
> > > > will take it as decimal number unless it contains a letter.
> > > 
> > > So what you're doing is possibly breaking existing parsers (which
> > > probably already assume it is hex) to fix buggy parsers?
> > 
> > I wouldn't call that fixing a buggy parser. On my board, the revision is
> > 0x101 which was parsed as 101. You can't know it is in hex unless you
> > look at the sources.
> 
> However, we export lots of stuff from the kernel in hex without an '0x'
> prefix.
> 
> The biggest point here, though, is that the field is already present and
> has been used, created about 10 years ago for NetWinder stuff.  So we've
> about 10 years of history of it being there.  Adding the '0x' prefix may
> break existing parsers, which is a good enough reason not to change it.
> 
> Moreover, if you try to parse the field using strtoul, you'll generally
> end up with it being interpreted as an _octal_ number due to the leading
> zeros - which should be enough of a hint that it isn't a decimal number.
> 
> If you really feel strongly about it, then the only portable way of fixing
> it would be to create a new entry in the file with the '0x' prefix.

Hmm, no. I think we will just force the parser to hex then. Not that
important. It's just that all other fields in /proc/cpuinfo which are in
hex _are_ prefixed indeed, but I see the risk of breaking existing
parsers, so let's keep it the way it is.

Thanks,
Daniel