multicast hash incorrect on big endian archs

multicast hash incorrect on big endian archs

[Manfred Spraul]

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

I noticed that the multicast hash calculations assumed little endian
byte ordering in the winbond-840 driver, and it seems that several other
drivers are also affected:

8139too, epic100, fealnx, pci-skeleton, sis900, starfile, sundance,
via-rhine, yellowfin
perhaps drivers/net/pcmcia/xircom_tulip_cb

I've attached an untested patch.
It's possible that the nic performs another byte swap if configured for
big endian support, but I've never seen a nic that performs byte swaps
on register writes, only on memory reads.

Please cc me, I'm not subscribed to the mailing lists.
--
	Manfred

[-- Attachment #2: patch-ask --]
[-- Type: text/plain, Size: 8070 bytes --]

diff -u 2.4/drivers/net/8139too.c build-2.4/drivers/net/8139too.c
--- 2.4/drivers/net/8139too.c	Sat Jun  2 14:19:44 2001
+++ build-2.4/drivers/net/8139too.c	Sun Jun  3 19:46:05 2001
@@ -2248,6 +2248,10 @@
 	return crc;
 }
 
+static void set_bit_32(int offset, u32 * data)
+{
+	data[offset >> 5] |= (1 << (offset & 0x1f));
+}
 
 static void rtl8139_set_rx_mode (struct net_device *dev)
 {
@@ -2283,7 +2287,7 @@
 		mc_filter[1] = mc_filter[0] = 0;
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 		     i++, mclist = mclist->next)
-			set_bit (ether_crc (ETH_ALEN, mclist->dmi_addr) >> 26,
+			set_bit_32(ether_crc (ETH_ALEN, mclist->dmi_addr) >> 26,
 				 mc_filter);
 	}
 
diff -u 2.4/drivers/net/epic100.c build-2.4/drivers/net/epic100.c
--- 2.4/drivers/net/epic100.c	Sat May 26 10:06:26 2001
+++ build-2.4/drivers/net/epic100.c	Sun Jun  3 19:48:44 2001
@@ -1305,11 +1305,16 @@
 	return crc;
 }
 
+static void set_bit_16(int offset, u16 *data)
+{
+	data[offset >> 4] |= (1<<(offset & 0xf));
+}
+
 static void set_rx_mode(struct net_device *dev)
 {
 	long ioaddr = dev->base_addr;
 	struct epic_private *ep = dev->priv;
-	unsigned char mc_filter[8];		 /* Multicast hash filter */
+	u16 mc_filter[8];		 /* Multicast hash filter */
 	int i;
 
 	if (dev->flags & IFF_PROMISC) {			/* Set promiscuous. */
@@ -1332,13 +1337,13 @@
 		memset(mc_filter, 0, sizeof(mc_filter));
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 			 i++, mclist = mclist->next)
-			set_bit(ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x3f,
+			set_bit_16(ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x3f,
 					mc_filter);
 	}
 	/* ToDo: perhaps we need to stop the Tx and Rx process here? */
 	if (memcmp(mc_filter, ep->mc_filter, sizeof(mc_filter))) {
 		for (i = 0; i < 4; i++)
-			outw(((u16 *)mc_filter)[i], ioaddr + MC0 + i*4);
+			outw(mc_filter[i], ioaddr + MC0 + i*4);
 		memcpy(ep->mc_filter, mc_filter, sizeof(mc_filter));
 	}
 	return;
diff -u 2.4/drivers/net/fealnx.c build-2.4/drivers/net/fealnx.c
--- 2.4/drivers/net/fealnx.c	Sat May 26 10:06:26 2001
+++ build-2.4/drivers/net/fealnx.c	Sun Jun  3 19:49:45 2001
@@ -1642,6 +1642,10 @@
 	return crc;
 }
 
+static void set_bit_32(int offset, u32 * data)
+{
+	data[offset >> 5] |= (1 << (offset & 0x1f));
+}
 
 static void set_rx_mode(struct net_device *dev)
 {
@@ -1667,7 +1671,7 @@
 		memset(mc_filter, 0, sizeof(mc_filter));
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 		     i++, mclist = mclist->next) {
-			set_bit((ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26) ^ 0x3F,
+			set_bit_32((ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26) ^ 0x3F,
 				mc_filter);
 		}
 		rx_mode = AB | AM;
diff -u 2.4/drivers/net/pci-skeleton.c build-2.4/drivers/net/pci-skeleton.c
--- 2.4/drivers/net/pci-skeleton.c	Fri Apr 20 20:54:23 2001
+++ build-2.4/drivers/net/pci-skeleton.c	Sun Jun  3 20:01:52 2001
@@ -1862,6 +1862,10 @@
 	return crc;
 }
 
+static void set_bit_32(int offset, u32 * data)
+{
+	data[offset >> 5] |= (1 << (offset & 0x1f));
+}
 
 static void netdrv_set_rx_mode (struct net_device *dev)
 {
@@ -1896,7 +1900,7 @@
 		mc_filter[1] = mc_filter[0] = 0;
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 		     i++, mclist = mclist->next)
-			set_bit (ether_crc (ETH_ALEN, mclist->dmi_addr) >> 26,
+			set_bit_32 (ether_crc (ETH_ALEN, mclist->dmi_addr) >> 26,
 				 mc_filter);
 	}
 
