[PATCH] sky2: sky2 FE+ receive status workaround

[PATCH] sky2: sky2 FE+ receive status workaround

[Stephen Hemminger]

The Yukon FE+ chip appears to have a hardware glitch that causes bogus
receive status values to be posted. The data in the packet is good, but
the status value is random garbage.  As a temporary workaround until the
problem is better understood, implement the workaround the vendor driver
used of ignoring the status value on this chip.

Since this means trusting dodgy hardware values; add additional checking
of the receive packet length.

Patch against 2.6.23-rc8, please apply before 2.6.23 release (or it will
need to go to stable).

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/drivers/net/sky2.c	2007-09-26 14:34:34.000000000 -0700
+++ b/drivers/net/sky2.c	2007-09-26 16:57:31.000000000 -0700
@@ -2148,6 +2148,18 @@ static struct sk_buff *sky2_receive(stru
 	sky2->rx_next = (sky2->rx_next + 1) % sky2->rx_pending;
 	prefetch(sky2->rx_ring + sky2->rx_next);
 
+	if (length < ETH_ZLEN || length > sky2->rx_data_size)
+		goto len_error;
+
+	/* This chip has hardware problems that generates bogus status.
+	 * So do only marginal checking and expect higher level protocols
+	 * to handle crap frames.
+	 */
+	if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
+	    sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
+	    length != count)
+		goto okay;
+
 	if (status & GMR_FS_ANY_ERR)
 		goto error;
 
@@ -2156,8 +2168,9 @@ static struct sk_buff *sky2_receive(stru
 
 	/* if length reported by DMA does not match PHY, packet was truncated */
 	if (length != count)
-		goto len_mismatch;
+		goto len_error;
 
+okay:
 	if (length < copybreak)
 		skb = receive_copy(sky2, re, length);
 	else
@@ -2167,13 +2180,13 @@ resubmit:
 
 	return skb;
 
-len_mismatch:
+len_error:
 	/* Truncation of overlength packets
 	   causes PHY length to not match MAC length */
 	++sky2->net_stats.rx_length_errors;
 	if (netif_msg_rx_err(sky2) && net_ratelimit())
-		pr_info(PFX "%s: rx length mismatch: length %d status %#x\n",
-			dev->name, length, status);
+		pr_info(PFX "%s: rx length error: status %#x length %d\n",
+			dev->name, status, length);
 	goto resubmit;
 
 error:
@@ -3934,13 +3947,6 @@ static __devinit struct net_device *sky2
 	sky2->hw = hw;
 	sky2->msg_enable = netif_msg_init(debug, default_msg);
 
-	/* This chip has hardware problems that generates
-	 * bogus PHY receive status so by default shut up the message.
-	 */
-	if (hw->chip_id == CHIP_ID_YUKON_FE_P &&
-	    hw->chip_rev == CHIP_REV_YU_FE2_A0)
-		sky2->msg_enable &= ~NETIF_MSG_RX_ERR;
-
 	/* Auto speed and flow control */
 	sky2->autoneg = AUTONEG_ENABLE;
 	sky2->flow_mode = FC_BOTH;

Re: [PATCH] sky2: sky2 FE+ receive status workaround

[Jochen Voß]

Hi Stephen,

On 27 Sep 2007, at 01:58, Stephen Hemminger wrote:
> +	/* This chip has hardware problems that generates bogus status.
> +	 * So do only marginal checking and expect higher level protocols
> +	 * to handle crap frames.
> +	 */
> +	if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
> +	    sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
> +	    length != count)
> +		goto okay;

Shouldn't the condition be "length == count"?

I hope this helps,
Jochen
--
http://seehuhn.de/

Re: [PATCH] sky2: sky2 FE+ receive status workaround

[Stephen Hemminger]

On Thu, 27 Sep 2007 09:14:11 +0100
Jochen Voß <voss@seehuhn.de> wrote:

> Hi Stephen,
> 
> On 27 Sep 2007, at 01:58, Stephen Hemminger wrote:
> > +	/* This chip has hardware problems that generates bogus status.
> > +	 * So do only marginal checking and expect higher level protocols
> > +	 * to handle crap frames.
> > +	 */
> > +	if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
> > +	    sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
> > +	    length != count)
> > +		goto okay;
> 
> Shouldn't the condition be "length == count"?
> 

No, the code is correct as is.  Basically if length == count, then
the status field is correct, and the driver can go ahead and use it.
If length != count, then the status is bogus but the data is okay.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] sky2: sky2 FE+ receive status workaround

[Jochen Voss]

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

Hi,

On Thu, Sep 27, 2007 at 06:58:07AM -0700, Stephen Hemminger wrote:
> On Thu, 27 Sep 2007 09:14:11 +0100 Jochen Voß <voss@seehuhn.de> wrote:
> > On 27 Sep 2007, at 01:58, Stephen Hemminger wrote:
> > > +	/* This chip has hardware problems that generates bogus status.
> > > +	 * So do only marginal checking and expect higher level protocols
> > > +	 * to handle crap frames.
> > > +	 */
> > > +	if (sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
> > > +	    sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0 &&
> > > +	    length != count)
> > > +		goto okay;
> > 
> > Shouldn't the condition be "length == count"?
> 
> No, the code is correct as is.  Basically if length == count, then
> the status field is correct, and the driver can go ahead and use it.
> If length != count, then the status is bogus but the data is okay.

