[PATCH] net #9

[PATCH] net #9

[Andrzej Krzysztofowicz]

The following patch removes some zero initializers from statics

Andrzej

**************************** PATCH 9 *****************************
diff -uNr linux-2.4.5-ac4/drivers/net/dmfe.c linux/drivers/net/dmfe.c
--- linux-2.4.5-ac4/drivers/net/dmfe.c	Wed May 30 01:08:54 2001
+++ linux/drivers/net/dmfe.c	Wed May 30 01:16:04 2001
@@ -261,20 +261,20 @@
 static char version[] __devinitdata =
 	KERN_INFO "Davicom DM9xxx net driver, version " DMFE_VERSION "\n";
 
-static int dmfe_debug = 0;
+static int dmfe_debug;
 static unsigned char dmfe_media_mode = DMFE_AUTO;
-static u32 dmfe_cr6_user_set = 0;
+static u32 dmfe_cr6_user_set;
 
 /* For module input parameter */
-static int debug = 0;
-static u32 cr6set = 0;
+static int debug;
+static u32 cr6set;
 static unsigned char mode = 8;
 static u8 chkmode = 1;
-static u8 HPNA_mode = 0;	/* Default: Low Power/High Speed */
-static u8 HPNA_rx_cmd = 0;	/* Default: Disable Rx remote command */
-static u8 HPNA_tx_cmd = 0;	/* Default: Don't issue remote command */
-static u8 HPNA_NoiseFloor = 0;	/* Default: HPNA NoiseFloor */
-static u8 SF_mode = 0;		/* Special Function: 1:VLAN, 2:RX Flow Control
+static u8 HPNA_mode;		/* Default: Low Power/High Speed */
+static u8 HPNA_rx_cmd;		/* Default: Disable Rx remote command */
+static u8 HPNA_tx_cmd;		/* Default: Don't issue remote command */
+static u8 HPNA_NoiseFloor;	/* Default: HPNA NoiseFloor */
+static u8 SF_mode;		/* Special Function: 1:VLAN, 2:RX Flow Control
 				   4: TX pause packet */
 
 unsigned long CrcTable[256] = {
diff -uNr linux-2.4.5-ac4/drivers/net/wavelan.p.h linux/drivers/net/wavelan.p.h
--- linux-2.4.5-ac4/drivers/net/wavelan.p.h	Wed May 30 01:08:56 2001
+++ linux/drivers/net/wavelan.p.h	Wed May 30 01:16:05 2001
@@ -699,9 +699,9 @@
 
 #ifdef	MODULE
 /* Parameters set by insmod */
-static int	io[4]	= { 0, 0, 0, 0 };
-static int	irq[4]	= { 0, 0, 0, 0 };
-static char	name[4][IFNAMSIZ] = { "", "", "", "" };
+static int	io[4];
+static int	irq[4];
+static char	name[4][IFNAMSIZ];
 MODULE_PARM(io, "1-4i");
 MODULE_PARM(irq, "1-4i");
 MODULE_PARM(name, "1-4c" __MODULE_STRING(IFNAMSIZ));


-- 
=======================================================================
  Andrzej M. Krzysztofowicz               ankry@mif.pg.gda.pl
  phone (48)(58) 347 14 61
Faculty of Applied Phys. & Math.,   Technical University of Gdansk

Re: [PATCH] net #9

[Jean Tourrilhes]

On Wed, May 30, 2001 at 02:48:24AM +0200, Andrzej Krzysztofowicz wrote:
> 
> The following patch removes some zero initializers from statics
> 
> Andrzej

	If I were you, I would fix gcc rather than making my code
unreadable.

	I write source code in C rather than coding ASM in hex because
of the semantic attached to what I write, which make the code readable
by me and by other, and make my code portable to other environments
(for example user space). Initialisating a variable to zero as opposed
to leaving it undefined has plenty of semantic attached to it.
	It's the job of the compiler to make sure that all this kind
of stupid optimisation are done and the code produced is as efficient
as possible and adapted to the exact characteristics of the operating
envirtonment. Especially that it's probably 10 lines to add the proper
option to gcc command line.

	Therefore, Alan, please do not apply those kind of patches to
my drivers.

	Thanks...

	Jean

Re: [PATCH] net #9

[Jeff Garzik]

Jean Tourrilhes wrote:
> 
> On Wed, May 30, 2001 at 02:48:24AM +0200, Andrzej Krzysztofowicz wrote:
> >
> > The following patch removes some zero initializers from statics
> >
> > Andrzej
> 
>         If I were you, I would fix gcc rather than making my code
> unreadable.
[...]
>         Therefore, Alan, please do not apply those kind of patches to
> my drivers.

This is standard kernel cleanup that makes the resulting image smaller. 
These patches have been going into all areas of the kernel for quite
some time.

-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |

Re: [PATCH] net #9

[Jean Tourrilhes]

On Tue, May 29, 2001 at 09:13:34PM -0400, Jeff Garzik wrote:
> 
> This is standard kernel cleanup that makes the resulting image smaller. 
> These patches have been going into all areas of the kernel for quite
> some time.

	This doesn't make it right.

	Ok, while we are on the topic : could somebody explain me why
we can't get gcc to do that for us ? What is preventing adding a gcc
command line flag to do exactly that ? It's not like rocket science
(simple test) and would avoid to have tediously to go through all
source code, past, present and *future* to make those changes.
	Bah, maybe it's too straightforward...

	Jean

Re: [PATCH] net #9

[Jeff Garzik]

Jean Tourrilhes wrote:
> 
> On Tue, May 29, 2001 at 09:13:34PM -0400, Jeff Garzik wrote:
> >
> > This is standard kernel cleanup that makes the resulting image smaller.
> > These patches have been going into all areas of the kernel for quite
> > some time.
> 
>         This doesn't make it right.
> 
>         Ok, while we are on the topic : could somebody explain me why
> we can't get gcc to do that for us ? What is preventing adding a gcc
> command line flag to do exactly that ? It's not like rocket science
> (simple test) and would avoid to have tediously to go through all
> source code, past, present and *future* to make those changes.
>         Bah, maybe it's too straightforward...

This is ANSI C standard stuff.  If a static object with a scalar type is
not explicitly initialized, it is initialized to zero by default.

Sure we can get gcc to recognize that case, but why use gcc to work
around code that avoids an ANSI feature?

-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |

Re: [PATCH] net #9

[Jean Tourrilhes]

On Tue, May 29, 2001 at 09:47:19PM -0400, Jeff Garzik wrote:
> Jean Tourrilhes wrote:
> > 
> > On Tue, May 29, 2001 at 09:13:34PM -0400, Jeff Garzik wrote:
> > >
> > > This is standard kernel cleanup that makes the resulting image smaller.
> > > These patches have been going into all areas of the kernel for quite
> > > some time.
> > 
> >         This doesn't make it right.
> > 
> >         Ok, while we are on the topic : could somebody explain me why
> > we can't get gcc to do that for us ? What is preventing adding a gcc
> > command line flag to do exactly that ? It's not like rocket science
> > (simple test) and would avoid to have tediously to go through all
> > source code, past, present and *future* to make those changes.
> >         Bah, maybe it's too straightforward...
> 
> This is ANSI C standard stuff.  If a static object with a scalar type is
> not explicitly initialized, it is initialized to zero by default.
> 
> Sure we can get gcc to recognize that case, but why use gcc to work
> around code that avoids an ANSI feature?

	Good standard don't mandate the implementation. And as
somebody doing some other language said, there is more than one way to
do it.
	ANSI C doesn't prohibit to initialise variable to zero
(AFAIK). ANSI C doesn't prevent the compiler to optimise for this
case. If I initialise the varaible 3 time to the same value in a row,
gcc will remove the other two.

	Let's put it another way... What's different between :
		for(i = 0; i < n; i++) xxx;
	And :
		i = 0; while(i++ < n) xxx;
	I would expect the compiler to produce code as efficient in
both cases, and leave the choice of coding style to the user.

	Jean

Re: [PATCH] net #9

[Jeff Garzik]

*shrug*  Well, if you want to go against the kernel standard that's fine
with me.  I won't put Andrzej's changes to your drivers upstream.  You
are going to continually see patches to clean that up, though, because
it makes the end user's kernel smaller.  Please consider noting this
special case in a comment in each of your drivers "do not clean up
static initializers" or similar.

It's really a pain in the ass to remember special cases like this, so
please reconsider.  Being not-like-the-others is detrimental to the long
term maintainability of the overall kernel.

Regards,

	Jeff


-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |

Re: [PATCH] net #9

[Jean Tourrilhes]

On Tue, May 29, 2001 at 10:13:02PM -0400, Jeff Garzik wrote:
> *shrug*  Well, if you want to go against the kernel standard that's fine
> with me.  I won't put Andrzej's changes to your drivers upstream.  You
> are going to continually see patches to clean that up, though, because
> it makes the end user's kernel smaller.  Please consider noting this
> special case in a comment in each of your drivers "do not clean up
> static initializers" or similar.
> 
> It's really a pain in the ass to remember special cases like this, so
> please reconsider.  Being not-like-the-others is detrimental to the long
> term maintainability of the overall kernel.
> 
> Regards,
> 
> 	Jeff

	I agree with you on the special case. I don't like it
either. Anyway, most patch to my drivers are applied wether I like it
or not, so I guess that I should be happy that I was notified and I
should sut up my big mouth because it won't make a difference.
	If I reject the patch now, I will be applied behind my
back. Been there, done that.
	In other words : yes, please apply the patch.

	Jean

Re: [PATCH] net #9

[Hermann Himmelbauer]

Jean Tourrilhes wrote:
> > This is ANSI C standard stuff.  If a static object with a scalar type is
> > not explicitly initialized, it is initialized to zero by default.
> >
> > Sure we can get gcc to recognize that case, but why use gcc to work
> > around code that avoids an ANSI feature?
> 
>         Good standard don't mandate the implementation. And as
> somebody doing some other language said, there is more than one way to
> do it.

Hmmm, I understand both sides perfectly, but what about that one:

int n; /* n=0 */

Would that be a compromise?

		Regards,
		Hermann


-- 
 ,_,
(O,O)     "There is more to life than increasing its speed."
(   )     -- Gandhi
-"-"--------------------------------------------------------------

Re: [PATCH] net #9

[Jean Tourrilhes]

On Tue, May 29, 2001 at 09:47:19PM -0400, Jeff Garzik wrote:
> 
> This is ANSI C standard stuff.  If a static object with a scalar type is
> not explicitly initialized, it is initialized to zero by default.
> 
> Sure we can get gcc to recognize that case, but why use gcc to work
> around code that avoids an ANSI feature?

	Apart from this stupid flame that I'm making, I've got my
Intel/Symbol card to work properly with the Orinoco driver. This mean
that we are not far away to have the 4 main flavor of 802.11b working
in 2.4.X (i.e. Lucent/Symbol/PrismII/Aironet).
	See :
http://www.hpl.hp.com/personal/Jean_Tourrilhes/Linux/Orinoco.html#patches

	Just to make sure we end on a positive note ;-) Now, if I
