Rework re_start and re_txeof to only check the producer/consumer ring

positions when deciding how much work to do, and to adjust rl_tx_free with
atomic operations; split the flag that indicates whether we're using
timer based interrupts or not out into a separate field so it can be
changed from interrupt context without needing a lock; take the kernel
lock when calling re_init and re_start from interrupt context; add an
interrupt barrier in re_stop; and finally mark the interrupt handler
as mpsafe.

started by Jim Smith a while ago, mostly finished up at n2k15
tested by dlg@, chris@ and Dimitris Papastamos on various hardware
ok dlg@

3 files changed, 56 insertions, 59 deletions

diff --git a/sys/dev/ic/re.c b/sys/dev/ic/re.c
index 63c09d4ef15..282e6715b35 100644
--- a/sys/dev/ic/re.c
+++ b/sys/dev/ic/re.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: re.c,v 1.187 2015/11/25 03:09:58 dlg Exp $	*/
+/*	$OpenBSD: re.c,v 1.188 2015/12/28 05:49:15 jmatthew Exp $	*/
 /*	$FreeBSD: if_re.c,v 1.31 2004/09/04 07:54:05 ru Exp $	*/
 /*
  * Copyright (c) 1997, 1998-2003
@@ -120,6 +120,7 @@
 #include <sys/device.h>
 #include <sys/timeout.h>
 #include <sys/socket.h>
+#include <sys/atomic.h>
 
 #include <machine/bus.h>
 
@@ -151,7 +152,7 @@ int redebug = 0;
 
 static inline void re_set_bufaddr(struct rl_desc *, bus_addr_t);
 
-int	re_encap(struct rl_softc *, struct mbuf *, int *);
+int	re_encap(struct rl_softc *, struct mbuf *, struct rl_txq *, int *);
 
 int	re_newbuf(struct rl_softc *);
 int	re_rx_list_init(struct rl_softc *);
@@ -1448,18 +1449,14 @@ re_txeof(struct rl_softc *sc)
 	struct ifnet	*ifp;
 	struct rl_txq	*txq;
 	uint32_t	txstat;
-	int		idx, descidx, tx = 0;
+	int		idx, descidx, tx_free, freed = 0;
 
 	ifp = &sc->sc_arpcom.ac_if;
 
-	for (idx = sc->rl_ldata.rl_txq_considx;; idx = RL_NEXT_TXQ(sc, idx)) {
+	for (idx = sc->rl_ldata.rl_txq_considx;
+	    idx != sc->rl_ldata.rl_txq_prodidx; idx = RL_NEXT_TXQ(sc, idx)) {
 		txq = &sc->rl_ldata.rl_txq[idx];
 
-		if (txq->txq_mbuf == NULL) {
-			KASSERT(idx == sc->rl_ldata.rl_txq_prodidx);
-			break;
-		}
-
 		descidx = txq->txq_descidx;
 		RL_TXDESCSYNC(sc, descidx,
 		    BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
@@ -1470,9 +1467,7 @@ re_txeof(struct rl_softc *sc)
 		if (txstat & RL_TDESC_CMD_OWN)
 			break;
 
-		tx = 1;
-		sc->rl_ldata.rl_tx_free += txq->txq_nsegs;
-		KASSERT(sc->rl_ldata.rl_tx_free <= sc->rl_ldata.rl_tx_desc_cnt);
+		freed += txq->txq_nsegs;
 		bus_dmamap_sync(sc->sc_dmat, txq->txq_dmamap,
 		    0, txq->txq_dmamap->dm_mapsize, BUS_DMASYNC_POSTWRITE);
 		bus_dmamap_unload(sc->sc_dmat, txq->txq_dmamap);
@@ -1487,9 +1482,13 @@ re_txeof(struct rl_softc *sc)
 			ifp->if_opackets++;
 	}
 
-	sc->rl_ldata.rl_txq_considx = idx;
+	if (freed == 0)
+		return (0);
 
-	ifq_clr_oactive(&ifp->if_snd);
+	tx_free = atomic_add_int_nv(&sc->rl_ldata.rl_tx_free, freed);
+	KASSERT(tx_free <= sc->rl_ldata.rl_tx_desc_cnt);
+
+	sc->rl_ldata.rl_txq_considx = idx;
 
 	/*
 	 * Some chips will ignore a second TX request issued while an
@@ -1498,12 +1497,14 @@ re_txeof(struct rl_softc *sc)
 	 * to restart the channel here to flush them out. This only
 	 * seems to be required with the PCIe devices.
 	 */
-	if (sc->rl_ldata.rl_tx_free < sc->rl_ldata.rl_tx_desc_cnt)
+	if (tx_free < sc->rl_ldata.rl_tx_desc_cnt)
 		CSR_WRITE_1(sc, sc->rl_txstart, RL_TXSTART_START);
-	else
+	else {
+		ifq_clr_oactive(&ifp->if_snd);
 		ifp->if_timer = 0;
+	}
 
