[PATCH] pcnet32 compilation fix for 2.4.3pre6

[PATCH] pcnet32 compilation fix for 2.4.3pre6

[Andrzej Krzysztofowicz]

Hi,
  It looks like a not fully merged patch from Alan's tree:

drivers/net/net.o: In function `pcnet32_open':
drivers/net/net.o(.text+0x3bb9): undefined reference to `is_valid_ether_addr'
drivers/net/net.o: In function `pcnet32_probe1':
drivers/net/net.o(.text.init+0x5fa): undefined reference to `is_valid_ether_addr'


diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla/include/linux/etherdevice.h linux.ac/include/linux/etherdevice.h
--- linux.vanilla/include/linux/etherdevice.h	Fri Oct 27 20:22:34 2000
+++ linux.ac/include/linux/etherdevice.h	Fri Feb 16 11:10:22 2001
@@ -45,6 +45,14 @@
 	memcpy (dest->data, src, len);
 }
 
+/* Check that the ethernet address (MAC) is not 00:00:00:00:00:00 and is not
+ * a multicast address.  Return true if the address is valid.
+ */
+static __inline__ int is_valid_ether_addr( u8 *addr )
+{
+	return !(addr[0]&1) && memcmp( addr, "\0\0\0\0\0\0", 6);
+}
+
 #endif
 
 #endif	/* _LINUX_ETHERDEVICE_H */

-- 
=======================================================================
  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] pcnet32 compilation fix for 2.4.3pre6

[Jeff Garzik]

Andrzej Krzysztofowicz wrote:
> 
> Hi,
>   It looks like a not fully merged patch from Alan's tree:
> 
> drivers/net/net.o: In function `pcnet32_open':
> drivers/net/net.o(.text+0x3bb9): undefined reference to `is_valid_ether_addr'
> drivers/net/net.o: In function `pcnet32_probe1':
> drivers/net/net.o(.text.init+0x5fa): undefined reference to `is_valid_ether_addr'

Ouch, missed that.  Thanks for the patch.

-- 
Jeff Garzik       | May you have warm words on a cold evening,
Building 1024     | a full mooon on a dark night,
MandrakeSoft      | and a smooth road all the way to your door.

[PATCH] pcnet32 compilation fix for 2.4.3pre6

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

hmm, on second thought, I think I would prefer the attached patch
(compiled but not tested).

Hardware usually returns all 1's when it's not present, or not working,
so think checking for addresses filled with 1's is a good idea too.

I also took the patch from alan's tree and made the memcmp against
six-byte 'zaddr' rather than a seven-byte string :)

-- 
Jeff Garzik       | May you have warm words on a cold evening,
Building 1024     | a full mooon on a dark night,
MandrakeSoft      | and a smooth road all the way to your door.

[-- Attachment #2: etherdev.patch --]
[-- Type: text/plain, Size: 1037 bytes --]

Index: include/linux/etherdevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/etherdevice.h,v
retrieving revision 1.1.1.14.4.2
diff -u -r1.1.1.14.4.2 etherdevice.h
--- include/linux/etherdevice.h	2001/03/21 14:10:50	1.1.1.14.4.2
+++ include/linux/etherdevice.h	2001/03/22 13:37:15
@@ -46,6 +46,25 @@
 	memcpy (dest->data, src, len);
 }
 
+/**
+ * is_valid_ether_addr - Determine if the given Ethernet address is valid
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
+ * a multicast address, and is not FF:FF:FF:FF:FF:FF.
+ *
+ * Return true if the address is valid.
+ */
+static inline int is_valid_ether_addr( u8 *addr )
+{
+	const char zaddr[6] = {0,};
+	const char faddr[6] = {0xFF,0xFF,0xFF,0xFF,0xFF,0xFF};
+
+	return !(addr[0]&1) &&
+	       memcmp( addr, zaddr, 6) &&
+	       memcmp( addr, faddr, 6);
+}
+
 #endif
 
 #endif	/* _LINUX_ETHERDEVICE_H */

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Eli Carter]

Jeff Garzik wrote:
> 
> hmm, on second thought, I think I would prefer the attached patch
> (compiled but not tested).
> 
> Hardware usually returns all 1's when it's not present, or not working,
> so think checking for addresses filled with 1's is a good idea too.
> 
> I also took the patch from alan's tree and made the memcmp against
> six-byte 'zaddr' rather than a seven-byte string :)

Jeff,

The "!(addr[0]&1)" part of the test already catches the ff's case, so
that is redundant.
Using 6 bytes instead of 7 is an improvement.

Eli


