Skip to contentSkip to conversation

Endian-ness bugs in yellowfin.c

PATCH In review Val Henson 7 participants 14 messages Sep 14, 2001

Subsystems (2)
Network drivers Documentation
lore
Share & tools
lore

Val Henson

author
#0 lore raw

PATCH 1/1 Endian-ness bugs in yellowfin.c

This patch applies to the 2.4.10-pre8 yellowfin driver. What it does:

* Fixes three bugs on big-endian architecture
* Changes flags to support at least one SYM53C885E card
* Replaces ncr885e driver with yellowfin

I ran into a couple of design decisions people won't like:

This driver is both a 1000 Mbit driver and 100 Mbit driver. My solution was to list the driver once under each category with the appropriate name.

The flag IsGigabit seems misnamed, since it doesn't seem to control anything related to gigabit-ness. I turned it on for the SYMBIOS card since, at least on my card, it uses the correct code.

#ifdef __powerpc__ probably should be changed to some form of #ifdef BIG_ENDIAN but I didn't include that in this patch.

And finally, I'd like to remove ncr885e.c entirely since it's redundant and extremely buggy. Any objections?

-VAL

Documentation/Configure.help +4 -4
diff -Nru a/Documentation/Configure.help b/Documentation/Configure.help
@@ -8824,10 +8824,10 @@
 Packet Engines Yellowfin Gigabit-NIC support
 CONFIG_YELLOWFIN
   Say Y here if you have a Packet Engines G-NIC PCI Gigabit Ethernet
-  adapter. This adapter is used by the Beowulf Linux cluster project.
-  See http://cesdis.gsfc.nasa.gov/linux/drivers/yellowfin.html for
-  more information about this driver in particular and Beowulf in
-  general.
+  adapter or the SYM53C885 Ethernet controller. The Gigabit adapter is
+  used by the Beowulf Linux cluster project.  See
+  http://cesdis.gsfc.nasa.gov/linux/drivers/yellowfin.html for more
+  information about this driver in particular and Beowulf in general.
 
   If you want to compile this driver as a module ( = code which can be
   inserted in and removed from the running kernel whenever you want),
drivers/net/Config.in +1 -1
diff -Nru a/drivers/net/Config.in b/drivers/net/Config.in
@@ -39,7 +39,7 @@
       fi
       dep_tristate '  BMAC (G3 ethernet) support' CONFIG_BMAC $CONFIG_ALL_PPC
       dep_tristate '  GMAC (G4/iBook ethernet) support' CONFIG_GMAC $CONFIG_ALL_PPC
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN
       tristate '  National DP83902AV (Oak ethernet) support' CONFIG_OAKNET
       dep_bool '  PowerPC 405 on-chip ethernet' CONFIG_PPC405_ENET $CONFIG_405GP
       if [ "$CONFIG_PPC405_ENET" = "y" ]; then
drivers/net/yellowfin.c +6 -6
diff -Nru a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c
@@ -276,7 +276,7 @@
 	 PCI_IOTYPE, YELLOWFIN_SIZE,
 	 FullTxStatus | IsGigabit | HasMulticastBug | HasMACAddrBug},
 	{"Symbios SYM83C885", { 0x07011000, 0xffffffff},
-	 PCI_IOTYPE, YELLOWFIN_SIZE, HasMII },
+	 PCI_IOTYPE, YELLOWFIN_SIZE, HasMII | IsGigabit | FullTxStatus },
 	{0,},
 };
 

@@ -785,8 +785,8 @@
 			break;
 		skb->dev = dev;		/* Mark as being used by this device. */
 		skb_reserve(skb, 2);	/* 16 byte align the IP header. */
-		yp->rx_ring[i].addr = pci_map_single(yp->pci_dev, skb->tail,
-			yp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		yp->rx_ring[i].addr = cpu_to_le32(pci_map_single(yp->pci_dev,
+			skb->tail, yp->rx_buf_sz, PCI_DMA_FROMDEVICE));
 	}
 	yp->rx_ring[i-1].dbdma_cmd = cpu_to_le32(CMD_STOP);
 	yp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);

