Add locking to snc(4) so it is MPSAFE:

- Add a mutex to the softc to protect the softc and device hardware.
- Use a private timer routine to drive the transmit watchdog timer instead
  of using if_watchdog/if_timer.
- If if_alloc() fails during attach, fail the attach with an error rather
  than panic'ing.
- Clear RUNNING and OACTIVE only in sncstop().
- Don't mess with IFF_UP.
- Don't leak 'struct ifnet' on detach.
- Setup interrupt handler after ether_ifattach().
- Call ether_ifdetach() rather than if_detach() in the pccard detach
  routine.

Tested by:	no one despite repeated requests

5 files changed, 130 insertions, 76 deletions

diff --git a/sys/dev/snc/dp83932.c b/sys/dev/snc/dp83932.c
index 669c293..8108a33 100644
--- a/sys/dev/snc/dp83932.c
+++ b/sys/dev/snc/dp83932.c
@@ -63,6 +63,7 @@
 #include "opt_inet.h"
 
 #include <sys/param.h>
+#include <sys/kernel.h>
 #include <sys/systm.h>
 #include <sys/sockio.h>
 #include <sys/mbuf.h>
@@ -85,11 +86,13 @@
 #include <dev/snc/dp83932reg.h>
 #include <dev/snc/dp83932var.h>
 
-static void	sncwatchdog(struct ifnet *);
+static void	sncwatchdog(void *);
 static void	sncinit(void *);
+static void	sncinit_locked(struct snc_softc *);
 static int	sncstop(struct snc_softc *sc);
 static int	sncioctl(struct ifnet *ifp, u_long cmd, caddr_t data);
 static void	sncstart(struct ifnet *ifp);
+static void	sncstart_locked(struct ifnet *ifp);
 static void	sncreset(struct snc_softc *sc);
 
 static void	caminitialise(struct snc_softc *);
@@ -138,7 +141,7 @@ void	snc_mediastatus(struct ifnet *, struct ifmediareq *);
 int sncdebug = 0;
 
 
-void
+int
 sncconfig(sc, media, nmedia, defmedia, myea)
 	struct snc_softc *sc;
 	int *media, nmedia, defmedia;
@@ -154,9 +157,10 @@ sncconfig(sc, media, nmedia, defmedia, myea)
 #endif
 
 	ifp = sc->sc_ifp = if_alloc(IFT_ETHER);
-	if (ifp == NULL)
-		panic("%s: can not if_alloc()\n",
-		    device_get_nameunit(sc->sc_dev));
+	if (ifp == NULL) {
+		device_printf(sc->sc_dev, "can not if_alloc()\n");
+		return (ENOMEM);
+	}
 
 #ifdef SNCDEBUG
 	device_printf(sc->sc_dev,
@@ -170,12 +174,10 @@ sncconfig(sc, media, nmedia, defmedia, myea)
 	    device_get_unit(sc->sc_dev));
 	ifp->if_ioctl = sncioctl;
 	ifp->if_start = sncstart;
-	ifp->if_flags =
-	    IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST | IFF_NEEDSGIANT;
-	ifp->if_watchdog = sncwatchdog;
+	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
         ifp->if_init = sncinit;
         ifp->if_mtu = ETHERMTU;
-        ifp->if_snd.ifq_maxlen = IFQ_MAXLEN;
+	IFQ_SET_MAXLEN(&ifp->if_snd, IFQ_MAXLEN);
 
 	/* Initialize media goo. */
 	ifmedia_init(&sc->sc_media, 0, snc_mediachange,
@@ -190,14 +192,17 @@ sncconfig(sc, media, nmedia, defmedia, myea)
 	}
 
 	ether_ifattach(ifp, myea);
