Various fixups to locking:

- Remove a lot of superfluous locking during attach.  There is no need
  to lock access to the driver until some other thread has a way of getting
  to it.  For ethernet drivers the other ways include registering an
  interrupt handler via bus_setup_intr(), calling ether_ifattach() to hook
  into the network stack, and kicking off a callout-driven timer via
  callout_reset().
- Use callout_* rather than timeout/untimeout.
- Break out of xl_rxeof() if IFF_DRV_RUNNING is clear after ifp->if_input
  returns to handle the case where the interface was stopped while we were
  passing a packet up the stack.  Don't call xl_rxeof() in xl_rxeof_task()
  unless IFF_DRV_RUNNING is set.  With these fixes in place, any
  outstanding task will gracefully terminate as soon as it gets a chance to
  run after the interface has been stopped via xl_stop().  As a result,
  taskqueue_drain() is no longer required in xl_stop().  The task is still
  drained in detach() however to make sure that detach() can safely destroy
  the driver mutex at the end of the function.
- Lock the driver lock in the ifmedia callouts and don't lock across
  ifmedia_ioctl() in xl_ioctl().

Note: glebius came up with most of (3) as well independently.  I took a
rather roundabout way of arriving at the same conclusion.

MFC after:	3 days

1 files changed, 32 insertions, 48 deletions

