NetBSD Problem Report #47788

From www@NetBSD.org  Mon Apr 29 21:50:24 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id D754C63EBCD
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 29 Apr 2013 21:50:23 +0000 (UTC)
Message-Id: <20130429215022.9275763EBCD@www.NetBSD.org>
Date: Mon, 29 Apr 2013 21:50:22 +0000 (UTC)
From: webpages@sprow.co.uk
Reply-To: webpages@sprow.co.uk
To: gnats-bugs@NetBSD.org
Subject: Incorrect packet length transmitted for odd-lengths with smc91cxx driver
X-Send-Pr-Version: www-1.0

>Number:         47788
>Category:       kern
>Synopsis:       Incorrect packet length transmitted for odd-lengths with smc91cxx driver
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Apr 29 21:55:01 +0000 2013
>Closed-Date:    Mon Oct 07 09:54:10 +0000 2013
>Last-Modified:  Mon Oct 07 09:54:10 +0000 2013
>Originator:     Robert Sprowson
>Release:        N/A
>Organization:
N/A
>Environment:
N/A
>Description:
The padding logic in src/sys/dev/ic/smc91cxx.c is incorrectly padding by being confused about padding to get to ETHER_MIN_LEN and padding needed because the SMSC91Cxx family needs odd length packets handling specially.

By example, the following "ping -s" commands yield the following packet sizes when inspected with WireShark

  1400 -> 1400 (correct)
  1401 -> 1402 (1 byte too long)
  1402 -> 1402 (correct)
  1403 -> 1405 (2 bytes too long)

this pattern repeats every 4 bytes.

The patch below simplifies the copying routine to always stop at a word boundary, regardless of the state of SMC91CXX_NO_BYTE_WRITE, then builds any padding needed in variables which are then written using only word accesses (hence are compatible with both byte-write capable and non-byte-write capable targets).

Tested by inspection in WireShark with "ping -s" of sizes 8; 9; 10; 11 (to check the padding of < ETHER_MIN_LEN) and 1400 to 1407 inclusive.

Please note the patch is a diff assuming the patch in PR/47765 is applied first.
>How-To-Repeat:
ping -s 1401 my.host.address
ping -s 1403 my.host.address

or any other odd number.
>Fix:
--- smc91cxx_new.c	Thu Apr 25 06:49:32 2013
+++ smc91cxx_len.c	Mon Apr 29 21:30:58 2013
@@ -652,13 +652,12 @@
 	 */
 	for (len = 0; m != NULL; m = m->m_next)
 		len += m->m_len;
-	pad = (len & 1);

 	/*
 	 * We drop packets that are too large.  Perhaps we should
 	 * truncate them instead?
 	 */
-	if ((len + pad) > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
+	if (len > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
 		printf("%s: large packet discarded\n", device_xname(sc->sc_dev));
 		ifp->if_oerrors++;
 		IFQ_DEQUEUE(&ifp->if_snd, m);
@@ -666,6 +665,7 @@
 		goto readcheck;
 	}

+	pad = 0;
 #ifdef SMC91CXX_SW_PAD
 	/*
 	 * Not using hardware padding; pad to ETHER_MIN_LEN.
@@ -745,24 +745,25 @@
 	IFQ_DEQUEUE(&ifp->if_snd, m);

 	/*
-	 * Push the packet out to the card.
+	 * Push the packet out to the card.  The copying function only does whole
+	 * words and returns the straggling byte (if any).
 	 */
 	oddbyte = smc91cxx_copy_tx_frame(sc, m);

 #ifdef SMC91CXX_SW_PAD
-#ifdef SMC91CXX_NO_BYTE_WRITE
 #if BYTE_ORDER == LITTLE_ENDIAN
 	if (pad > 1 && (pad & 1)) {
 		bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 0);
 		oddbyte = 0;
+		pad -= 1;
 	}
 #else
 	if (pad > 1 && (pad & 1)) {
 		bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 8);
 		oddbyte = 0;
+		pad -= 1;
 	}
 #endif