could get the card of Alan to work...

	Have fun, don't take it seriously...

	Jean

Re: [PATCH] net #9

[Jeff Garzik]

Jean Tourrilhes wrote:
>         Apart from this stupid flame that I'm making, I've got my
> Intel/Symbol card to work properly with the Orinoco driver. This mean
> that we are not far away to have the 4 main flavor of 802.11b working
> in 2.4.X (i.e. Lucent/Symbol/PrismII/Aironet).
>         See :
> http://www.hpl.hp.com/personal/Jean_Tourrilhes/Linux/Orinoco.html#patches
> 
>         Just to make sure we end on a positive note ;-) Now, if I
> could get the card of Alan to work...

Cool!  Though I do have a comment on this too.  :)

Are you sure priv->hw_ready is not racy?  You should probably use an
atomic operation like test_bit and set_bit.

-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |

Re: [PATCH] net #9

[Henning P. Schmiedehausen]

Jeff Garzik <jgarzik@mandrakesoft.com> writes:

>This is ANSI C standard stuff.  If a static object with a scalar type is
>not explicitly initialized, it is initialized to zero by default.

"The kernel is not in ANSI C". Who wrote this again? 

	Regards
		Henning

-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [PATCH] net #9

[Pavel Machek]

Hi!

> > This is standard kernel cleanup that makes the resulting image smaller. 
> > These patches have been going into all areas of the kernel for quite
> > some time.
> 
> 	This doesn't make it right.

