Skip to contentSkip to conversation

multicast hash incorrect on big endian archs

PATCH Manfred Spraul 2 participants 5 messages Jun 3, 2001 – Jun 4, 2001 Network drivers

lore
Share & tools
lore

Manfred Spraul

author
#0 lore raw

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

drivers/net/8139too.c +5 -1
diff -u 2.4/drivers/net/8139too.c build-2.4/drivers/net/8139too.c
@@ -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);
 	}
drivers/net/epic100.c +8 -3
diff -u 2.4/drivers/net/epic100.c build-2.4/drivers/net/epic100.c
@@ -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;
drivers/net/fealnx.c +5 -1
diff -u 2.4/drivers/net/fealnx.c build-2.4/drivers/net/fealnx.c
@@ -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;
drivers/net/pci-skeleton.c +5 -1
diff -u 2.4/drivers/net/pci-skeleton.c build-2.4/drivers/net/pci-skeleton.c
@@ -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);
 	}
drivers/net/sis900.c +6 -1
diff -u 2.4/drivers/net/sis900.c build-2.4/drivers/net/sis900.c
@@ -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);
 	}
drivers/net/starfire.c +8 -2
diff -u 2.4/drivers/net/starfire.c build-2.4/drivers/net/starfire.c
@@ -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;
drivers/net/sundance.c +6 -1
diff -u 2.4/drivers/net/sundance.c build-2.4/drivers/net/sundance.c
@@ -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;
drivers/net/via-rhine.c +6 -1
diff -u 2.4/drivers/net/via-rhine.c build-2.4/drivers/net/via-rhine.c
@@ -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);
drivers/net/yellowfin.c +8 -4
diff -u 2.4/drivers/net/yellowfin.c build-2.4/drivers/net/yellowfin.c
@@ -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. */
#1 lore raw
Quoted 7 lines · Manfred Spraul

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
[email protected]

author
#2 lore raw
In reply to Manfred Spraul
Quoted 10 lines · David S. Miller
Quoted 7 lines · Manfred Spraul

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

Quoted 6 lines

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

author
#3 lore raw
In reply to Manfred Spraul
Quoted 9 lines · Manfred Spraul
Quoted 7 lines · David S. Miller

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

#4 lore raw
Quoted 8 lines · Manfred Spraul

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
[email protected]