> Index: include/linux/etherdevice.h
> ===================================================================
> RCS file: /cvsroot/gkernel/linux_2_4/include/linux/etherdevice.h,v
> retrieving revision 1.1.1.14.4.2
> diff -u -r1.1.1.14.4.2 etherdevice.h
> --- include/linux/etherdevice.h 2001/03/21 14:10:50     1.1.1.14.4.2
> +++ include/linux/etherdevice.h 2001/03/22 13:37:15
> @@ -46,6 +46,25 @@
>         memcpy (dest->data, src, len);
>  }
> 
> +/**
> + * is_valid_ether_addr - Determine if the given Ethernet address is valid
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
> + * a multicast address, and is not FF:FF:FF:FF:FF:FF.
> + *
> + * Return true if the address is valid.
> + */
> +static inline int is_valid_ether_addr( u8 *addr )
> +{
> +       const char zaddr[6] = {0,};
> +       const char faddr[6] = {0xFF,0xFF,0xFF,0xFF,0xFF,0xFF};
> +
> +       return !(addr[0]&1) &&
> +              memcmp( addr, zaddr, 6) &&
> +              memcmp( addr, faddr, 6);
> +}
> +
>  #endif
> 
>  #endif /* _LINUX_ETHERDEVICE_H */

-- 
-----------------------.           Rule of Accuracy: When working toward
Eli Carter             |            the solution of a problem, it always 
eli.carter(at)inet.com `------------------ helps if you know the answer.

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

Eli Carter wrote:
> The "!(addr[0]&1)" part of the test already catches the ff's case, so
> that is redundant.
> Using 6 bytes instead of 7 is an improvement.

oops.  Thanks, updated patch attached.  My patch also adds inline source
docs, and uses 'static inline' instead of 'static __inline__', two small
style improvements.
-- 
Jeff Garzik       | May you have warm words on a cold evening,
Building 1024     | a full mooon on a dark night,
MandrakeSoft      | and a smooth road all the way to your door.

[-- Attachment #2: etherdev.patch --]
[-- Type: text/plain, Size: 935 bytes --]

Index: include/linux/etherdevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/etherdevice.h,v
retrieving revision 1.1.1.14.4.2
diff -u -r1.1.1.14.4.2 etherdevice.h
--- include/linux/etherdevice.h	2001/03/21 14:10:50	1.1.1.14.4.2
+++ include/linux/etherdevice.h	2001/03/22 14:44:51
@@ -46,6 +46,22 @@
 	memcpy (dest->data, src, len);
 }
 
+/**
+ * is_valid_ether_addr - Determine if the given Ethernet address is valid
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
+ * a multicast address, and is not FF:FF:FF:FF:FF:FF.
+ *
+ * Return true if the address is valid.
+ */
+static inline int is_valid_ether_addr( u8 *addr )
+{
+	const char zaddr[6] = {0,};
+
+	return !(addr[0]&1) && memcmp( addr, zaddr, 6);
+}
+
 #endif
 
 #endif	/* _LINUX_ETHERDEVICE_H */

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Eli Carter]

Jeff Garzik wrote:
> 
> Eli Carter wrote:
> > The "!(addr[0]&1)" part of the test already catches the ff's case, so
> > that is redundant.
> > Using 6 bytes instead of 7 is an improvement.
> 
> oops.  Thanks, updated patch attached.  My patch also adds inline source
> docs, and uses 'static inline' instead of 'static __inline__', two small
> style improvements.

Mmmm.... documentation.  Yummy.  ;)

When I submitted the original patch, someone wanted to add the ff's
check as well... to reduce the number of people who make that
suggestion, perhaps the comment should read:

+ * Check that the Ethernet address (MAC) is not a multicast address or 
+ * FF:FF:FF:FF:FF:FF (by checking addr[0]&1) and is not
00:00:00:00:00:00.
+ *

Does that make it clear that both cases are covered by the one test?

Hmm... I used __inline__ because the other function in the same
headerfile used it...  What is the difference between the two, and is
one depricated now?  (And what about in 2.2.x?)

Eli

>   ------------------------------------------------------------------------
> Index: include/linux/etherdevice.h
> ===================================================================
> RCS file: /cvsroot/gkernel/linux_2_4/include/linux/etherdevice.h,v
> retrieving revision 1.1.1.14.4.2
> diff -u -r1.1.1.14.4.2 etherdevice.h
> --- include/linux/etherdevice.h 2001/03/21 14:10:50     1.1.1.14.4.2
> +++ include/linux/etherdevice.h 2001/03/22 14:44:51
> @@ -46,6 +46,22 @@
>         memcpy (dest->data, src, len);
>  }
> 
> +/**
> + * is_valid_ether_addr - Determine if the given Ethernet address is valid
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not
> + * a multicast address, and is not FF:FF:FF:FF:FF:FF.
> + *
> + * Return true if the address is valid.
> + */
> +static inline int is_valid_ether_addr( u8 *addr )
> +{
> +       const char zaddr[6] = {0,};
> +
> +       return !(addr[0]&1) && memcmp( addr, zaddr, 6);
> +}
> +
>  #endif
> 
>  #endif /* _LINUX_ETHERDEVICE_H */
-----------------------.           Rule of Accuracy: When working toward
Eli Carter             |            the solution of a problem, it always 
eli.carter(at)inet.com `------------------ helps if you know the answer.

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Jeff Garzik]