+	return (0);
 }
 
 void
 sncshutdown(arg)
 	void *arg;
 {
+	struct snc_softc *sc = arg;
 
-	sncstop((struct snc_softc *)arg);
+	SNC_ASSERT_LOCKED(sc);
+	sncstop(sc);
 }
 
 /*
@@ -208,10 +213,15 @@ snc_mediachange(ifp)
 	struct ifnet *ifp;
 {
 	struct snc_softc *sc = ifp->if_softc;
+	int error;
 
+	SNC_LOCK(sc);
 	if (sc->sc_mediachange)
-		return ((*sc->sc_mediachange)(sc));
-	return (EINVAL);
+		error = (*sc->sc_mediachange)(sc);
+	else
+		error = EINVAL;
+	SNC_UNLOCK(sc);
+	return (error);
 }
 
 /*
@@ -224,14 +234,17 @@ snc_mediastatus(ifp, ifmr)
 {
 	struct snc_softc *sc = ifp->if_softc;
 
+	SNC_LOCK(sc);
 	if (sc->sc_enabled == 0) {
 		ifmr->ifm_active = IFM_ETHER | IFM_NONE;
 		ifmr->ifm_status = 0;
+		SNC_UNLOCK(sc);
 		return;
 	}
 
 	if (sc->sc_mediastatus)
 		(*sc->sc_mediastatus)(sc, ifmr);
+	SNC_UNLOCK(sc);
 }
 
 
@@ -243,12 +256,12 @@ sncioctl(ifp, cmd, data)
 {
 	struct ifreq *ifr;
 	struct snc_softc *sc = ifp->if_softc;
-	int	s = splimp(), err = 0;
-	int	temp;
+	int	err = 0;
 
 	switch (cmd) {
 
 	case SIOCSIFFLAGS:
+		SNC_LOCK(sc);
 		if ((ifp->if_flags & IFF_UP) == 0 &&
 		    (ifp->if_drv_flags & IFF_DRV_RUNNING) != 0) {
 			/*
@@ -256,7 +269,6 @@ sncioctl(ifp, cmd, data)
 			 * then stop it.
 			 */
 			sncstop(sc);
-			ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
 			snc_disable(sc);
 		} else if ((ifp->if_flags & IFF_UP) != 0 &&
 		    (ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) {
@@ -266,28 +278,28 @@ sncioctl(ifp, cmd, data)
 			 */
 			if ((err = snc_enable(sc)) != 0)
 				break;
-			sncinit(sc);
+			sncinit_locked(sc);
 		} else if (sc->sc_enabled) {
 			/*
 			 * reset the interface to pick up any other changes
 			 * in flags
 			 */
-			temp = ifp->if_flags & IFF_UP;
 			sncreset(sc);
-			ifp->if_flags |= temp;
-			sncstart(ifp);
+			sncstart_locked(ifp);
 		}
+		SNC_UNLOCK(sc);
 		break;
 
 	case SIOCADDMULTI:
 	case SIOCDELMULTI:
+		SNC_LOCK(sc);
 		if (sc->sc_enabled == 0) {
 			err = EIO;
+			SNC_UNLOCK(sc);
 			break;
 		}
-		temp = ifp->if_flags & IFF_UP;
 		sncreset(sc);
-		ifp->if_flags |= temp;
+		SNC_UNLOCK(sc);
 		err = 0;
 		break;
 	case SIOCGIFMEDIA:
@@ -299,7 +311,6 @@ sncioctl(ifp, cmd, data)
 		err = ether_ioctl(ifp, cmd, data);
 		break;
 	}
-	splx(s);
 	return (err);
 }
 
