[PATCH] Small cleanup to kernel/setup.c

[PATCH] Small cleanup to kernel/setup.c

[Art Haas]

Hi.

The following patch replaces unnecessary duplicated testing with a set
of if/else tests, and removes some unused variables in the same routine.
Maybe the 'highest_paddr' value was used in earlier versions of the
file, but it isn't used anywhere now. I've booted a 2.6.6-rc kernel with
this patch applied.

Art Haas

=== arch/sparc/kernel/setup.c 1.28 vs edited ==--- 1.28/arch/sparc/kernel/setup.c	Sun Mar 21 16:43:34 2004
+++ edited/arch/sparc/kernel/setup.c	Mon May  3 14:21:51 2004
@@ -257,7 +257,6 @@
 void __init setup_arch(char **cmdline_p)
 {
 	int i;
-	unsigned long highest_paddr;
 
 	sparc_ttable = (struct tt_entry *) &start;
 
@@ -266,14 +265,23 @@
 	strcpy(saved_command_line, *cmdline_p);
 
 	/* Set sparc_cpu_model */
-	sparc_cpu_model = sun_unknown;
-	if(!strcmp(&cputypval,"sun4 ")) { sparc_cpu_model=sun4; }
-	if(!strcmp(&cputypval,"sun4c")) { sparc_cpu_model=sun4c; }
-	if(!strcmp(&cputypval,"sun4m")) { sparc_cpu_model=sun4m; }
-	if(!strcmp(&cputypval,"sun4s")) { sparc_cpu_model=sun4m; }  /* CP-1200 with PROM 2.30 -E */
-	if(!strcmp(&cputypval,"sun4d")) { sparc_cpu_model=sun4d; }
-	if(!strcmp(&cputypval,"sun4e")) { sparc_cpu_model=sun4e; }
-	if(!strcmp(&cputypval,"sun4u")) { sparc_cpu_model=sun4u; }
+	if (!strcmp(&cputypval,"sun4 ")) {
+		sparc_cpu_model=sun4;
+	} else if (!strcmp(&cputypval,"sun4c")) {
+		sparc_cpu_model=sun4c;
+	} else if (!strcmp(&cputypval,"sun4m")) {
+		sparc_cpu_model=sun4m;
+	} else if (!strcmp(&cputypval,"sun4s")) {
+		sparc_cpu_model=sun4m;	 /* CP-1200 with PROM 2.30 -E */
+	} else if (!strcmp(&cputypval,"sun4d")) {
+		sparc_cpu_model=sun4d;
+	} else if (!strcmp(&cputypval,"sun4e")) {
+		sparc_cpu_model=sun4e;
+	} else if (!strcmp(&cputypval,"sun4u")) {
+		sparc_cpu_model=sun4u;
+	} else {
+		sparc_cpu_model = sun_unknown;
+	}
 
 #ifdef CONFIG_SUN4
 	if (sparc_cpu_model != sun4) {
@@ -320,16 +328,9 @@
 	(void) prom_probe_memory();
 
 	phys_base = 0xffffffffUL;
-	highest_paddr = 0UL;
 	for (i = 0; sp_banks[i].num_bytes != 0; i++) {
-		unsigned long top;
-
 		if (sp_banks[i].base_addr < phys_base)
 			phys_base = sp_banks[i].base_addr;
-		top = sp_banks[i].base_addr +
-			sp_banks[i].num_bytes;
-		if (highest_paddr < top)
-			highest_paddr = top;
 	}
 	pfn_base = phys_base >> PAGE_SHIFT;
 
-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: [PATCH] Small cleanup to kernel/setup.c

[Ben Collins]

Maybe it's just me, but the original version seems more readable, even
if it isn't proper formatting.

> -	sparc_cpu_model = sun_unknown;
> -	if(!strcmp(&cputypval,"sun4 ")) { sparc_cpu_model=sun4; }
> -	if(!strcmp(&cputypval,"sun4c")) { sparc_cpu_model=sun4c; }
> -	if(!strcmp(&cputypval,"sun4m")) { sparc_cpu_model=sun4m; }
> -	if(!strcmp(&cputypval,"sun4s")) { sparc_cpu_model=sun4m; }  /* CP-1200 with PROM 2.30 -E */
> -	if(!strcmp(&cputypval,"sun4d")) { sparc_cpu_model=sun4d; }
> -	if(!strcmp(&cputypval,"sun4e")) { sparc_cpu_model=sun4e; }
> -	if(!strcmp(&cputypval,"sun4u")) { sparc_cpu_model=sun4u; }
> +	if (!strcmp(&cputypval,"sun4 ")) {
> +		sparc_cpu_model=sun4;
> +	} else if (!strcmp(&cputypval,"sun4c")) {
> +		sparc_cpu_model=sun4c;
> +	} else if (!strcmp(&cputypval,"sun4m")) {
> +		sparc_cpu_model=sun4m;
> +	} else if (!strcmp(&cputypval,"sun4s")) {
> +		sparc_cpu_model=sun4m;	 /* CP-1200 with PROM 2.30 -E */
> +	} else if (!strcmp(&cputypval,"sun4d")) {
> +		sparc_cpu_model=sun4d;
> +	} else if (!strcmp(&cputypval,"sun4e")) {
> +		sparc_cpu_model=sun4e;
> +	} else if (!strcmp(&cputypval,"sun4u")) {
> +		sparc_cpu_model=sun4u;
> +	} else {
> +		sparc_cpu_model = sun_unknown;
> +	}

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Re: [PATCH] Small cleanup to kernel/setup.c

[Art Haas]

On Tue, May 04, 2004 at 10:04:58AM -0400, Ben Collins wrote:
> Maybe it's just me, but the original version seems more readable, even
> if it isn't proper formatting.
> 
> > -	sparc_cpu_model = sun_unknown;
> > -	if(!strcmp(&cputypval,"sun4 ")) { sparc_cpu_model=sun4; }
> > -	if(!strcmp(&cputypval,"sun4c")) { sparc_cpu_model=sun4c; }
> > -	if(!strcmp(&cputypval,"sun4m")) { sparc_cpu_model=sun4m; }
> > -	if(!strcmp(&cputypval,"sun4s")) { sparc_cpu_model=sun4m; }  /* CP-1200 with PROM 2.30 -E */
> > -	if(!strcmp(&cputypval,"sun4d")) { sparc_cpu_model=sun4d; }
> > -	if(!strcmp(&cputypval,"sun4e")) { sparc_cpu_model=sun4e; }
> > -	if(!strcmp(&cputypval,"sun4u")) { sparc_cpu_model=sun4u; }
> > +	if (!strcmp(&cputypval,"sun4 ")) {
> > +		sparc_cpu_model=sun4;
> > +	} else if (!strcmp(&cputypval,"sun4c")) {
> > +		sparc_cpu_model=sun4c;
> > +	} else if (!strcmp(&cputypval,"sun4m")) {
> > +		sparc_cpu_model=sun4m;
> > +	} else if (!strcmp(&cputypval,"sun4s")) {
> > +		sparc_cpu_model=sun4m;	 /* CP-1200 with PROM 2.30 -E */
> > +	} else if (!strcmp(&cputypval,"sun4d")) {
> > +		sparc_cpu_model=sun4d;
> > +	} else if (!strcmp(&cputypval,"sun4e")) {
> > +		sparc_cpu_model=sun4e;
> > +	} else if (!strcmp(&cputypval,"sun4u")) {
> > +		sparc_cpu_model=sun4u;
> > +	} else {
> > +		sparc_cpu_model = sun_unknown;
> > +	}

I don't like the original because it keeps doing strcmp() tests more
times than necessary. That was my motivation in doing changing this bit
of code.

Art Haas

-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: [PATCH] Small cleanup to kernel/setup.c

[Ben Collins]

> I don't like the original because it keeps doing strcmp() tests more
> times than necessary. That was my motivation in doing changing this bit
> of code.

Only happens once at boot. Can't be that much overhead. Could just add
else's in front of the later if's, and leave the formatting the same.

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Re: [PATCH] Small cleanup to kernel/setup.c

[Keith M Wesolowski]

On Tue, May 04, 2004 at 10:26:55AM -0400, Ben Collins wrote:

> > I don't like the original because it keeps doing strcmp() tests more
> > times than necessary. That was my motivation in doing changing this bit
> > of code.
> 
> Only happens once at boot. Can't be that much overhead. Could just add
> else's in front of the later if's, and leave the formatting the same.

Right, this is completely non-critical, and I also like the existing
formatting.  I see no good reason to change this.

-- 
Keith M Wesolowski

Re: [PATCH] Small cleanup to kernel/setup.c

[Art Haas]

On Tue, May 04, 2004 at 07:57:14AM -0700, Keith M Wesolowski wrote:
> On Tue, May 04, 2004 at 10:26:55AM -0400, Ben Collins wrote:
> 
> > > I don't like the original because it keeps doing strcmp() tests more
> > > times than necessary. That was my motivation in doing changing this bit
> > > of code.
> > 
> > Only happens once at boot. Can't be that much overhead. Could just add
> > else's in front of the later if's, and leave the formatting the same.
> 
> Right, this is completely non-critical, and I also like the existing
> formatting.  I see no good reason to change this.
> 

So the formatting changes are out - what about the variable removal?

Art Haas
-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: [PATCH] Small cleanup to kernel/setup.c

[Keith M Wesolowski]

On Tue, May 04, 2004 at 10:02:22AM -0500, Art Haas wrote:

> So the formatting changes are out - what about the variable removal?

I'll take that part - highest_paddr is completely useless.  Thanks.

-- 
Keith M Wesolowski

Re: [PATCH] Small cleanup to kernel/setup.c

[Thomas Nemeth]

Le 04.05.04, Art Haas a tapoté :

| On Tue, May 04, 2004 at 10:04:58AM -0400, Ben Collins wrote:
| > Maybe it's just me, but the original version seems more readable, even
| > if it isn't proper formatting.
| >
| > > -	sparc_cpu_model = sun_unknown;
| > > -	if(!strcmp(&cputypval,"sun4 ")) { sparc_cpu_model=sun4; }
| > > -	if(!strcmp(&cputypval,"sun4c")) { sparc_cpu_model=sun4c; }
| > > -	if(!strcmp(&cputypval,"sun4m")) { sparc_cpu_model=sun4m; }
| > > -	if(!strcmp(&cputypval,"sun4s")) { sparc_cpu_model=sun4m; }  /* CP-1200 with PROM 2.30 -E */
| > > -	if(!strcmp(&cputypval,"sun4d")) { sparc_cpu_model=sun4d; }
| > > -	if(!strcmp(&cputypval,"sun4e")) { sparc_cpu_model=sun4e; }
| > > -	if(!strcmp(&cputypval,"sun4u")) { sparc_cpu_model=sun4u; }
| > > +	if (!strcmp(&cputypval,"sun4 ")) {
| > > +		sparc_cpu_model=sun4;
| > > +	} else if (!strcmp(&cputypval,"sun4c")) {
| > > +		sparc_cpu_model=sun4c;
| > > +	} else if (!strcmp(&cputypval,"sun4m")) {
| > > +		sparc_cpu_model=sun4m;
| > > +	} else if (!strcmp(&cputypval,"sun4s")) {
| > > +		sparc_cpu_model=sun4m;	 /* CP-1200 with PROM 2.30 -E */
| > > +	} else if (!strcmp(&cputypval,"sun4d")) {
| > > +		sparc_cpu_model=sun4d;
| > > +	} else if (!strcmp(&cputypval,"sun4e")) {
| > > +		sparc_cpu_model=sun4e;
| > > +	} else if (!strcmp(&cputypval,"sun4u")) {
| > > +		sparc_cpu_model=sun4u;
| > > +	} else {
| > > +		sparc_cpu_model = sun_unknown;
| > > +	}
|
| I don't like the original because it keeps doing strcmp() tests more
| times than necessary. That was my motivation in doing changing this bit
| of code.

	Hi.

	Why don't you just remove all strcmp's ?
	Something like
if (strlen(&cputypval)=4)
	sparc_cpu_model=sun4;
else if (&cputypval[4]='c')
	sparc_cpu_model=sun4c;
else if (&cputypval[4]='d')
	sparc_cpu_model=sun4d;
...

	Could be better :) But of course, if Sun changes its naming
	scheme, it won't work anymore...


Thomas
-- 
printk("VFS: Busy inodes after unmount. "
        "Self-destruct in 5 seconds.  Have a nice day...\n");
        2.3.99-pre8 /usr/src/linux/fs/super.c

Re: [PATCH] Small cleanup to kernel/setup.c

[Keith M Wesolowski]

On Tue, May 04, 2004 at 06:11:44PM +0200, Thomas Nemeth wrote:

> 	Why don't you just remove all strcmp's ?
> 	Something like
> if (strlen(&cputypval)=4)
> 	sparc_cpu_model=sun4;

It's not "sun4" but rather "sun4 " so this is wrong.

> else if (&cputypval[4]='c')
> 	sparc_cpu_model=sun4c;
> else if (&cputypval[4]='d')
> 	sparc_cpu_model=sun4d;

See also head.S.

Any patch that modifies this stuff needs to make a significant
improvement; for example, setting these values early in head.S and
getting rid of this altogether.

But really it is not worth spending time on.

-- 
Keith M Wesolowski

Re: [PATCH] Small cleanup to kernel/setup.c

[Art Haas]

On Tue, May 04, 2004 at 06:11:44PM +0200, Thomas Nemeth wrote:
> Le 04.05.04, Art Haas a tapot? :
> 
> [ ... snip ... ]
> | I don't like the original because it keeps doing strcmp() tests more
> | times than necessary. That was my motivation in doing changing this bit
> | of code.
> 
> 	Hi.
> 
> 	Why don't you just remove all strcmp's ?
> 	Something like
> if (strlen(&cputypval)=4)
> 	sparc_cpu_model=sun4;
> else if (&cputypval[4]='c')
> 	sparc_cpu_model=sun4c;
> else if (&cputypval[4]='d')
> 	sparc_cpu_model=sun4d;
> ...
> 
> 	Could be better :) But of course, if Sun changes its naming
> 	scheme, it won't work anymore...

The strlen() above won't work because of a trailing space in 'sun4 '
name. Regardless, the maintainers don't think the change is worthwhile,
so I'm not going to pester them about this anymore. This bit of code is
only used once, and for the few extra strcmp() calls it isn't worth
making a fuss. There are other places in the sparc code that developer
time and efforts can be spent, such as getting SMP stuff to work in 2.6,
and once that hurdle is crossed then little micro-optimizations like I'd
proposed in the patch might be worth re-examining.

It would be nice if there were as many people examining the Sparc code
as there are the i386 or PPC code, but that isn't the case.

Art Haas

-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: [PATCH] Small cleanup to kernel/setup.c

[Keith M Wesolowski]

On Tue, May 04, 2004 at 11:42:04AM -0500, Art Haas wrote:

> making a fuss. There are other places in the sparc code that developer
> time and efforts can be spent, such as getting SMP stuff to work in 2.6,

Exactly.  I think I need to update and re-post Pete's status list so
that interested people will know what needs doing.  For now that old
post is not a bad starting point; only about half of the items have
been completed.

-- 
Keith M Wesolowski