Eli Carter wrote:
> Mmmm.... documentation.  Yummy.  ;)
> 
> When I submitted the original patch, someone wanted to add the ff's
> check as well... to reduce the number of people who make that
> suggestion, perhaps the comment should read:
> 
> + * Check that the Ethernet address (MAC) is not a multicast address or
> + * FF:FF:FF:FF:FF:FF (by checking addr[0]&1) and is not
> 00:00:00:00:00:00.
> + *
> 
> Does that make it clear that both cases are covered by the one test?

yeah

> Hmm... I used __inline__ because the other function in the same
> headerfile used it...  What is the difference between the two, and is
> one depricated now?  (And what about in 2.2.x?)

since kernel requires gcc, which supports plaine ole 'inline', we don't
need to use the longer form.

-- 
Jeff Garzik       | May you have warm words on a cold evening,
Building 1024     | a full mooon on a dark night,
MandrakeSoft      | and a smooth road all the way to your door.

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[dank]

Eli Carter wrote:
> Hmm... I used __inline__ because the other function in the same
> headerfile used it...  What is the difference between the two, and is
> one depricated now?  (And what about in 2.2.x?)

the inline keyword was not added into the c language until the ansi/iso
c99 revision, echoing its use in c++.  prior to that time, gcc supplied
__inline__ as a vendor extension simulating this behavior which could be
compiled without violating checks for strict ansi conformance. 

with the new ansi standard, this use of __inline__ is no longer
necessary, although for gcc to grok it as legal ansi requires (iirc) the
macro _ISOC99_SOURCE_ must be defined.

-- 
nick black * head developer, trellis network security * www.trellisinc.com
"the tao gave birth to machine language.  machine language gave birth to the
 assembler.  the assembler gave rise to the compiler.  now there are ten
 thousand languages.  each has its place, but avoid cobol if you can." - ttop

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Ulrich Drepper]

dank@trellisinc.com writes:

> with the new ansi standard, this use of __inline__ is no longer
> necessary,

This is not correct.  Since the semantics of inline in C99 and gcc
differ all code which depends on the gcc semantics should continue to
use __inline__ since this keyword will hopefully forever signal the
gcc semantics.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Eli Carter]

Ulrich Drepper wrote:
> 
> dank@trellisinc.com writes:
> 
> > with the new ansi standard, this use of __inline__ is no longer
> > necessary,
> 
> This is not correct.  Since the semantics of inline in C99 and gcc
> differ all code which depends on the gcc semantics should continue to
> use __inline__ since this keyword will hopefully forever signal the
> gcc semantics.

So what are the differences?  (Or, what would I read to learn the
differences?)
When are they important to us?

TIA,

Eli
-----------------------.           Rule of Accuracy: When working toward
Eli Carter             |            the solution of a problem, it always 
eli.carter(at)inet.com `------------------ helps if you know the answer.

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Tom Leete]

Ulrich Drepper wrote:
> 
> dank@trellisinc.com writes:
> 
> > with the new ansi standard, this use of __inline__ is no longer
> > necessary,
> 
> This is not correct.  Since the semantics of inline in C99 and gcc
> differ all code which depends on the gcc semantics should continue to
> use __inline__ since this keyword will hopefully forever signal the
> gcc semantics.

Unfortunately, it seems that gcc will define __inline__ as a synonym for
inline, whatever inline is currently in use. I asked this on the gcc list a
while ago. The archive there should have the replies.

Regards,
Tom

-- 
The Daemons lurk and are dumb. -- Emerson

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Jeff Garzik]

On Thu, 29 Mar 2001, Tom Leete wrote:
> Ulrich Drepper wrote:
> > 
> > dank@trellisinc.com writes:
> > 
> > > with the new ansi standard, this use of __inline__ is no longer
> > > necessary,
> > 
> > This is not correct.  Since the semantics of inline in C99 and gcc
> > differ all code which depends on the gcc semantics should continue to
> > use __inline__ since this keyword will hopefully forever signal the
> > gcc semantics.

> Unfortunately, it seems that gcc will define __inline__ as a synonym for
> inline, whatever inline is currently in use. I asked this on the gcc list a
> while ago. The archive there should have the replies.

None of this matters :)

The kernel standard is 'static inline', so that is the preferred
usage in standard kernel code, without overriding reasons.

(also note that it is an outstanding cleanup item to replace
 'extern [__]inline[__]' with 'static inline', unless there are
 overriding reasons not to do so.)

	Jeff

Re: [PATCH] pcnet32 compilation fix for 2.4.3pre6

[Alan Cox]

> hmm, on second thought, I think I would prefer the attached patch
> (compiled but not tested).

Pointless...

> Hardware usually returns all 1's when it's not present, or not working,
> so think checking for addresses filled with 1's is a good idea too.

If the multicast bit is set then we already fail the address. Your test
does nothing.

Alan