@@ -311,6 +322,17 @@ sncstart(ifp)
 	struct ifnet *ifp;
 {
 	struct snc_softc	*sc = ifp->if_softc;
+
+	SNC_LOCK(sc);
+	sncstart_locked(ifp);
+	SNC_UNLOCK(sc);
+}
+
+static void
+sncstart_locked(ifp)
+	struct ifnet *ifp;
+{
+	struct snc_softc	*sc = ifp->if_softc;
 	struct mbuf	*m;
 	int		mtd_next;
 
@@ -373,7 +395,7 @@ sncreset(sc)
 	struct snc_softc *sc;
 {
 	sncstop(sc);
-	sncinit(sc);
+	sncinit_locked(sc);
 }
 
 static void
@@ -381,15 +403,21 @@ sncinit(xsc)
 	void *xsc;
 {
 	struct snc_softc *sc = xsc;
+
+	SNC_LOCK(sc);
+	sncinit_locked(sc);
+	SNC_UNLOCK(sc);
+}
+
+static void
+sncinit_locked(struct snc_softc *sc)
+{
 	u_long	s_rcr;
-	int	s;
 
 	if (sc->sc_ifp->if_drv_flags & IFF_DRV_RUNNING)
 		/* already running */
 		return;
 
-	s = splimp();
-
 	NIC_PUT(sc, SNCR_CR, CR_RST);	/* DCR only accessable in reset mode! */
 
 	/* config it */
@@ -438,8 +466,8 @@ sncinit(xsc)
 	/* flag interface as "running" */
 	sc->sc_ifp->if_drv_flags |= IFF_DRV_RUNNING;
 	sc->sc_ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
+	callout_reset(&sc->sc_timer, hz, sncwatchdog, sc);
 
-	splx(s);
 	return;
 }
 
@@ -453,7 +481,8 @@ sncstop(sc)
 	struct snc_softc *sc;
 {
 	struct mtd *mtd;
-	int	s = splimp();
+
+	SNC_ASSERT_LOCKED(sc);
 
 	/* stick chip in reset */
 	NIC_PUT(sc, SNCR_CR, CR_RST);
@@ -469,11 +498,10 @@ sncstop(sc)
 		if (++sc->mtd_hw == NTDA) sc->mtd_hw = 0;
 	}
 
-	sc->sc_ifp->if_timer = 0;
-	sc->sc_ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
-	sc->sc_ifp->if_flags &= ~IFF_UP;
+	callout_stop(&sc->sc_timer);
+	sc->sc_tx_timeout = 0;
+	sc->sc_ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE);
 
-	splx(s);
 	return (0);
 }
 
@@ -483,30 +511,30 @@ sncstop(sc)
  * will be handled by higher level protocol timeouts.
  */
 static void
-sncwatchdog(ifp)
-	struct ifnet *ifp;
+sncwatchdog(void *arg)
 {
-	struct snc_softc *sc = ifp->if_softc;
+	struct snc_softc *sc = arg;
 	struct mtd *mtd;
-	int	temp;
 
-	if (sc->mtd_hw != sc->mtd_free) {
-		/* something still pending for transmit */
-		mtd = &sc->mtda[sc->mtd_hw];
-		if (SRO(sc, mtd->mtd_vtxp, TXP_STATUS) == 0)
-			log(LOG_ERR, "%s: Tx - timeout\n",
-			    device_get_nameunit(sc->sc_dev));
-		else
-			log(LOG_ERR, "%s: Tx - lost interrupt\n",
-			    device_get_nameunit(sc->sc_dev));
-		temp = ifp->if_flags & IFF_UP;
-		sncreset(sc);
-		ifp->if_flags |= temp;
+	SNC_ASSERT_LOCKED(sc);
+	if (sc->sc_tx_timeout && --sc->sc_tx_timeout == 0) {
+		if (sc->mtd_hw != sc->mtd_free) {
+			/* something still pending for transmit */
+			mtd = &sc->mtda[sc->mtd_hw];
+			if (SRO(sc, mtd->mtd_vtxp, TXP_STATUS) == 0)
+				log(LOG_ERR, "%s: Tx - timeout\n",
+				    device_get_nameunit(sc->sc_dev));
+			else
+				log(LOG_ERR, "%s: Tx - lost interrupt\n",
+				    device_get_nameunit(sc->sc_dev));
+			sncreset(sc);
+		}
 	}
+	callout_reset(&sc->sc_timer, hz, sncwatchdog, sc);
 }
 
 /*
- * stuff packet into sonic (at splnet)
+ * stuff packet into sonic
  */
 static u_int
 sonicput(sc, m0, mtd_next)
@@ -587,7 +615,9 @@ sonicput(sc, m0, mtd_next)
 	wbflush();
 	NIC_PUT(sc, SNCR_CR, CR_TXP);
 	wbflush();
-	sc->sc_ifp->if_timer = 5;	/* 5 seconds to watch for failing to transmit */
+
+	/* 5 seconds to watch for failing to transmit */
+	sc->sc_tx_timeout = 5;
 
 	return (totlen);
 }
@@ -822,6 +852,7 @@ sncintr(arg)
 	if (sc->sc_enabled == 0)
 		return;
 
+	SNC_LOCK(sc);
 	while ((isr = (NIC_GET(sc, SNCR_ISR) & ISR_ALL)) != 0) {
 		/* scrub the interrupts that we are going to service */
 		NIC_PUT(sc, SNCR_ISR, isr);
@@ -872,8 +903,9 @@ sncintr(arg)
 				sc->sc_mptally++;
 #endif
 		}