@@ -1109,7 +1109,7 @@
 		buf_addr = rx_skb->tail;
 		data_size = (le32_to_cpu(desc->dbdma_cmd) - 
 			le32_to_cpu(desc->result_status)) & 0xffff;
-		frame_status = get_unaligned((s16*)&(buf_addr[data_size - 2]));
+		frame_status = le16_to_cpu(get_unaligned((s16*)&(buf_addr[data_size - 2])));
 		if (yellowfin_debug > 4)
 			printk(KERN_DEBUG "  yellowfin_rx() status was %4.4x.\n",
 				   frame_status);

@@ -1206,8 +1206,8 @@
 			yp->rx_skbuff[entry] = skb;
 			skb->dev = dev;	/* Mark as being used by this device. */
 			skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
-			yp->rx_ring[entry].addr = pci_map_single(yp->pci_dev,
-				skb->tail, yp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+			yp->rx_ring[entry].addr = cpu_to_le32(pci_map_single(yp->pci_dev,
+				skb->tail, yp->rx_buf_sz, PCI_DMA_FROMDEVICE));
 		}
 		yp->rx_ring[entry].dbdma_cmd = cpu_to_le32(CMD_STOP);
 		yp->rx_ring[entry].result_status = 0;	/* Clear complete bit. */
#1 lore raw

drivers/net/Config.in line 45

Quoted 12 lines · Val Henson

diff -Nru a/drivers/net/Config.in b/drivers/net/Config.in

drivers/net/Config.in +1 -1
@@ -39,7 +39,7 @@
       fi
       dep_tristate '  BMAC (G3 ethernet) support' CONFIG_BMAC $CONFIG_ALL_PPC
       dep_tristate '  GMAC (G4/iBook ethernet) support' CONFIG_GMAC $CONFIG_ALL_PPC
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN
       tristate '  National DP83902AV (Oak ethernet) support' CONFIG_OAKNET
       dep_bool '  PowerPC 405 on-chip ethernet' CONFIG_PPC405_ENET $CONFIG_405GP
       if [ "$CONFIG_PPC405_ENET" = "y" ]; then

Since you're killing this, why not just remove this question entirely?

Signature · 2 lines
Tom Rini (TR1265)
http://gate.crashing.org/~trini/
#2 lore raw
In reply to Val Henson
Quoted 11 lines · Val Henson
Quoted 5 lines · Tom Rini

drivers/net/Config.in line 42

Quoted 2 lines · Val Henson
drivers/net/Config.in +1 -1
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN

Since you're killing this, why not just remove this question entirely?

This is one of the "design decisions" I referred to. It makes no sense to list a 100 Mbit driver under "Ethernet (1000 Mbit)". This is my solution. This is the first case of a dual 1000/100 Mbit driver and if there's a better way to handle it I'd like to hear it.

Er, sungem does 10/100/1000 too I think.. (It's what's in the new G4 towers..). IMHO, listing it once under the greatest makes sense. But sungem/gmac are under 10/100 I think.

Signature · 2 lines
Tom Rini (TR1265)
http://gate.crashing.org/~trini/
author
#3 lore raw
In reply to Val Henson
Quoted 5 lines · Tom Rini

drivers/net/Config.in line 42

Quoted 2 lines · Val Henson
drivers/net/Config.in +1 -1
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN

Since you're killing this, why not just remove this question entirely?

This is one of the "design decisions" I referred to. It makes no sense to list a 100 Mbit driver under "Ethernet (1000 Mbit)". This is my solution. This is the first case of a dual 1000/100 Mbit driver and if there's a better way to handle it I'd like to hear it.

-VAL

#4 lore raw
In reply to Val Henson

I would agree that listing them under the highest capable is probably the best. It is, at least, consistent with the 10/100 cards.

Also, people will pick up the card, think "gigabit ethernet", and then look under the 1000 section. I don't think anyone will really think GigE and then look under 10/100.

