[PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Aleksandar Markovic]

From: Dragan Cecavac <dragan.cecavac@mips.com>

Remove unnecessary space from FPU info segment of /proc/cpuinfo.

Signed-off-by: Dragan Cecavac <dragan.cecavac@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 arch/mips/kernel/proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index bd9bf52..99f9aab 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c
@@ -58,7 +58,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 
 	seq_printf(m, "processor\t\t: %ld\n", n);
 	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
-		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
+		      cpu_data[n].options & MIPS_CPU_FPU ? " FPU V%d.%d" : "");
 	seq_printf(m, fmt, __cpu_name[n],
 		      (version >> 4) & 0x0f, version & 0x0f,
 		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);
-- 
2.7.4

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Joe Perches]

On Fri, 2017-10-20 at 16:20 +0200, Aleksandar Markovic wrote:
> From: Dragan Cecavac <dragan.cecavac@mips.com>
> 
> Remove unnecessary space from FPU info segment of /proc/cpuinfo.
> 
> Signed-off-by: Dragan Cecavac <dragan.cecavac@mips.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
> ---
>  arch/mips/kernel/proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
> index bd9bf52..99f9aab 100644
> --- a/arch/mips/kernel/proc.c
> +++ b/arch/mips/kernel/proc.c
> @@ -58,7 +58,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  
>  	seq_printf(m, "processor\t\t: %ld\n", n);
>  	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
> -		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
> +		      cpu_data[n].options & MIPS_CPU_FPU ? " FPU V%d.%d" : "");
>  	seq_printf(m, fmt, __cpu_name[n],
>  		      (version >> 4) & 0x0f, version & 0x0f,
>  		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);

That's somewhat unpleasant code as it formats a fmt string
and the compiler can not verify fmt and args.

Perhaps something like the below is preferable:
---
 arch/mips/kernel/proc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
index bd9bf528f19b..dda4af8fcd7b 100644
--- a/arch/mips/kernel/proc.c
+++ b/arch/mips/kernel/proc.c
@@ -38,7 +38,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 	unsigned long n = (unsigned long) v - 1;
 	unsigned int version = cpu_data[n].processor_id;
 	unsigned int fp_vers = cpu_data[n].fpu_id;
-	char fmt [64];
 	int i;
 
 #ifdef CONFIG_SMP
@@ -57,11 +56,13 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 	}
 
 	seq_printf(m, "processor\t\t: %ld\n", n);
-	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
-		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
-	seq_printf(m, fmt, __cpu_name[n],
-		      (version >> 4) & 0x0f, version & 0x0f,
+	seq_printf(m, "cpu model\t\t: %s V%d.%d",
+		   __cpu_name[n], (version >> 4) & 0x0f, version & 0x0f);
+	if (cpu_data[n].options & MIPS_CPU_FPU)
+		seq_printf(m, " FPU V%d.%d\n",
 		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);
+	else
+		seq_printf(m, "\n");
 	seq_printf(m, "BogoMIPS\t\t: %u.%02u\n",
 		      cpu_data[n].udelay_val / (500000/HZ),
 		      (cpu_data[n].udelay_val / (5000/HZ)) % 100);
@@ -143,10 +144,13 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		seq_printf(m, "VP\t\t\t: %d\n", cpu_vpe_id(&cpu_data[n]));
 #endif
 
-	sprintf(fmt, "VCE%%c exceptions\t\t: %s\n",
-		      cpu_has_vce ? "%u" : "not available");
-	seq_printf(m, fmt, 'D', vced_count);
-	seq_printf(m, fmt, 'I', vcei_count);
+	if (cpu_has_vce) {
+		seq_printf(m, "VCE%c exceptions\t\t: %u\n", 'D', vced_count);
+		seq_printf(m, "VCE%c exceptions\t\t: %u\n", 'I', vcei_count);
+	} else {
+		seq_printf(m, "VCE%c exceptions\t\t: not available\n", 'D');
+		seq_printf(m, "VCE%c exceptions\t\t: not available\n", 'I');
+	}
 
 	proc_cpuinfo_notifier_args.m = m;
 	proc_cpuinfo_notifier_args.n = n;

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Joe Perches wrote:

> > diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
> > index bd9bf52..99f9aab 100644
> > --- a/arch/mips/kernel/proc.c
> > +++ b/arch/mips/kernel/proc.c
> > @@ -58,7 +58,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> >  
> >  	seq_printf(m, "processor\t\t: %ld\n", n);
> >  	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
> > -		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
> > +		      cpu_data[n].options & MIPS_CPU_FPU ? " FPU V%d.%d" : "");
> >  	seq_printf(m, fmt, __cpu_name[n],
> >  		      (version >> 4) & 0x0f, version & 0x0f,
> >  		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);
> 
> That's somewhat unpleasant code as it formats a fmt string
> and the compiler can not verify fmt and args.
> 
> Perhaps something like the below is preferable:

 Hmm, what problem exactly are you trying to solve with code that has 