diff --git a/sys/pci/if_xl.c b/sys/pci/if_xl.c
index 78d100d..b4f1f86 100644
--- a/sys/pci/if_xl.c
+++ b/sys/pci/if_xl.c
@@ -458,8 +458,6 @@ xl_mii_readreg(struct xl_softc *sc, struct xl_mii_frame *frame)
 {
 	int			i, ack;
 
-	/*XL_LOCK_ASSERT(sc);*/
-
 	/* Set up frame for RX. */
 	frame->mii_stdelim = XL_MII_STARTDELIM;
 	frame->mii_opcode = XL_MII_READOP;
@@ -528,8 +526,6 @@ static int
 xl_mii_writereg(struct xl_softc *sc, struct xl_mii_frame *frame)
 {
 
-	/*XL_LOCK_ASSERT(sc);*/
-
 	/* Set up frame for TX. */
 	frame->mii_stdelim = XL_MII_STARTDELIM;
 	frame->mii_opcode = XL_MII_WRITEOP;
@@ -617,8 +613,6 @@ xl_miibus_statchg(device_t dev)
 	sc = device_get_softc(dev);
 	mii = device_get_softc(sc->xl_miibus);
 
-	/*XL_LOCK_ASSERT(sc);*/
-
 	xl_setcfg(sc);
 
 	/* Set ASIC's duplex mode to match the PHY. */
@@ -651,8 +645,6 @@ xl_miibus_mediainit(device_t dev)
 	mii = device_get_softc(sc->xl_miibus);
 	ifm = &mii->mii_media;
 
-	/*XL_LOCK_ASSERT(sc);*/
-
 	if (sc->xl_media & (XL_MEDIAOPT_AUI | XL_MEDIAOPT_10FL)) {
 		/*
 		 * Check for a 10baseFL board in disguise.
@@ -716,8 +708,6 @@ xl_read_eeprom(struct xl_softc *sc, caddr_t dest, int off, int cnt, int swap)
 	int			err = 0, i;
 	u_int16_t		word = 0, *ptr;
 
-	XL_LOCK_ASSERT(sc);
-
 #define EEPROM_5BIT_OFFSET(A) ((((A) << 2) & 0x7F00) | ((A) & 0x003F))
 #define EEPROM_8BIT_OFFSET(A) ((A) & 0x003F)
 	/*
@@ -904,7 +894,7 @@ xl_setmode(struct xl_softc *sc, int media)
 	u_int16_t		mediastat;
 	char			*pmsg = "", *dmsg = "";
 
-	/*XL_LOCK_ASSERT(sc);*/
+	XL_LOCK_ASSERT(sc);
 
 	XL_SEL_WIN(4);
 	mediastat = CSR_READ_2(sc, XL_W4_MEDIA_STATUS);
@@ -1091,8 +1081,6 @@ static void
 xl_mediacheck(struct xl_softc *sc)
 {
 
-	XL_LOCK_ASSERT(sc);
-
 	/*
 	 * If some of the media options bits are set, assume they are
 	 * correct. If not, try to figure it out down below.
@@ -1364,10 +1352,10 @@ xl_attach(device_t dev)
 	ifp->if_softc = sc;
 	if_initname(ifp, device_get_name(dev), device_get_unit(dev));
 
-	XL_LOCK(sc);
-
 	/* Reset the adapter. */
+	XL_LOCK(sc);
 	xl_reset(sc);
+	XL_UNLOCK(sc);
 
 	/*
 	 * Get station address from the EEPROM.
@@ -1375,14 +1363,11 @@ xl_attach(device_t dev)
 	if (xl_read_eeprom(sc, (caddr_t)&eaddr, XL_EE_OEM_ADR0, 3, 1)) {
 		device_printf(dev, "failed to read station address\n");
 		error = ENXIO;
-		XL_UNLOCK(sc);
 		goto fail;
 	}
 
-	XL_UNLOCK(sc);
-
 	sc->xl_unit = unit;
-	callout_handle_init(&sc->xl_stat_ch);
+	callout_init_mtx(&sc->xl_stat_callout, &sc->xl_mtx, 0);
 	TASK_INIT(&sc->xl_task, 0, xl_rxeof_task, sc);
 
 	/*
@@ -1473,8 +1458,6 @@ xl_attach(device_t dev)
 	if (error)
 		goto fail;
 
-	XL_LOCK(sc);
-
 	/*
 	 * Figure out the card type. 3c905B adapters have the
 	 * 'supportsNoTxLength' bit set in the capabilities
@@ -1533,9 +1516,6 @@ xl_attach(device_t dev)
 
 	xl_mediacheck(sc);
 
-	/* XXX Downcalls to ifmedia, miibus about to happen. */
-	XL_UNLOCK(sc);
-
 	if (sc->xl_media & XL_MEDIAOPT_MII ||
 	    sc->xl_media & XL_MEDIAOPT_BTX ||
 	    sc->xl_media & XL_MEDIAOPT_BT4) {
@@ -1556,12 +1536,8 @@ xl_attach(device_t dev)
 	 * a 10/100 card of some kind, we need to force the transceiver
 	 * type to something sane.
 	 */
-	if (sc->xl_xcvr == XL_XCVR_AUTO) {
-		/* XXX Direct hardware access needs lock coverage. */
-		XL_LOCK(sc);
+	if (sc->xl_xcvr == XL_XCVR_AUTO)
 		xl_choose_xcvr(sc, bootverbose);
-		XL_UNLOCK(sc);
-	}
 
 	/*
 	 * Do ifmedia setup.
@@ -1610,7 +1586,6 @@ xl_attach(device_t dev)
 		ifmedia_add(&sc->ifmedia, IFM_ETHER|IFM_100_FX, 0, NULL);
 	}
 
-	/* XXX: Unlocked, leaf will take lock. */
 	media = IFM_ETHER|IFM_100_TX|IFM_FDX;
 	xl_choose_media(sc, &media);
 
@@ -1618,7 +1593,6 @@ xl_attach(device_t dev)
 		ifmedia_set(&sc->ifmedia, media);
 
 done:
-	/* XXX: Unlocked hardware access, narrow race. */
 	if (sc->xl_flags & XL_FLAG_NO_XCVR_PWR) {
 		XL_SEL_WIN(0);
 		CSR_WRITE_2(sc, XL_W0_MFG_ID, XL_NO_XCVR_PWR_MAGICBITS);
@@ -1648,7 +1622,8 @@ fail:
 /*
  * Choose a default media.
  * XXX This is a leaf function only called by xl_attach() and
- *     acquires/releases the non-recursible driver mutex.
+ *     acquires/releases the non-recursible driver mutex to
+ *     satisfy lock assertions.
  */
 static void
 xl_choose_media(struct xl_softc *sc, int *media)
@@ -1715,7 +1690,6 @@ xl_detach(device_t dev)
 	ifp = sc->xl_ifp;
 
 	KASSERT(mtx_initialized(&sc->xl_mtx), ("xl mutex not initialized"));
-	XL_LOCK(sc);
 
 	if (sc->xl_flags & XL_FLAG_USE_MMIO) {
 		rid = XL_PCI_LOMEM;
@@ -1727,8 +1701,12 @@ xl_detach(device_t dev)
 
 	/* These should only be active if attach succeeded */
 	if (device_is_attached(dev)) {
+		XL_LOCK(sc);
 		xl_reset(sc);
 		xl_stop(sc);
+		XL_UNLOCK(sc);
+		taskqueue_drain(taskqueue_swi, &sc->xl_task);
+		callout_drain(&sc->xl_stat_callout);
 		ether_ifdetach(ifp);
 		if_free(ifp);
 	}
@@ -1766,7 +1744,6 @@ xl_detach(device_t dev)
 		bus_dma_tag_destroy(sc->xl_ldata.xl_tx_tag);
 	}
 
-	XL_UNLOCK(sc);
 	mtx_destroy(&sc->xl_mtx);
 
 	return (0);
@@ -2076,6 +2053,14 @@ again:
 		XL_UNLOCK(sc);
 		(*ifp->if_input)(ifp, m);
 		XL_LOCK(sc);
+
+		/*
+		 * If we are running from the taskqueue, the interface
+		 * might have been stopped while we were passing the last
+		 * packet up the network stack.
+		 */
+		if (!(ifp->if_drv_flags & IFF_DRV_RUNNING))
+			return;
 	}
 
 	/*
@@ -2111,7 +2096,8 @@ xl_rxeof_task(void *arg, int pending)
 
 	NET_LOCK_GIANT();
 	XL_LOCK(sc);
-	xl_rxeof(sc);
+	if (sc->xl_ifp->if_drv_flags & IFF_DRV_RUNNING)
+		xl_rxeof(sc);
 	XL_UNLOCK(sc);
 	NET_UNLOCK_GIANT();
 }
@@ -2441,9 +2427,8 @@ xl_stats_update(void *xsc)
 {
 	struct xl_softc *sc = xsc;
 
-	XL_LOCK(sc);
+	XL_LOCK_ASSERT(sc);
 	xl_stats_update_locked(sc);
-	XL_UNLOCK(sc);
 }
 
 static void
@@ -2490,7 +2475,7 @@ xl_stats_update_locked(struct xl_softc *sc)
 	XL_SEL_WIN(7);
 
 	if (!sc->xl_stats_no_timeout)
-		sc->xl_stat_ch = timeout(xl_stats_update, sc, hz);
+		callout_reset(&sc->xl_stat_callout, hz, xl_stats_update, sc);
 }
 
 /*
@@ -3032,7 +3017,7 @@ xl_init_locked(struct xl_softc *sc)
 	ifp->if_drv_flags |= IFF_DRV_RUNNING;
 	ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
 
-	sc->xl_stat_ch = timeout(xl_stats_update, sc, hz);
+	callout_reset(&sc->xl_stat_callout, hz, xl_stats_update, sc);
 }
 
 /*
@@ -3045,7 +3030,7 @@ xl_ifmedia_upd(struct ifnet *ifp)
 	struct ifmedia		*ifm = NULL;
 	struct mii_data		*mii = NULL;
 
-	/*XL_LOCK_ASSERT(sc);*/
+	XL_LOCK(sc);
 
 	if (sc->xl_miibus != NULL)
 		mii = device_get_softc(sc->xl_miibus);
@@ -3069,11 +3054,13 @@ xl_ifmedia_upd(struct ifnet *ifp)
 	if (sc->xl_media & XL_MEDIAOPT_MII ||
 	    sc->xl_media & XL_MEDIAOPT_BTX ||
 	    sc->xl_media & XL_MEDIAOPT_BT4) {
-		xl_init(sc); /* XXX */
+		xl_init_locked(sc);
 	} else {
 		xl_setmode(sc, ifm->ifm_media);
 	}
 
+	XL_UNLOCK(sc);
+
 	return (0);
 }
 
@@ -3088,7 +3075,7 @@ xl_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr)
 	u_int16_t		status = 0;
 	struct mii_data		*mii = NULL;
 
-	/*XL_LOCK_ASSERT(sc);*/
+	XL_LOCK(sc);
 
 	if (sc->xl_miibus != NULL)
 		mii = device_get_softc(sc->xl_miibus);
@@ -3148,6 +3135,8 @@ xl_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr)
 		if_printf(ifp, "unknown XCVR type: %d\n", icfg);
 		break;
 	}
+
+	XL_UNLOCK(sc);
 }
 
 static int