Well... It does. Code should be uniform. You may like

if (
  x == y
) {
  printf(
	"ahoj"
	);
}

style of indentation, but it is not okay in kernel.

> 	Ok, while we are on the topic : could somebody explain me why
> we can't get gcc to do that for us ? What is preventing adding a gcc

int xyzzy;

goes to BSS, while

int xyzzy = 0 

goes to DATA. That is important for some folks.

> command line flag to do exactly that ? It's not like rocket science
> (simple test) and would avoid to have tediously to go through all

It is actually not *so* easy to do it in gcc; there's patch, however.

								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org

Re: [PATCH] net #9

[Henning P. Schmiedehausen]

Andrzej Krzysztofowicz <ankry@green.mif.pg.gda.pl> writes:

>-static char	name[4][IFNAMSIZ] = { "", "", "", "" };

>+static char	name[4][IFNAMSIZ];

Ugh. Sure about that one? the variables have been pointers to zero,
now they're zero...

	Regards
		Henning

-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [PATCH] net #9

[Andrzej Krzysztofowicz]

"Henning P. Schmiedehausen wrote:"
> Andrzej Krzysztofowicz <ankry@green.mif.pg.gda.pl> writes:
> 
> >-static char	name[4][IFNAMSIZ] = { "", "", "", "" };
> 
> >+static char	name[4][IFNAMSIZ];
> 
> Ugh. Sure about that one? the variables have been pointers to zero,
> now they're zero...