-		sncstart(sc->sc_ifp);
+		sncstart_locked(sc->sc_ifp);
 	}
+	SNC_UNLOCK(sc);
 	return;
 }
 
@@ -920,6 +952,7 @@ sonictxint(sc)
 		}
 #endif /* SNCDEBUG */
 
+		sc->sc_tx_timeout = 0;
 		ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
 
 		if (mtd->mtd_mbuf != 0) {
@@ -1088,7 +1121,9 @@ sonic_read(sc, pkt, len)
 #endif /* SNCDEBUG */
 
 	/* Pass the packet up. */
+	SNC_UNLOCK(sc);
 	(*ifp->if_input)(ifp, m);
+	SNC_LOCK(sc);
 	return (1);
 }
 
diff --git a/sys/dev/snc/dp83932var.h b/sys/dev/snc/dp83932var.h
index 1ac2ee7..2ed9b9d 100644
--- a/sys/dev/snc/dp83932var.h
+++ b/sys/dev/snc/dp83932var.h
@@ -200,8 +200,15 @@ typedef struct snc_softc {
 
 	void	*sc_sh;		/* shutdownhook cookie */
 	int	gone;
+	struct mtx	sc_lock;
+	struct callout	sc_timer;
+	int		sc_tx_timeout;
 } snc_softc_t;
 
+#define	SNC_LOCK(sc)		mtx_lock(&(sc)->sc_lock)
+#define	SNC_UNLOCK(sc)		mtx_unlock(&(sc)->sc_lock)
+#define	SNC_ASSERT_LOCKED(sc)	mtx_assert(&(sc)->sc_lock, MA_OWNED)
+
 /*
  * Accessing SONIC data structures and registers as 32 bit values
  * makes code endianess independent.  The SONIC is however always in
@@ -274,6 +281,6 @@ typedef struct snc_softc {
 #define	CDA_ENABLE	64	/* mask enabling CAM entries */
 #define	CDA_SIZE(sc)	((4*16 + 1) * ((sc->bitmode) ? 4 : 2))
 
-void	sncconfig(struct snc_softc *, int *, int, int, u_int8_t *);
+int	sncconfig(struct snc_softc *, int *, int, int, u_int8_t *);
 void	sncintr(void *);
 void	sncshutdown(void *);
diff --git a/sys/dev/snc/if_snc.c b/sys/dev/snc/if_snc.c
index 7cbdd80..e89e1c7 100644
--- a/sys/dev/snc/if_snc.c
+++ b/sys/dev/snc/if_snc.c
@@ -159,6 +159,10 @@ snc_release_resources(dev)
 				     sc->irq_rid, sc->irq);
 		sc->irq = 0;
 	}
+	if (sc->sc_ifp) {
+		if_free(sc->sc_ifp);
+		sc->sc_ifp = 0;
+	}
 }
 
 /****************************************************************
@@ -189,6 +193,7 @@ snc_attach(dev)
 {
 	struct snc_softc *sc = device_get_softc(dev);
 	u_int8_t myea[ETHER_ADDR_LEN];
+	int error;
 
 	if (snc_nec16_register_irq(sc, rman_get_start(sc->irq)) == 0 || 
 	    snc_nec16_register_mem(sc, rman_get_start(sc->iomem)) == 0) {
@@ -220,7 +225,25 @@ snc_attach(dev)
 		return(ENOENT);
 	}
 
-	sncconfig(sc, NULL, 0, 0, myea);
+	mtx_init(&sc->sc_lock, device_get_nameunit(dev), MTX_NETWORK_LOCK,
+	    MTX_DEF);
+	callout_init_mtx(&sc->sc_timer, &sc->sc_lock, 0);
+	error = sncconfig(sc, NULL, 0, 0, myea);
+	if (error) {
+		snc_release_resources(dev);
+		mtx_destroy(&sc->sc_lock);
+		return (error);
+	}
+
+	error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET | INTR_MPSAFE,
+			       NULL, sncintr, sc, &sc->irq_handle);
+	if (error) {
+		printf("snc_isa_attach: bus_setup_intr() failed\n");
+		ether_ifdetach(sc->sc_ifp);		
+		snc_release_resources(dev);
+		mtx_destroy(&sc->sc_lock);
+		return (error);
+	}
 
 	return 0;
 }
@@ -233,5 +256,9 @@ void
 snc_shutdown(dev)
 	device_t dev;
 {
-	sncshutdown(device_get_softc(dev));
+	struct snc_softc *sc = device_get_softc(dev);
+
+	SNC_LOCK(sc);
+	sncshutdown(sc);
+	SNC_UNLOCK(sc);
 }
diff --git a/sys/dev/snc/if_snc_cbus.c b/sys/dev/snc/if_snc_cbus.c
index f280de3..e859b7a 100644
--- a/sys/dev/snc/if_snc_cbus.c
+++ b/sys/dev/snc/if_snc_cbus.c
@@ -150,7 +150,7 @@ snc_isa_probe(dev)
 			bus_set_resource(dev, SYS_RES_IOPORT, rid,
 					 port, SNEC_NREGS);
 			res = bus_alloc_resource(dev, SYS_RES_IOPORT, &rid,
-						 0, ~0, SNEC_NREGS,
+						 0ul, ~0ul, SNEC_NREGS,
 						 0 /* !RF_ACTIVE */);
 			if (res) break;
 		}
