[PATCH] Endian-ness bugs in yellowfin.c

[PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

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

diff -Nru a/Documentation/Configure.help b/Documentation/Configure.help
--- a/Documentation/Configure.help	Thu Sep 13 19:20:29 2001
+++ b/Documentation/Configure.help	Thu Sep 13 19:20:29 2001
@@ -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),
diff -Nru a/drivers/net/Config.in b/drivers/net/Config.in
--- a/drivers/net/Config.in	Thu Sep 13 19:20:29 2001
+++ b/drivers/net/Config.in	Thu Sep 13 19:20:29 2001
@@ -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
diff -Nru a/drivers/net/yellowfin.c b/drivers/net/yellowfin.c
--- a/drivers/net/yellowfin.c	Thu Sep 13 19:20:29 2001
+++ b/drivers/net/yellowfin.c	Thu Sep 13 19:20:29 2001
@@ -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. */

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Tom Rini]

On Thu, Sep 13, 2001 at 07:51:41PM -0600, Val Henson wrote:

> diff -Nru a/drivers/net/Config.in b/drivers/net/Config.in
> --- a/drivers/net/Config.in	Thu Sep 13 19:20:29 2001
> +++ b/drivers/net/Config.in	Thu Sep 13 19:20:29 2001
> @@ -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?

-- 
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

On Thu, Sep 13, 2001 at 07:39:37PM -0700, Tom Rini wrote:
> On Thu, Sep 13, 2001 at 07:51:41PM -0600, Val Henson wrote:
> > -      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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Tom Rini]

On Thu, Sep 13, 2001 at 08:55:01PM -0600, Val Henson wrote:
> On Thu, Sep 13, 2001 at 07:39:37PM -0700, Tom Rini wrote:
> > On Thu, Sep 13, 2001 at 07:51:41PM -0600, Val Henson wrote:
> > > -      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.

-- 
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Matthew Dharm]

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

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

On Thu, Sep 13, 2001 at 08:02:37PM -0700, Tom Rini wrote:
> On Thu, Sep 13, 2001 at 08:55:01PM -0600, Val Henson wrote:
> > On Thu, Sep 13, 2001 at 07:39:37PM -0700, Tom Rini wrote:
> > > On Thu, Sep 13, 2001 at 07:51:41PM -0600, Val Henson wrote:
> > > > -      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.
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

On Thu, Sep 13, 2001 at 10:01:18PM -0700, Matthew Dharm wrote:
> 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

> 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
> 
> On Thu, Sep 13, 2001 at 08:02:37PM -0700, Tom Rini wrote:
> > On Thu, Sep 13, 2001 at 08:55:01PM -0600, Val Henson wrote:
> > > On Thu, Sep 13, 2001 at 07:39:37PM -0700, Tom Rini wrote:
> > > > On Thu, Sep 13, 2001 at 07:51:41PM -0600, Val Henson wrote:
> > > > > -      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.
> > 
> > -- 
> > 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> -- 
> Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
> 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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

On Thu, Sep 13, 2001 at 08:02:37PM -0700, Tom Rini wrote:
> On Thu, Sep 13, 2001 at 08:55:01PM -0600, Val Henson wrote:
> > 
> > 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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Jeff Garzik]

On Fri, 14 Sep 2001, Val Henson wrote:
> 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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Jeff Garzik]

On Thu, 13 Sep 2001, Val Henson wrote:
> 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...

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Kai Henningsen]

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

> On Fri, 14 Sep 2001, Val Henson wrote:
> > 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.

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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Alan Cox]

> 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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[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.

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...

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

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

On Fri, Sep 14, 2001 at 12:50:54PM -0600, Cort Dougan wrote:
> 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

Re: [PATCH] Endian-ness bugs in yellowfin.c

[Val Henson]

On Fri, Sep 14, 2001 at 10:07:00AM +0200, Kai Henningsen wrote:
> 
> 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