I do not agree. As I understand C "name" is a table, i.e. a pointer to a
prealocated area of size of 4*IFNAMSIZ*sizeof(char) bytes. There is no
pointers to the strings stored seprately. 
[ The strings are copied into the apropriate locations in the array during
  initialization ]

After applying the mentioned patch just the whole area of the array will be
zeroed. Not only the first characters of name[i] i=0,1,2,3 ...

Andrzej
-- 
=======================================================================
  Andrzej M. Krzysztofowicz               ankry@mif.pg.gda.pl
  phone (48)(58) 347 14 61
Faculty of Applied Phys. & Math.,   Technical University of Gdansk

Re: [PATCH] net #9

[Keith Owens]

On Wed, 30 May 2001 09:32:39 +0000 (UTC), 
"Henning P. Schmiedehausen" <mailgate@hometree.net> wrote:
>Andrzej Krzysztofowicz <ankry@green.mif.pg.gda.pl> writes:
>
>>-static char	name[4][IFNAMSIZ] = { "", "", "", "" };
>
>>+static char	name[4][IFNAMSIZ];
>
>Ugh. Sure about that one? the variables have been pointers to zero,
>now they're zero...

Bzzt.  Arrays and pointers are not always equivalent.  This code
defines an area of 4*IFNAMSIZ chars.  The old code then initialised
each IFNAMSIZ chars to the empty string (an array starting with '\0'),
not a pointer to an empty string.  Test file x.c.

#define IFNAMSIZ 7
static char name[4][IFNAMSIZ];
int main(void) { return(0); }

# gcc -g x.c -o x
# gdb x
(gdb) ptype name
type = char [4][7]
(gdb) x name[0]
0x80494f4 <name>:       0x00000000
(gdb) x name[1]
0x80494fb <name+7>:     0x00000000
(gdb) x name[2]
0x8049502 <name+14>:    0x00000000
(gdb) x name[3]
0x8049509 <name+21>:    0x00000000

Zero initialisation via bss works fine.  Try it with 
  static char name[4][IFNAMSIZ] = {"", "", "", ""};
and you get exactly the same results, at the expense of more disk space
and time to load.  Not much extra I know, but it all adds up.

Re: [PATCH] net #9

[Henning P. Schmiedehausen]

Keith Owens <kaos@ocs.com.au> writes:

>On Wed, 30 May 2001 09:32:39 +0000 (UTC), 
>"Henning P. Schmiedehausen" <mailgate@hometree.net> wrote:
>>Andrzej Krzysztofowicz <ankry@green.mif.pg.gda.pl> writes:
>>
>>>-static char	name[4][IFNAMSIZ] = { "", "", "", "" };
>>
>>>+static char	name[4][IFNAMSIZ];
>>
>>Ugh. Sure about that one? the variables have been pointers to zero,
>>now they're zero...

>Bzzt.  Arrays and pointers are not always equivalent.  This code

yeah, you're right. I got fooled by the fact that these are already
arrays, not initialized pointers.

There _is_ a point for Java in this. ;-) 

	Regards
		Henning
-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [PATCH] net #9

[Jeff Garzik]

"Henning P. Schmiedehausen" wrote:
> 
> Andrzej Krzysztofowicz <ankry@green.mif.pg.gda.pl> writes:
> 
> >-static char   name[4][IFNAMSIZ] = { "", "", "", "" };
> 
> >+static char   name[4][IFNAMSIZ];
> 
> Ugh. Sure about that one? the variables have been pointers to zero,
> now they're zero...

No, the variables were and always have been arrays, not pointers.

The previous incarnation, {"","","",""}, was actually doubly lame,
because not only was it an unnecessary zeroing of the var, but using ""
causes an extra string to be generated in the output asm.

-- 
Jeff Garzik      | Disbelief, that's why you fail.
Building 1024    |
MandrakeSoft     |