@@ -181,7 +181,6 @@ snc_isa_attach(dev)
 	device_t dev;
 {
 	struct snc_softc *sc = device_get_softc(dev);
-	int error;
 	
 	bzero(sc, sizeof(struct snc_softc));
 
@@ -189,14 +188,6 @@ snc_isa_attach(dev)
 	snc_alloc_memory(dev, 0);
 	snc_alloc_irq(dev, 0, 0);
 		
-	error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET,
-			       NULL, sncintr, sc, &sc->irq_handle);
-	if (error) {
-		printf("snc_isa_attach: bus_setup_intr() failed\n");
-		snc_release_resources(dev);
-		return (error);
-	}	       
-
 	/* This interface is always enabled. */
 	sc->sc_enabled = 1;
 
diff --git a/sys/dev/snc/if_snc_pccard.c b/sys/dev/snc/if_snc_pccard.c
index 5a0a2d5..05babd1 100644
--- a/sys/dev/snc/if_snc_pccard.c
+++ b/sys/dev/snc/if_snc_pccard.c
@@ -46,11 +46,11 @@ __FBSDID("$FreeBSD$");
 #include <sys/bus.h>
 #include <machine/bus.h>
 
+#include <net/ethernet.h>
 #include <net/if.h>
 #include <net/if_arp.h>
 #include <net/if_media.h>
 
-
 #include <dev/snc/dp83932var.h>
 #include <dev/snc/if_sncvar.h>
 #include <dev/snc/if_sncreg.h>
@@ -94,12 +94,15 @@ snc_pccard_detach(device_t dev)
 		device_printf(dev, "already unloaded\n");
 		return (0);
 	}
+	SNC_LOCK(sc);
 	sncshutdown(sc);
-	ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
-	if_detach(ifp);
+	SNC_UNLOCK(sc);
+	callout_drain(&sc->sc_timer);
+	ether_ifdetach(ifp);
 	sc->gone = 1;
 	bus_teardown_intr(dev, sc->irq, sc->irq_handle);
 	snc_release_resources(dev);
+	mtx_destroy(&sc->sc_lock);
 	return (0);
 }
 
@@ -126,7 +129,6 @@ static int
 snc_pccard_attach(device_t dev)
 {
 	struct snc_softc *sc = device_get_softc(dev);
-	int error;
 	
 	bzero(sc, sizeof(struct snc_softc));
 
@@ -134,14 +136,6 @@ snc_pccard_attach(device_t dev)
 	snc_alloc_memory(dev, 0);
 	snc_alloc_irq(dev, 0, 0);
 		
-	error = bus_setup_intr(dev, sc->irq, INTR_TYPE_NET,
-			       NULL, sncintr, sc, &sc->irq_handle);
-	if (error) {
-		printf("snc_isa_attach: bus_setup_intr() failed\n");
-		snc_release_resources(dev);
-		return (error);
-	}	       
-
 	/* This interface is always enabled. */
 	sc->sc_enabled = 1;