-#endif

 	/*
 	 * Push out padding.
@@ -773,22 +774,17 @@
 	}
 #endif

-#ifdef SMC91CXX_NO_BYTE_WRITE
 	/*
 	 * Push out control byte and unused packet byte.  The control byte
-	 * is 0, meaning the packet is even lengthed and no special
+	 * denotes whether this is an odd or even length packet, and that no special
 	 * CRC handling is necessary.
 	 */
 #if BYTE_ORDER == LITTLE_ENDIAN
 	bus_space_write_2(bst, bsh, DATA_REG_W,
-	    oddbyte | (pad ? (CTLB_ODD << 8) : 0));
+	    oddbyte | ((length & 1) ? (CTLB_ODD << 8) : 0));
 #else
 	bus_space_write_2(bst, bsh, DATA_REG_W,
-	    (oddbyte << 8) | (pad ? CTLB_ODD : 0));
-#endif
-#else
-	if (pad)
-		bus_space_write_1(bst, bsh, DATA_REG_B, 0);
+	    (oddbyte << 8) | ((length & 1) ? CTLB_ODD : 0));
 #endif

 	/*
@@ -892,10 +888,7 @@
 			panic("smc91cxx_copy_tx_frame: p != lim");
 #endif
 	}
-#ifndef SMC91CXX_NO_BYTE_WRITE
-	if (leftover)
-		bus_space_write_1(bst, bsh, DATA_REG_B, dbuf);
-#endif
+
 	return dbuf;
 }

>Release-Note:

>Audit-Trail:
From: Robert Sprowson <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/47788
Date: Thu, 30 May 2013 08:49:50 +0100

 Minor update to patch following testing by Chuck on a big endian 68000, two
 places in the send packet function were reversing the data bytes.

 --- smc91cxx_new.c	Thu Apr 25 06:49:32 2013
 +++ smc91cxx_len.c	Thu May 30 08:45:20 2013
 @@ -652,13 +652,12 @@
  	 */
  	for (len = 0; m != NULL; m = m->m_next)
  		len += m->m_len;
 -	pad = (len & 1);

  	/*
  	 * We drop packets that are too large.  Perhaps we should
  	 * truncate them instead?
  	 */
 -	if ((len + pad) > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
 +	if (len > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
  		printf("%s: large packet discarded\n", device_xname(sc->sc_dev));
  		ifp->if_oerrors++;
  		IFQ_DEQUEUE(&ifp->if_snd, m);
 @@ -666,6 +665,7 @@
  		goto readcheck;
  	}

 +	pad = 0;
  #ifdef SMC91CXX_SW_PAD
  	/*
  	 * Not using hardware padding; pad to ETHER_MIN_LEN.
 @@ -745,24 +745,17 @@
  	IFQ_DEQUEUE(&ifp->if_snd, m);

  	/*
 -	 * Push the packet out to the card.
 +	 * Push the packet out to the card.  The copying function only does whole
 +	 * words and returns the straggling byte (if any).
  	 */
  	oddbyte = smc91cxx_copy_tx_frame(sc, m);

  #ifdef SMC91CXX_SW_PAD
 -#ifdef SMC91CXX_NO_BYTE_WRITE
 -#if BYTE_ORDER == LITTLE_ENDIAN
  	if (pad > 1 && (pad & 1)) {
  		bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 0);
  		oddbyte = 0;
 +		pad -= 1;
  	}
 -#else
 -	if (pad > 1 && (pad & 1)) {
 -		bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 8);
 -		oddbyte = 0;
 -	}
 -#endif
 -#endif

  	/*
  	 * Push out padding.
 @@ -773,23 +766,13 @@
  	}
  #endif

 -#ifdef SMC91CXX_NO_BYTE_WRITE
  	/*
  	 * Push out control byte and unused packet byte.  The control byte
 -	 * is 0, meaning the packet is even lengthed and no special
 +	 * denotes whether this is an odd or even length packet, and that no
 special
  	 * CRC handling is necessary.
  	 */
 -#if BYTE_ORDER == LITTLE_ENDIAN
  	bus_space_write_2(bst, bsh, DATA_REG_W,
 -	    oddbyte | (pad ? (CTLB_ODD << 8) : 0));
 -#else
 -	bus_space_write_2(bst, bsh, DATA_REG_W,
 -	    (oddbyte << 8) | (pad ? CTLB_ODD : 0));
 -#endif
 -#else
 -	if (pad)
 -		bus_space_write_1(bst, bsh, DATA_REG_B, 0);
 -#endif
 +	    oddbyte | ((length & 1) ? (CTLB_ODD << 8) : 0));

  	/*
  	 * Enable transmit interrupts and let the chip go.  Set a watchdog
 @@ -892,10 +875,7 @@
  			panic("smc91cxx_copy_tx_frame: p != lim");
  #endif
  	}
 -#ifndef SMC91CXX_NO_BYTE_WRITE
 -	if (leftover)
 -		bus_space_write_1(bst, bsh, DATA_REG_B, dbuf);
 -#endif
 +
  	return dbuf;
  }

From: "Chuck Silvers" <chs@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47788 CVS commit: src/sys/dev/ic
Date: Sat, 7 Sep 2013 18:55:29 +0000

 Module Name:	src
 Committed By:	chs
 Date:		Sat Sep  7 18:55:29 UTC 2013

 Modified Files:
 	src/sys/dev/ic: smc91cxx.c

 Log Message:
 apply changes from Robert Sprowson in PR 47765 and PR 47788:
  - make sure we wait long enough after resetting the chip.
  - add the necessary delay after changing the FIFO pointer.
  - add a missing interrupt ACK.
  - fix padding of short and odd-length packets.

 and a few more changes from me:
  - do 2-byte writes in most places even if SMC91CXX_NO_BYTE_WRITE
    is not defined.  the only ones still conditionalized are
    writing to the interrupt ack/mask registers.
  - the only big-endian front-end of this driver (on mac68k) uses
    a bus_space that does all the byte-swapping in that layer,
    so remove the explicit byte-swapping in the MI part.

 tested on mac68k.


 To generate a diff of this commit:
 cvs rdiff -u -r1.84 -r1.85 src/sys/dev/ic/smc91cxx.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 01:08:23 +0000
State-Changed-Why:
Is this fixed?


From: Robert Sprowson <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/47788
Date: Mon, 07 Oct 2013 08:51:34 +0100

 On 07 Oct, <dholland@NetBSD.org> wrote:
 > Synopsis: Incorrect packet length transmitted for odd-lengths with smc91cxx driver

 > State-Changed-From-To: open->feedback
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Mon, 07 Oct 2013 01:08:23 +0000
 > State-Changed-Why:
 > Is this fixed?

 Yes - Chuck committed this in revision 1.86. PR can be closed.

State-Changed-From-To: feedback->closed
State-Changed-By: martin@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 09:54:10 +0000
State-Changed-Why:
confirmed fixed, thanks for the PR


>Unformatted:

Home	PR Database Search