diff -u 2.4/drivers/net/sis900.c build-2.4/drivers/net/sis900.c
--- 2.4/drivers/net/sis900.c	Sat Jun  2 14:19:44 2001
+++ build-2.4/drivers/net/sis900.c	Sun Jun  3 19:51:38 2001
@@ -1870,6 +1870,11 @@
  *	Multicast hash table changes from 128 to 256 bits for 635M/B & 900B0.
  */
 
+static void set_bit_16(int offset, u16 *data)
+{
+	data[offset >> 4] |= (1<<(offset & 0xf));
+}
+
 static void set_rx_mode(struct net_device *net_dev)
 {
 	long ioaddr = net_dev->base_addr;
@@ -1904,7 +1909,7 @@
 		rx_mode = RFAAB;
 		for (i = 0, mclist = net_dev->mc_list; mclist && i < net_dev->mc_count;
 		     i++, mclist = mclist->next)
-			set_bit(sis900_compute_hashtable_index(mclist->dmi_addr, revision),
+			set_bit_16(sis900_compute_hashtable_index(mclist->dmi_addr, revision),
 				mc_filter);
 	}
 
diff -u 2.4/drivers/net/starfire.c build-2.4/drivers/net/starfire.c
--- 2.4/drivers/net/starfire.c	Fri Apr 20 20:54:23 2001
+++ build-2.4/drivers/net/starfire.c	Sun Jun  3 19:53:07 2001
@@ -1185,6 +1185,12 @@
 	return crc;
 }
 
+static void set_bit_16(int offset, u16 *data)
+{
+	data[offset >> 4] |= (1<<(offset & 0xf));
+}
+
+
 static void set_rx_mode(struct net_device *dev)
 {
 	long ioaddr = dev->base_addr;
@@ -1219,12 +1225,12 @@
 	} else {
 		/* Must use a multicast hash table. */
 		long filter_addr;
-		u16 mc_filter[32] __attribute__ ((aligned(sizeof(long))));	/* Multicast hash filter */
+		u16 mc_filter[32];	/* Multicast hash filter */
 
 		memset(mc_filter, 0, sizeof(mc_filter));
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 			 i++, mclist = mclist->next) {
-			set_bit(ether_crc_le(ETH_ALEN, mclist->dmi_addr) >> 23, mc_filter);
+			set_bit_16(ether_crc_le(ETH_ALEN, mclist->dmi_addr) >> 23, mc_filter);
 		}
 		/* Clear the perfect filter list. */
 		filter_addr = ioaddr + 0x56000 + 1*16;
diff -u 2.4/drivers/net/sundance.c build-2.4/drivers/net/sundance.c
--- 2.4/drivers/net/sundance.c	Fri Apr 20 20:54:22 2001
+++ build-2.4/drivers/net/sundance.c	Sun Jun  3 19:53:45 2001
@@ -1121,6 +1121,11 @@
 	return crc;
 }
 
+static void set_bit_16(int offset, u16 *data)
+{
+	data[offset >> 4] |= (1<<(offset & 0xf));
+}
+
 static void set_rx_mode(struct net_device *dev)
 {
 	long ioaddr = dev->base_addr;
@@ -1143,7 +1148,7 @@
 		memset(mc_filter, 0, sizeof(mc_filter));
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 			 i++, mclist = mclist->next) {
-			set_bit(ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x3f,
+			set_bit_16(ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x3f,
 					mc_filter);
 		}
 		rx_mode = AcceptBroadcast | AcceptMultiHash | AcceptMyPhys;
diff -u 2.4/drivers/net/via-rhine.c build-2.4/drivers/net/via-rhine.c
--- 2.4/drivers/net/via-rhine.c	Fri Apr 20 20:54:23 2001
+++ build-2.4/drivers/net/via-rhine.c	Sun Jun  3 19:54:28 2001
@@ -1365,6 +1365,11 @@
     return crc;
 }
 
+static void set_bit_32(int offset, u32 * data)
+{
+	data[offset >> 5] |= (1 << (offset & 0x1f));
+}
+
 static void via_rhine_set_rx_mode(struct net_device *dev)
 {
 	struct netdev_private *np = dev->priv;
@@ -1388,7 +1393,7 @@
 		memset(mc_filter, 0, sizeof(mc_filter));
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
 			 i++, mclist = mclist->next) {
-			set_bit(ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26, mc_filter);
+			set_bit_32(ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26, mc_filter);
 		}
 		writel(mc_filter[0], ioaddr + MulticastFilter0);
 		writel(mc_filter[1], ioaddr + MulticastFilter1);
diff -u 2.4/drivers/net/yellowfin.c build-2.4/drivers/net/yellowfin.c
--- 2.4/drivers/net/yellowfin.c	Sat May 26 10:06:26 2001
+++ build-2.4/drivers/net/yellowfin.c	Sun Jun  3 19:55:38 2001
@@ -1283,6 +1283,10 @@
 	return crc;
 }
 
