Subject: Re: if_sk.c fixes from FreeBSD - do these look OK?
To: None <tech-net@netbsd.org>
From: Christos Zoulas <christos@astron.com>
List: tech-net
Date: 11/19/2005 18:03:42
In article <20051119161524.GA4400@boogers.sf.ca.us>,
Jeff Rizzo  <riz@netbsd.org> wrote:
>-=-=-=-=-=-
>-=-=-=-=-=-
>
>I was seeing occasional "sk0: watchdog timeout" messages under 3.99.11
>on my amd64 system ( ASUS A8V motherboard ), so I went looking around
>a bit.  Pulling in these two revisions from FreeBSD (well, the 
>appropriate part of them, anyway) seems to have cleared up the
>timeouts:
>
>----------------------------
>revision 1.92
>date: 2004/11/17 21:35:22;  author: jmg;  state: Exp;  lines: +4 -4
>only clear the IFF_OACTIVE flag when we have a chance of being able to
>queue a packet to the hardware... instead of when the hardware queue is
>empty..
>
>don't initalize cur_tx now that it doesn't need to be...
>
>Pointed out by: bde
>----------------------------
>revision 1.90
>date: 2004/11/15 19:37:21;  author: jmg;  state: Exp;  lines: +22 -10
>fix the missing lock in sk_jfree (verified w/ an assert)
>also fix up handling and proding of the tx, _OACTIVE is now handled
>better...
>
>Submitted by:   Peter Edwards (sk_jfree)
>Obtained from:  OpenBSD and/or NetBSD (tx prod)
>----------------------------
>
>
>I looked through OpenBSD's code as well, and they pulled these two fixes
>in back in January.
>
>Does anyone mind if I commit?
>
>Also, looking through FreeBSD's if_sk.c, there seem to be a lot of fixes
>we haven't grabbed yet;  I'd like to start testing some of them out, but
>this is the only sk(4) interface I have... is there anyone who'd be
>willing to help me test stuff out?
>
>Here's the sk-related bit of my dmesg:
>
>skc0 at pci0 dev 10 function 0: ioapic0 pin 17 (irq 10)
>skc0: Marvell Yukon Lite Gigabit Ethernet rev. (0x9)
>sk0 at skc0 port A: Ethernet address 00:13:d4:65:d9:b6
>makphy0 at sk0 phy 0: Marvell 88E1011 Gigabit PHY, rev. 5
>makphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-FD
>X, auto
>
>
>Thanks,
>+j
>
>
>-=-=-=-=-=-
>
>Index: if_sk.c
>===================================================================
>RCS file: /cvsroot/src/sys/dev/pci/if_sk.c,v
>retrieving revision 1.16
>diff -u -r1.16 if_sk.c
>--- if_sk.c	11 Sep 2005 23:49:39 -0000	1.16
>+++ if_sk.c	19 Nov 2005 16:06:30 -0000
>@@ -1759,11 +1759,13 @@
> 		return;
> 
> 	/* Transmit */
>-	sc_if->sk_cdata.sk_tx_prod = idx;
>-	CSR_WRITE_4(sc, sc_if->sk_tx_bmu, SK_TXBMU_TX_START);
>+	if (idx != sc_if->sk_cdata.sk_tx_prod) {
>+		sc_if->sk_cdata.sk_tx_prod = idx;
>+		CSR_WRITE_4(sc, sc_if->sk_tx_bmu, SK_TXBMU_TX_START);
> 
>-	/* Set a timeout in case the chip goes out to lunch. */
>-	ifp->if_timer = 5;
>+		/* Set a timeout in case the chip goes out to lunch. */
>+		ifp->if_timer = 5;
>+	}
> }
> 
> 
>@@ -1888,7 +1890,7 @@
> sk_txeof(struct sk_if_softc *sc_if)
> {
> 	struct sk_softc		*sc = sc_if->sk_softc;
>-	struct sk_tx_desc	*cur_tx = NULL;
>+	struct sk_tx_desc	*cur_tx;
> 	struct ifnet		*ifp = &sc_if->sk_ethercom.ec_if;
> 	u_int32_t		idx;
> 	struct sk_txmap_entry	*entry;
>@@ -1937,10 +1939,10 @@
> 	else /* nudge chip to keep tx ring moving */
> 		CSR_WRITE_4(sc, sc_if->sk_tx_bmu, SK_TXBMU_TX_START);
> 
>-	sc_if->sk_cdata.sk_tx_cons = idx;
>-
>-	if (cur_tx != NULL)
>+	if (sc_if->sk_cdata.sk_tx_cnt < SK_TX_RING_CNT - 2)
> 		ifp->if_flags &= ~IFF_OACTIVE;
>+
>+	sc_if->sk_cdata.sk_tx_cons = idx;
> }
> 
> void

Looks ok to me.

christos