Oh, I see.  Thanks for the explanation.

All the best,
Jochen
-- 
http://seehuhn.de/

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

[PATCH 1/2] sky2: FE+ vlan workaround

[Stephen Hemminger]

The FE+ workaround means the driver can no longer trust the status register
to indicate VLAN tagged frames.  The fix for this is to just disable VLAN
acceleration for that chip version. Tested and works fine.

This patch applies to 2.6.23-rc8 after yesterday's patch:
      sky2 FE+ receive status workaround

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/drivers/net/sky2.c	2007-09-27 08:45:13.000000000 -0700
+++ b/drivers/net/sky2.c	2007-09-27 09:43:15.000000000 -0700
@@ -3970,8 +3970,12 @@ static __devinit struct net_device *sky2
 		dev->features |= NETIF_F_HIGHDMA;
 
 #ifdef SKY2_VLAN_TAG_USED
-	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-	dev->vlan_rx_register = sky2_vlan_rx_register;
+	/* The workaround for FE+ status conflicts with VLAN tag detection. */
+	if (!(sky2->hw->chip_id == CHIP_ID_YUKON_FE_P &&
+	      sky2->hw->chip_rev == CHIP_REV_YU_FE2_A0)) {
+		dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
+		dev->vlan_rx_register = sky2_vlan_rx_register;
+	}
 #endif
 
 	/* read the mac address */

Re: [PATCH 1/2] sky2: FE+ vlan workaround

[Jeff Garzik]

Stephen Hemminger wrote:
> This patch applies to 2.6.23-rc8 after yesterday's patch:
>       sky2 FE+ receive status workaround

Applied.  Please always put meta-information such as the above quoted 
after the "---" patch description terminator, so that it is not copied 
into the permanent kernel changelog.  This text must be hand-edited out 
of each patch, before application.

See item #14 of Documentation/SubmittingPatches.

[PATCH 2/2] sky2: fix transmit state on resume

[Stephen Hemminger]

This should fix http://bugzilla.kernel.org/show_bug.cgi?id=8667

After resume, driver has reset the chip so the current state
of transmit checksum offload state machine and DMA state machine
will be undefined.

The fix is to set the state so that first Tx will set MSS and offset
values.

Patch is against 2.6.23-rc8 after last patch:
 sky2: FE+ vlan workaround

(Should also work on older releases with minor fuzz).

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/drivers/net/sky2.c	2007-09-27 08:45:13.000000000 -0700
+++ b/drivers/net/sky2.c	2007-09-27 09:39:49.000000000 -0700
@@ -910,6 +910,20 @@ static inline struct sky2_tx_le *get_tx_
 	return le;
 }
 
+static void tx_init(struct sky2_port *sky2)
+{
+	struct sky2_tx_le *le;
+
+	sky2->tx_prod = sky2->tx_cons = 0;
+	sky2->tx_tcpsum = 0;
+	sky2->tx_last_mss = 0;
+
+	le = get_tx_le(sky2);
+	le->addr = 0;
+	le->opcode = OP_ADDR64 | HW_OWNER;
+	sky2->tx_addr64 = 0;
+}
+
 static inline struct tx_ring_info *tx_le_re(struct sky2_port *sky2,
 					    struct sky2_tx_le *le)
 {
@@ -1320,7 +1334,8 @@ static int sky2_up(struct net_device *de
 				GFP_KERNEL);
 	if (!sky2->tx_ring)
 		goto err_out;
-	sky2->tx_prod = sky2->tx_cons = 0;
+
+	tx_init(sky2);
 
 	sky2->rx_le = pci_alloc_consistent(hw->pdev, RX_LE_BYTES,
 					   &sky2->rx_le_map);

Re: [PATCH 2/2] sky2: fix transmit state on resume

[Jeff Garzik]

Stephen Hemminger wrote:
> Patch is against 2.6.23-rc8 after last patch:
>  sky2: FE+ vlan workaround
> 
> (Should also work on older releases with minor fuzz).

Applied.  Please always put meta-information such as the above quoted 
after the "---" patch description terminator, so that it is not copied 
into the permanent kernel changelog.  This text must be hand-edited out 
of each patch, before application.

See item #14 of Documentation/SubmittingPatches.

Re: [PATCH] sky2: sky2 FE+ receive status workaround

[Jeff Garzik]

Stephen Hemminger wrote:
> Patch against 2.6.23-rc8, please apply before 2.6.23 release (or it will
> need to go to stable).


Applied.  Please always put meta-information such as the above quoted 
after the "---" patch description terminator, so that it is not copied 
into the permanent kernel changelog.  This text must be hand-edited out 
of each patch, before application.