@@ -3205,8 +3194,6 @@ xl_ioctl(struct ifnet *ifp, u_long command, caddr_t data)
 		break;
 	case SIOCGIFMEDIA:
 	case SIOCSIFMEDIA:
-		/* XXX Downcall from ifmedia possibly with locks held. */
-		/*XL_LOCK(sc);*/
 		if (sc->xl_miibus != NULL)
 			mii = device_get_softc(sc->xl_miibus);
 		if (mii == NULL)
@@ -3215,7 +3202,6 @@ xl_ioctl(struct ifnet *ifp, u_long command, caddr_t data)
 		else
 			error = ifmedia_ioctl(ifp, ifr,
 			    &mii->mii_media, command);
-		/*XL_UNLOCK(sc);*/
 		break;
 	case SIOCSIFCAP:
 		XL_LOCK(sc);
@@ -3286,8 +3272,6 @@ xl_stop(struct xl_softc *sc)
 	ether_poll_deregister(ifp);
 #endif /* DEVICE_POLLING */
 
-	taskqueue_drain(taskqueue_swi, &sc->xl_task);
-
 	CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_RX_DISABLE);
 	CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_STATS_DISABLE);
 	CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_INTR_ENB);
@@ -3311,7 +3295,7 @@ xl_stop(struct xl_softc *sc)
 		bus_space_write_4(sc->xl_ftag, sc->xl_fhandle, 4, 0x8000);
 
 	/* Stop the stats updater. */
-	untimeout(xl_stats_update, sc, sc->xl_stat_ch);
+	callout_stop(&sc->xl_stat_callout);
 
 	/*
 	 * Free data in the RX lists.