-	return (tx);
+	return (1);
 }
 
 void
@@ -1566,13 +1567,15 @@ re_intr(void *arg)
 		}
 
 		if (status & RL_ISR_SYSTEM_ERR) {
+			KERNEL_LOCK();
 			re_init(ifp);
+			KERNEL_UNLOCK();
 			claimed = 1;
 		}
 	}
 
 	if (sc->rl_imtype == RL_IMTYPE_SIM) {
-		if ((sc->rl_flags & RL_FLAG_TIMERINTR)) {
+		if (sc->rl_timerintr) {
 			if ((tx | rx) == 0) {
 				/*
 				 * Nothing needs to be processed, fallback
@@ -1599,7 +1602,11 @@ re_intr(void *arg)
 		}
 	}
 
-	re_start(ifp);
+	if (!IFQ_IS_EMPTY(&ifp->if_snd)) {
+		KERNEL_LOCK();
+		re_start(ifp);
+		KERNEL_UNLOCK();
+	}
 
 	CSR_WRITE_2(sc, RL_IMR, sc->rl_intrs);
 
@@ -1607,7 +1614,7 @@ re_intr(void *arg)
 }
 
 int
-re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
+re_encap(struct rl_softc *sc, struct mbuf *m, struct rl_txq *txq, int *used)
 {
 	bus_dmamap_t	map;
 	struct mbuf	*mp, mh;
@@ -1616,7 +1623,6 @@ re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
 	struct ip	*ip;
 	struct rl_desc	*d;
 	u_int32_t	cmdstat, vlanctl = 0, csum_flags = 0;
-	struct rl_txq	*txq;
 
 	/*
 	 * Set up checksum offload. Note: checksum offload bits must
@@ -1669,7 +1675,6 @@ re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
 		}
 	}
 
-	txq = &sc->rl_ldata.rl_txq[*idx];
 	map = txq->txq_dmamap;
 
 	error = bus_dmamap_load_mbuf(sc->sc_dmat, map, m,
@@ -1710,7 +1715,7 @@ re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
 		nsegs++;
 	}
 
-	if (sc->rl_ldata.rl_tx_free - nsegs <= 1) {
+	if (sc->rl_ldata.rl_tx_free - (*used + nsegs) <= 1) {
 		error = EFBIG;
 		goto fail_unload;
 	}
@@ -1812,10 +1817,9 @@ re_encap(struct rl_softc *sc, struct mbuf *m, int *idx)
 	txq->txq_descidx = lastidx;
 	txq->txq_nsegs = nsegs;
 
-	sc->rl_ldata.rl_tx_free -= nsegs;
 	sc->rl_ldata.rl_tx_nextfree = curidx;
 
-	*idx = RL_NEXT_TXQ(sc, *idx);
+	*used += nsegs;
 
 	return (0);
 
@@ -1834,30 +1838,25 @@ re_start(struct ifnet *ifp)
 {
 	struct rl_softc	*sc = ifp->if_softc;
 	struct mbuf	*m;
-	int		idx, queued = 0, error;
+	int		idx, used = 0, txq_free, error;
 
 	if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(&ifp->if_snd))
 		return;
 	if ((sc->rl_flags & RL_FLAG_LINK) == 0)
 		return;
-	if (IFQ_IS_EMPTY(&ifp->if_snd))
-		return;
 
+	txq_free = sc->rl_ldata.rl_txq_considx;
 	idx = sc->rl_ldata.rl_txq_prodidx;
+	if (idx >= txq_free)
+		txq_free += RL_TX_QLEN;
+	txq_free -= idx;
 
-	for (;;) {
+	while (txq_free > 1) {
 		m = ifq_deq_begin(&ifp->if_snd);
 		if (m == NULL)
 			break;
 
-		if (sc->rl_ldata.rl_txq[idx].txq_mbuf != NULL) {
-			KASSERT(idx == sc->rl_ldata.rl_txq_considx);
-			ifq_deq_rollback(&ifp->if_snd, m);
-			ifq_set_oactive(&ifp->if_snd);
-			break;
-		}
-
-		error = re_encap(sc, m, &idx);
+		error = re_encap(sc, m, &sc->rl_ldata.rl_txq[idx], &used);
 		if (error != 0 && error != ENOBUFS) {
 			ifq_deq_rollback(&ifp->if_snd, m);
 			ifq_set_oactive(&ifp->if_snd);
@@ -1869,31 +1868,25 @@ re_start(struct ifnet *ifp)
 			continue;
 		}
 
-		/* now we are committed to transmit the packet */
 		ifq_deq_commit(&ifp->if_snd, m);
-		queued++;
 
 #if NBPFILTER > 0
-		/*
-		 * If there's a BPF listener, bounce a copy of this frame
-		 * to him.
-		 */
 		if (ifp->if_bpf)
 			bpf_mtap_ether(ifp->if_bpf, m, BPF_DIRECTION_OUT);
 #endif
+		idx = RL_NEXT_TXQ(sc, idx);
+		txq_free--;
 	}
 
-	if (queued == 0)
+	if (used == 0)
 		return;
-
+	
+	ifp->if_timer = 5;
 	sc->rl_ldata.rl_txq_prodidx = idx;
+	atomic_sub_int(&sc->rl_ldata.rl_tx_free, used);
+	KASSERT(sc->rl_ldata.rl_tx_free >= 0);
 
 	CSR_WRITE_1(sc, sc->rl_txstart, RL_TXSTART_START);
-
-	/*
-	 * Set a timeout in case the chip goes out to lunch.
-	 */
-	ifp->if_timer = 5;
 }
 
 int
@@ -2142,14 +2135,13 @@ re_stop(struct ifnet *ifp)
 	sc = ifp->if_softc;
 
 	ifp->if_timer = 0;
-	sc->rl_flags &= ~(RL_FLAG_LINK|RL_FLAG_TIMERINTR);
+	sc->rl_flags &= ~RL_FLAG_LINK;
+	sc->rl_timerintr = 0;
 
 	timeout_del(&sc->timer_handle);
 	ifp->if_flags &= ~IFF_RUNNING;
 	ifq_clr_oactive(&ifp->if_snd);
 
-	mii_down(&sc->sc_mii);
-
 	/*
 	 * Disable accepting frames to put RX MAC into idle state.
 	 * Otherwise it's possible to get frames while stop command
@@ -2196,6 +2188,10 @@ re_stop(struct ifnet *ifp)
 		sc->rl_head = sc->rl_tail = NULL;
 	}
 
+	intr_barrier(sc->sc_ih);
+
+	mii_down(&sc->sc_mii);
+
 	/* Free the TX list buffers. */
 	for (i = 0; i < RL_TX_QLEN; i++) {
 		if (sc->rl_ldata.rl_txq[i].txq_mbuf != NULL) {
@@ -2276,7 +2272,7 @@ re_setup_sim_im(struct rl_softc *sc)
 		CSR_WRITE_4(sc, RL_TIMERINT_8169, ticks);
 	}
 	CSR_WRITE_4(sc, RL_TIMERCNT, 1); /* reload */
-	sc->rl_flags |= RL_FLAG_TIMERINTR;
+	sc->rl_timerintr = 1;
 }
 
 void
@@ -2286,7 +2282,7 @@ re_disable_sim_im(struct rl_softc *sc)
 		CSR_WRITE_4(sc, RL_TIMERINT, 0);
 	else 
 		CSR_WRITE_4(sc, RL_TIMERINT_8169, 0);
-	sc->rl_flags &= ~RL_FLAG_TIMERINTR;
+	sc->rl_timerintr = 0;
 }
 
 void
diff --git a/sys/dev/ic/rtl81x9reg.h b/sys/dev/ic/rtl81x9reg.h
index f527c09817c..a021ae9c302 100644
--- a/sys/dev/ic/rtl81x9reg.h
+++ b/sys/dev/ic/rtl81x9reg.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rtl81x9reg.h,v 1.96 2015/11/02 00:08:50 dlg Exp $	*/
+/*	$OpenBSD: rtl81x9reg.h,v 1.97 2015/12/28 05:49:15 jmatthew Exp $	*/
 
 /*
  * Copyright (c) 1997, 1998
@@ -918,6 +918,7 @@ struct rl_softc {
 #define	RL_IMTYPE_NONE		0
 #define	RL_IMTYPE_SIM		1	/* simulated */
 #define	RL_IMTYPE_HW		2	/* hardware based */
+	int			rl_timerintr;
 };
 
 /*
diff --git a/sys/dev/pci/if_re_pci.c b/sys/dev/pci/if_re_pci.c
index 7fdd8584f09..8a29f45cc93 100644
--- a/sys/dev/pci/if_re_pci.c
+++ b/sys/dev/pci/if_re_pci.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_re_pci.c,v 1.49 2015/11/24 17:11:39 mpi Exp $	*/
+/*	$OpenBSD: if_re_pci.c,v 1.50 2015/12/28 05:49:15 jmatthew Exp $	*/
 
 /*
  * Copyright (c) 2005 Peter Valchev <pvalchev@openbsd.org>
@@ -156,8 +156,8 @@ re_pci_attach(struct device *parent, struct device *self, void *aux)
 		return;
 	}
 	intrstr = pci_intr_string(pc, ih);
-	psc->sc_ih = pci_intr_establish(pc, ih, IPL_NET, re_intr, sc,
-	    sc->sc_dev.dv_xname);
+	psc->sc_ih = pci_intr_establish(pc, ih, IPL_NET | IPL_MPSAFE, re_intr,
+	    sc, sc->sc_dev.dv_xname);
 	if (psc->sc_ih == NULL) {
 		printf(": couldn't establish interrupt");
 		if (intrstr != NULL)