The Intel 82543 and 82544 gigabit parts are all 10/100/1000 -- I'll be writing a driver for those in a few weeks if nobody beats me to it, so I think it would be good to settle this.

Matt

Quoted 25 lines · Tom Rini
Quoted 11 lines · Val Henson
Quoted 5 lines · Tom Rini

drivers/net/Config.in line 42

Quoted 2 lines · Val Henson
drivers/net/Config.in +1 -1
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN

Since you're killing this, why not just remove this question entirely?

This is one of the "design decisions" I referred to. It makes no sense to list a 100 Mbit driver under "Ethernet (1000 Mbit)". This is my solution. This is the first case of a dual 1000/100 Mbit driver and if there's a better way to handle it I'd like to hear it.

Er, sungem does 10/100/1000 too I think.. (It's what's in the new G4 towers..). IMHO, listing it once under the greatest makes sense. But sungem/gmac are under 10/100 I think.

Signature · 7 lines
Tom Rini (TR1265)
http://gate.crashing.org/~trini/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Signature · 7 lines
Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997
#5 lore raw
Quoted 3 lines

Also, people will pick up the card, think "gigabit ethernet", and then look under the 1000 section. I don't think anyone will really think GigE and then look under 10/100.

List it once under GigE

author
#6 lore raw
In reply to Val Henson
Quoted 2 lines · Matthew Dharm

I would agree that listing them under the highest capable is probably the best. It is, at least, consistent with the 10/100 cards.

I'm interested to hear why you think this is better than listing the card under both of the 100 and 1000 Mbit categories (as the patch I sent does it). Also, we are not talking about one 10/100/1000 card, we are talking about one driver that supports two separate cards, one 10/100/1000 and one 10/100.

-VAL

Quoted 45 lines

Also, people will pick up the card, think "gigabit ethernet", and then look under the 1000 section. I don't think anyone will really think GigE and then look under 10/100.

The Intel 82543 and 82544 gigabit parts are all 10/100/1000 -- I'll be writing a driver for those in a few weeks if nobody beats me to it, so I think it would be good to settle this.

Matt

Quoted 25 lines · Tom Rini
Quoted 11 lines · Val Henson
Quoted 5 lines · Tom Rini

drivers/net/Config.in line 42

Quoted 2 lines · Val Henson
drivers/net/Config.in +1 -1
-      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_NCR885E
+      tristate '  Symbios 53c885 (Synergy ethernet) support' CONFIG_YELLOWFIN

Since you're killing this, why not just remove this question entirely?

This is one of the "design decisions" I referred to. It makes no sense to list a 100 Mbit driver under "Ethernet (1000 Mbit)". This is my solution. This is the first case of a dual 1000/100 Mbit driver and if there's a better way to handle it I'd like to hear it.

Er, sungem does 10/100/1000 too I think.. (It's what's in the new G4 towers..). IMHO, listing it once under the greatest makes sense. But sungem/gmac are under 10/100 I think.

Signature · 7 lines
Tom Rini (TR1265)
http://gate.crashing.org/~trini/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Signature · 7 lines
Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997
author
#7 lore raw
In reply to Val Henson
Quoted 10 lines · Tom Rini
Quoted 5 lines · Val Henson

This is one of the "design decisions" I referred to. It makes no sense to list a 100 Mbit driver under "Ethernet (1000 Mbit)". This is my solution. This is the first case of a dual 1000/100 Mbit driver and if there's a better way to handle it I'd like to hear it.

Er, sungem does 10/100/1000 too I think.. (It's what's in the new G4 towers..). IMHO, listing it once under the greatest makes sense. But sungem/gmac are under 10/100 I think.

You misunderstood what I meant. This is the first case of one driver supporting two different cards, one 10/100 and one 10/100/1000. All the gigabit cards are 10/100/1000 as far as I know. I still think the driver should be listed in both the 100 Mbit and 1000 Mbit Ethernet menu sections, unless someone comes up with a better idea.

-VAL

#8 lore raw
Quoted 5 lines · Val Henson