+static void set_bit_16(int offset, u16 *data)
+{
+	data[offset >> 4] |= (1<<(offset & 0xf));
+}
 
 static void set_rx_mode(struct net_device *dev)
 {
@@ -1309,14 +1313,14 @@
 			/* Due to a bug in the early chip versions, multiple filter
 			   slots must be set for each address. */
 			if (yp->drv_flags & HasMulticastBug) {
-				set_bit((ether_crc_le(3, mclist->dmi_addr) >> 3) & 0x3f,
+				set_bit_16((ether_crc_le(3, mclist->dmi_addr) >> 3) & 0x3f,
 						hash_table);
-				set_bit((ether_crc_le(4, mclist->dmi_addr) >> 3) & 0x3f,
+				set_bit_16((ether_crc_le(4, mclist->dmi_addr) >> 3) & 0x3f,
 						hash_table);
-				set_bit((ether_crc_le(5, mclist->dmi_addr) >> 3) & 0x3f,
+				set_bit_16((ether_crc_le(5, mclist->dmi_addr) >> 3) & 0x3f,
 						hash_table);
 			}
-			set_bit((ether_crc_le(6, mclist->dmi_addr) >> 3) & 0x3f,
+			set_bit_16((ether_crc_le(6, mclist->dmi_addr) >> 3) & 0x3f,
 					hash_table);
 		}
 		/* Copy the hash table to the chip. */

Re: multicast hash incorrect on big endian archs

[David S. Miller]

Manfred Spraul writes:
 > I noticed that the multicast hash calculations assumed little endian
 > byte ordering in the winbond-840 driver, and it seems that several other
 > drivers are also affected:
 > 
 > 8139too, epic100, fealnx, pci-skeleton, sis900, starfile, sundance,
 > via-rhine, yellowfin
 > perhaps drivers/net/pcmcia/xircom_tulip_cb

Many big-endian systems already need to provide little-endian bitops,
for ext2's sake for example.

We should formalize this, with {set,clear,change,test}_le_bit which
technically every port has implemented in some for or another already.

Later,
David S. Miller
davem@redhat.com

Re: multicast hash incorrect on big endian archs

[Manfred Spraul]

"David S. Miller" wrote:
> 
> Manfred Spraul writes:
>  > I noticed that the multicast hash calculations assumed little endian
>  > byte ordering in the winbond-840 driver, and it seems that several other
>  > drivers are also affected:
>  >
>  > 8139too, epic100, fealnx, pci-skeleton, sis900, starfile, sundance,
>  > via-rhine, yellowfin
>  > perhaps drivers/net/pcmcia/xircom_tulip_cb
> 
> Many big-endian systems already need to provide little-endian bitops,
> for ext2's sake for example.
> 
> We should formalize this, with {set,clear,change,test}_le_bit which
> technically every port has implemented in some for or another already.
>
The multicast hash is written into a nic register with

	set_bit(crc(...),mc_list);
	...
	out{b,w,l}(mc_list[i],ioaddr);

set_bit_le only helps for outb. My patch uses set_bit_16 and set_bit_32.

Another option would be
	set_bit_le(crc(...),mc_list)
	...
	out{w,l}(le{16,32}_to_cpu(mc_list[i]),ioaddr);

but I think set_bit_{8,16,32,64} are the better solution.

Obviously we could move them into a header file.

--
	Manfred

Re: multicast hash incorrect on big endian archs

[Manfred Spraul]

Manfred Spraul wrote:
> 
> "David S. Miller" wrote:
> >
> > Many big-endian systems already need to provide little-endian bitops,
> > for ext2's sake for example.
> >
> > We should formalize this, with {set,clear,change,test}_le_bit which
> > technically every port has implemented in some for or another already.
> >

That could cause alignment problems.
<<< from starfire.c
{
     long filter_addr;
     u16 mc_filter[32] __attribute__ ((aligned(sizeof(long)))); 
<<<
set_bit requires word alignment, but without the __attibute__ the
compiler would only guarantee 16-bit alignment. IMHO ugly.

Should I add __set_bit_{8,16,32} into <linux/bitops.h>, overridable with
__HAVE_ARCH_SET_BIT_n?

Default implementation for the nonatomic __set_bit could be added into
<linux/bitops.h>, too.

Btw, the correct name would be __set_bit_n: the function don't guarantee
atomicity.

--
	Manfred

Re: multicast hash incorrect on big endian archs

[David S. Miller]

Manfred Spraul writes:
 > That could cause alignment problems.
 > <<< from starfire.c
 > {
 >      long filter_addr;
 >      u16 mc_filter[32] __attribute__ ((aligned(sizeof(long)))); 
 > <<<
 > set_bit requires word alignment, but without the __attibute__ the
 > compiler would only guarantee 16-bit alignment. IMHO ugly.

Correction, it requires "long" alignment and that is 64-bits
on several platforms.

Later,
David S. Miller
davem@redhat.com