worked just fine for 16 years now?

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Joe Perches wrote:

> > diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
> > index bd9bf52..99f9aab 100644
> > --- a/arch/mips/kernel/proc.c
> > +++ b/arch/mips/kernel/proc.c
> > @@ -58,7 +58,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> >  
> >  	seq_printf(m, "processor\t\t: %ld\n", n);
> >  	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
> > -		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
> > +		      cpu_data[n].options & MIPS_CPU_FPU ? " FPU V%d.%d" : "");
> >  	seq_printf(m, fmt, __cpu_name[n],
> >  		      (version >> 4) & 0x0f, version & 0x0f,
> >  		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);
> 
> That's somewhat unpleasant code as it formats a fmt string
> and the compiler can not verify fmt and args.
> 
> Perhaps something like the below is preferable:

 Hmm, what problem exactly are you trying to solve with code that has 
worked just fine for 16 years now?

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Joe Perches]

On Fri, 2017-10-20 at 21:52 +0100, Maciej W. Rozycki wrote:
> On Fri, 20 Oct 2017, Joe Perches wrote:
> 
> > > diff --git a/arch/mips/kernel/proc.c b/arch/mips/kernel/proc.c
> > > index bd9bf52..99f9aab 100644
> > > --- a/arch/mips/kernel/proc.c
> > > +++ b/arch/mips/kernel/proc.c
> > > @@ -58,7 +58,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> > >  
> > >  	seq_printf(m, "processor\t\t: %ld\n", n);
> > >  	sprintf(fmt, "cpu model\t\t: %%s V%%d.%%d%s\n",
> > > -		      cpu_data[n].options & MIPS_CPU_FPU ? "  FPU V%d.%d" : "");
> > > +		      cpu_data[n].options & MIPS_CPU_FPU ? " FPU V%d.%d" : "");
> > >  	seq_printf(m, fmt, __cpu_name[n],
> > >  		      (version >> 4) & 0x0f, version & 0x0f,
> > >  		      (fp_vers >> 4) & 0x0f, fp_vers & 0x0f);
> > 
> > That's somewhat unpleasant code as it formats a fmt string
> > and the compiler can not verify fmt and args.
> > 
> > Perhaps something like the below is preferable:
> 
>  Hmm, what problem exactly are you trying to solve with code that has 
> worked just fine for 16 years now?

The compiler cannot verify fmt and args.

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Joe Perches wrote:

> > > That's somewhat unpleasant code as it formats a fmt string
> > > and the compiler can not verify fmt and args.
> > > 
> > > Perhaps something like the below is preferable:
> > 
> >  Hmm, what problem exactly are you trying to solve with code that has 
> > worked just fine for 16 years now?
> 
> The compiler cannot verify fmt and args.

 You have stated that already.  Why is that a problem?

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Joe Perches wrote:

> > > That's somewhat unpleasant code as it formats a fmt string
> > > and the compiler can not verify fmt and args.
> > > 
> > > Perhaps something like the below is preferable:
> > 
> >  Hmm, what problem exactly are you trying to solve with code that has 
> > worked just fine for 16 years now?
> 
> The compiler cannot verify fmt and args.

 You have stated that already.  Why is that a problem?

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Joe Perches]

On Fri, 2017-10-20 at 22:46 +0100, Maciej W. Rozycki wrote:
> On Fri, 20 Oct 2017, Joe Perches wrote:
> 
> > > > That's somewhat unpleasant code as it formats a fmt string
> > > > and the compiler can not verify fmt and args.
> > > > 
> > > > Perhaps something like the below is preferable:
> > > 
> > >  Hmm, what problem exactly are you trying to solve with code that has 
> > > worked just fine for 16 years now?
> > 
> > The compiler cannot verify fmt and args.
> 
>  You have stated that already.  Why is that a problem?

Jeeze, perhaps you don't like the word perhaps.

There is no absolute defect here.

There are unnecessary pushes to stack that are
unwound by the compiler.

Stylistically, format/argument mismatches can
cause errors.  It's
generally bad form and error
prone to use non constant strings as
formats.

Note it's not signed and is a simple suggestion.
If you don't like it, don't do anything with it.

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Aleksandar Markovic wrote:

> Remove unnecessary space from FPU info segment of /proc/cpuinfo.

 NAK.  As I recall back in Nov 2001 I placed the extra space there to 
visually separate the CPU part from the FPU part, e.g.:

cpu model		: R3000A V3.0  FPU V4.0
cpu model		: SiByte SB1 V0.2  FPU V0.2

etc.  And the motivation behind it still stands.  Please remember that 
/proc/cpuinfo is there for live humans to parse and grouping all these 
pieces together would make it harder.  Which means your change adds no 
value I'm afraid.

 NB regrettably back in those days much of the patch traffic happened off 