You misunderstood what I meant. This is the first case of one driver supporting two different cards, one 10/100 and one 10/100/1000. All the gigabit cards are 10/100/1000 as far as I know. I still think the driver should be listed in both the 100 Mbit and 1000 Mbit Ethernet menu sections, unless someone comes up with a better idea.

Please do not add duplicate items... put drivers that can do gigabit under gigabit. Eventually as we get more of them we can come up with better Config.in categories. Besides being an ugly solution, duplicating items has the potential to push edge cases in the various Config.in parsers.

Jeff

#9 lore raw

[email protected] (Jeff Garzik) wrote on 14.09.01 in <Pine.LNX.3.96.1010914014353.8683A-100000@mandrakesoft.mandrakesoft.com>:

Quoted 7 lines · Val Henson
Quoted 5 lines

You misunderstood what I meant. This is the first case of one driver supporting two different cards, one 10/100 and one 10/100/1000. All the gigabit cards are 10/100/1000 as far as I know. I still think the driver should be listed in both the 100 Mbit and 1000 Mbit Ethernet menu sections, unless someone comes up with a better idea.

Quoted 5 lines

Please do not add duplicate items... put drivers that can do gigabit under gigabit. Eventually as we get more of them we can come up with better Config.in categories. Besides being an ugly solution, duplicating items has the potential to push edge cases in the various Config.in parsers.

Actually, I'd say that practically *all* the categories used for ethernet cards are confusing. I certainly often end up backtracking several times when configuring a new machine.

Just throwing the lot out and using a flat list would be better than the current state.

Bad category strategies:

* Brand name. Drivers serve different brands.
* Bus type. Drivers serve cards on different bus types.
* Speed. Drivers serve cards capable of different speeds.

I really don't know what's left.

Unless, of course, mentioning one driver several places becomes possible. Then all these arguments become irrelevant.

MfG Kai

author
#10 lore raw
In reply to Val Henson
Quoted 7 lines · Kai Henningsen

Actually, I'd say that practically *all* the categories used for ethernet cards are confusing. I certainly often end up backtracking several times when configuring a new machine.

Just throwing the lot out and using a flat list would be better than the current state.

I could not agree more.

-VAL

#11 lore raw
In reply to Val Henson
Quoted 2 lines · Val Henson

And finally, I'd like to remove ncr885e.c entirely since it's redundant and extremely buggy. Any objections?

I may be missing some context... have you tested yellowfin on big endian boxes? If so, go ahead and remove it. Cort said it was destined for the scrapheap a while ago, and IIRC it disappeared from the 'ac' tree for a while...

#12 lore raw
In reply to Val Henson

Only the PPC-based gemini board uses that driver (ncr885e) and Val is the maintainer of that board. These patches that she sent fix yellowfin.c for the ncr885e chip on the big-endian PPC. I just pointed at the junk pile, Val is actually doing the work to put the ncr885e drivers there.

This is a truly rare patch, it allows the removal of entire drivers not just a few lines. As an encore I suggest combining the >5 Zilog 8530 drivers.

} I may be missing some context... have you tested yellowfin on big endian } boxes? If so, go ahead and remove it. Cort said it was destined for } the scrapheap a while ago, and IIRC it disappeared from the 'ac' tree } for a while...

author
#13 lore raw
In reply to Val Henson

(Removed Donald Becker from CC list since his mail server is timing out.)

Quoted 4 lines · Cort Dougan

Only the PPC-based gemini board uses that driver (ncr885e) and Val is the maintainer of that board. These patches that she sent fix yellowfin.c for the ncr885e chip on the big-endian PPC. I just pointed at the junk pile, Val is actually doing the work to put the ncr885e drivers there.

Yep, exactly. Here's the revised patch. It lists the yellowfin driver once in Config.in, removes the ncr885e driver, and changes the #ifdef __powerpc__ to #ifdef __BIG_ENDIAN.

Unfortunately, the patch is now 56K. It's available for download here:

http://www.nmt.edu/~val/patches/yellowfinpatch

-VAL