any mailing list, however I have quickly tracked down my archival copy of 
the original submission of the change introducing this piece of code and
I'll be happy to share it with anyone upon request.

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Maciej W. Rozycki]

On Fri, 20 Oct 2017, Aleksandar Markovic wrote:

> Remove unnecessary space from FPU info segment of /proc/cpuinfo.

 NAK.  As I recall back in Nov 2001 I placed the extra space there to 
visually separate the CPU part from the FPU part, e.g.:

cpu model		: R3000A V3.0  FPU V4.0
cpu model		: SiByte SB1 V0.2  FPU V0.2

etc.  And the motivation behind it still stands.  Please remember that 
/proc/cpuinfo is there for live humans to parse and grouping all these 
pieces together would make it harder.  Which means your change adds no 
value I'm afraid.

 NB regrettably back in those days much of the patch traffic happened off 
any mailing list, however I have quickly tracked down my archival copy of 
the original submission of the change introducing this piece of code and
I'll be happy to share it with anyone upon request.

  Maciej

Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[David Daney]

On 10/20/2017 01:47 PM, Maciej W. Rozycki wrote:
> On Fri, 20 Oct 2017, Aleksandar Markovic wrote:
> 
>> Remove unnecessary space from FPU info segment of /proc/cpuinfo.
> 
>   NAK.  As I recall back in Nov 2001 I placed the extra space there to
> visually separate the CPU part from the FPU part, e.g.:
> 
> cpu model		: R3000A V3.0  FPU V4.0
> cpu model		: SiByte SB1 V0.2  FPU V0.2
> 
> etc.  And the motivation behind it still stands.  Please remember that
> /proc/cpuinfo is there for live humans to parse and grouping all these
> pieces together would make it harder.  Which means your change adds no
> value I'm afraid.

I think it is even riskier than that.  This is part of the 
kernel-userspace ABI, many programs parse this file, any gratuitous 
changes risk breaking something.

I don't really have an opinion about the various *printf functions being 
used, but think the resultant change in what is visible to userspace 
should not be done.

> 
>   NB regrettably back in those days much of the patch traffic happened off
> any mailing list, however I have quickly tracked down my archival copy of
> the original submission of the change introducing this piece of code and
> I'll be happy to share it with anyone upon request.
> 
>    Maciej
> 

RE: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

[Miodrag Dinic]

Hi,

the issue was found on Android using VTS, where its cpuinfo parser stumbled upon an extra space while trying to extract information about FPU.
By comparing with ARM and Intel it seemed that only MIPS had this quirk in the cpuinfo format, so we submitted this change to make it conform
to the format used by other architectures.  

However I agree that this is pretty sensitive code to userspace and it is better to leave it as is.
@Macijej, David, thank you for pointing out the risks and explanation of the origin of this extra space in the cpuinfo format.

Please ignore this change.

Kind regards,
Miodrag
________________________________________
From: David Daney [ddaney@caviumnetworks.com]
Sent: Saturday, October 21, 2017 1:56 AM
To: Maciej Rozycki; Aleksandar Markovic
Cc: linux-mips@linux-mips.org; Dragan Cecavac; Aleksandar Markovic; Douglas Leung; Goran Ferenc; James Hogan; James Hogan; linux-kernel@vger.kernel.org; Maciej W. Rozycki; Miodrag Dinic; Paul Burton; Paul Burton; Petar Jovanovic; Raghu Gandham; Ralf Baechle
Subject: Re: [PATCH] MIPS: kernel: proc: Remove spurious white space in cpuinfo

On 10/20/2017 01:47 PM, Maciej W. Rozycki wrote:
> On Fri, 20 Oct 2017, Aleksandar Markovic wrote:
>
>> Remove unnecessary space from FPU info segment of /proc/cpuinfo.
>
>   NAK.  As I recall back in Nov 2001 I placed the extra space there to
> visually separate the CPU part from the FPU part, e.g.:
>
> cpu model             : R3000A V3.0  FPU V4.0
> cpu model             : SiByte SB1 V0.2  FPU V0.2
>
> etc.  And the motivation behind it still stands.  Please remember that
> /proc/cpuinfo is there for live humans to parse and grouping all these
> pieces together would make it harder.  Which means your change adds no
> value I'm afraid.

I think it is even riskier than that.  This is part of the
kernel-userspace ABI, many programs parse this file, any gratuitous
changes risk breaking something.

I don't really have an opinion about the various *printf functions being
used, but think the resultant change in what is visible to userspace
should not be done.

>
>   NB regrettably back in those days much of the patch traffic happened off
> any mailing list, however I have quickly tracked down my archival copy of
> the original submission of the change introducing this piece of code and
> I'll be happy to share it with anyone upon request.
>
>    Maciej
>