When you call MiniportInitialize() for an 802.11 driver, it will

at some point result in a status event being triggered (it should
be a link down event: the Microsoft driver design guide says you
should generate one when the NIC is initialized). Some drivers
generate the event during MiniportInitialize(), such that by the
time MiniportInitialize() completes, the NIC is ready to go. But
some drivers, in particular the ones for Atheros wireless NICs,
don't generate the event until after a device interrupt occurs
at some point after MiniportInitialize() has completed.

The gotcha is that you have to wait until the link status event
occurs one way or the other before you try to fiddle with any
settings (ssid, channel, etc...). For the drivers that set the
event sycnhronously this isn't a problem, but for the others
we have to pause after calling ndis_init_nic() and wait for the event
to arrive before continuing. Failing to wait can cause big trouble:
on my SMP system, calling ndis_setstate_80211() after ndis_init_nic()
completes, but _before_ the link event arrives, will lock up or
reset the system.

What we do now is check to see if a link event arrived while
ndis_init_nic() was running, and if it didn't we msleep() until
it does.

Along the way, I discovered a few other problems:

- Defered procedure calls run at PASSIVE_LEVEL, not DISPATCH_LEVEL.
  ntoskrnl_run_dpc() has been fixed accordingly. (I read the documentation
  wrong.)

- Similarly, the NDIS interrupt handler, which is essentially a
  DPC, also doesn't need to run at DISPATCH_LEVEL. ndis_intrtask()
  has been fixed accordingly.

- MiniportQueryInformation() and MiniportSetInformation() run at
  DISPATCH_LEVEL, and each request must complete before another
  can be submitted. ndis_get_info() and ndis_set_info() have been
  fixed accordingly.

- Turned the sleep lock that guards the NDIS thread job list into
  a spin lock. We never do anything with this lock held except manage
  the job list (no other locks are held), so it's safe to do this,
  and it's possible that ndis_sched() and ndis_unsched() can be
  called from DISPATCH_LEVEL, so using a sleep lock here is
  semantically incorrect. Also updated subr_witness.c to add the
  lock to the order list.

4 files changed, 120 insertions, 55 deletions

diff --git a/sys/compat/ndis/kern_ndis.c b/sys/compat/ndis/kern_ndis.c
index b71835a..3054396 100644
--- a/sys/compat/ndis/kern_ndis.c
+++ b/sys/compat/ndis/kern_ndis.c
@@ -239,19 +239,19 @@ ndis_runq(arg)
 
 		/* Look for any jobs on the work queue. */
 
-		mtx_lock(&ndis_thr_mtx);
+		mtx_lock_spin(&ndis_thr_mtx);
 		p->np_state = NDIS_PSTATE_RUNNING;
 		while(STAILQ_FIRST(p->np_q) != NULL) {
 			r = STAILQ_FIRST(p->np_q);
 			STAILQ_REMOVE_HEAD(p->np_q, link);
-			mtx_unlock(&ndis_thr_mtx);
+			mtx_unlock_spin(&ndis_thr_mtx);
 
 			/* Do the work. */
 
 			if (r->nr_func != NULL)
 				(*r->nr_func)(r->nr_arg);
 
-			mtx_lock(&ndis_thr_mtx);
+			mtx_lock_spin(&ndis_thr_mtx);
 			STAILQ_INSERT_HEAD(&ndis_free, r, link);
 
 			/* Check for a shutdown request */
@@ -260,7 +260,7 @@ ndis_runq(arg)
 				die = r;
 		}
 		p->np_state = NDIS_PSTATE_SLEEPING;
-		mtx_unlock(&ndis_thr_mtx);
+		mtx_unlock_spin(&ndis_thr_mtx);
 
 		/* Bail if we were told to shut down. */
 
@@ -282,10 +282,8 @@ ndis_create_kthreads()
 	struct ndis_req		*r;
 	int			i, error = 0;
 
-	mtx_init(&ndis_thr_mtx, "NDIS thread lock",
-	   MTX_NDIS_LOCK, MTX_DEF);
-	mtx_init(&ndis_req_mtx, "NDIS request lock",
-	   MTX_NDIS_LOCK, MTX_DEF);
+	mtx_init(&ndis_thr_mtx, "NDIS thread lock", NULL, MTX_SPIN);
+	mtx_init(&ndis_req_mtx, "NDIS request lock", MTX_NDIS_LOCK, MTX_DEF);
 
 	STAILQ_INIT(&ndis_ttodo);
 	STAILQ_INIT(&ndis_itodo);
@@ -368,14 +366,14 @@ ndis_stop_thread(t)
 
 	/* Create and post a special 'exit' job. */
 
-	mtx_lock(&ndis_thr_mtx);
+	mtx_lock_spin(&ndis_thr_mtx);
 	r = STAILQ_FIRST(&ndis_free);
 	STAILQ_REMOVE_HEAD(&ndis_free, link);
 	r->nr_func = NULL;
 	r->nr_arg = NULL;
 	r->nr_exit = TRUE;
 	STAILQ_INSERT_TAIL(q, r, link);
-	mtx_unlock(&ndis_thr_mtx);
+	mtx_unlock_spin(&ndis_thr_mtx);
 
 	ndis_thresume(p);
 
@@ -385,12 +383,12 @@ ndis_stop_thread(t)
 
 	/* Now empty the job list. */
 
-	mtx_lock(&ndis_thr_mtx);
+	mtx_lock_spin(&ndis_thr_mtx);
 	while ((r = STAILQ_FIRST(q)) != NULL) {
 		STAILQ_REMOVE_HEAD(q, link);
 		STAILQ_INSERT_HEAD(&ndis_free, r, link);
 	}
-	mtx_unlock(&ndis_thr_mtx);
+	mtx_unlock_spin(&ndis_thr_mtx);
 
 	return;
 }
@@ -406,10 +404,10 @@ ndis_enlarge_thrqueue(cnt)
 		r = malloc(sizeof(struct ndis_req), M_DEVBUF, M_WAITOK);
 		if (r == NULL)
 			return(ENOMEM);
-		mtx_lock(&ndis_thr_mtx);
+		mtx_lock_spin(&ndis_thr_mtx);
 		STAILQ_INSERT_HEAD(&ndis_free, r, link);
 		ndis_jobs++;
-		mtx_unlock(&ndis_thr_mtx);
+		mtx_unlock_spin(&ndis_thr_mtx);
 	}
 
 	return(0);
@@ -423,15 +421,15 @@ ndis_shrink_thrqueue(cnt)
 	int			i;
 
 	for (i = 0; i < cnt; i++) {
-		mtx_lock(&ndis_thr_mtx);
+		mtx_lock_spin(&ndis_thr_mtx);
 		r = STAILQ_FIRST(&ndis_free);
 		if (r == NULL) {
-			mtx_unlock(&ndis_thr_mtx);
+			mtx_unlock_spin(&ndis_thr_mtx);
 			return(ENOMEM);
 		}
 		STAILQ_REMOVE_HEAD(&ndis_free, link);
 		ndis_jobs--;
-		mtx_unlock(&ndis_thr_mtx);
+		mtx_unlock_spin(&ndis_thr_mtx);
 		free(r, M_DEVBUF);
 	}
 
@@ -456,17 +454,17 @@ ndis_unsched(func, arg, t)
 		p = ndis_iproc.np_p;
 	}
 
-	mtx_lock(&ndis_thr_mtx);
+	mtx_lock_spin(&ndis_thr_mtx);
 	STAILQ_FOREACH(r, q, link) {
 		if (r->nr_func == func && r->nr_arg == arg) {
 			STAILQ_REMOVE(q, r, ndis_req, link);
 			STAILQ_INSERT_HEAD(&ndis_free, r, link);
-			mtx_unlock(&ndis_thr_mtx);
+			mtx_unlock_spin(&ndis_thr_mtx);
 			return(0);
 		}
 	}
 
-	mtx_unlock(&ndis_thr_mtx);
+	mtx_unlock_spin(&ndis_thr_mtx);
 
 	return(ENOENT);
 }
@@ -490,20 +488,20 @@ ndis_sched(func, arg, t)
 		p = ndis_iproc.np_p;
 	}
 
-	mtx_lock(&ndis_thr_mtx);
+	mtx_lock_spin(&ndis_thr_mtx);
 	/*
 	 * Check to see if an instance of this job is already
 	 * pending. If so, don't bother queuing it again.
 	 */
 	STAILQ_FOREACH(r, q, link) {
 		if (r->nr_func == func && r->nr_arg == arg) {
-			mtx_unlock(&ndis_thr_mtx);
+			mtx_unlock_spin(&ndis_thr_mtx);
 			return(0);
 		}
 	}
 	r = STAILQ_FIRST(&ndis_free);
 	if (r == NULL) {
-		mtx_unlock(&ndis_thr_mtx);
+		mtx_unlock_spin(&ndis_thr_mtx);
 		return(EAGAIN);
 	}
 	STAILQ_REMOVE_HEAD(&ndis_free, link);
@@ -515,7 +513,7 @@ ndis_sched(func, arg, t)
 		s = ndis_tproc.np_state;
 	else
 		s = ndis_iproc.np_state;
-	mtx_unlock(&ndis_thr_mtx);
+	mtx_unlock_spin(&ndis_thr_mtx);
 
 	/*
 	 * Post the job, but only if the thread is actually blocked
@@ -635,7 +633,7 @@ ndis_resetdone_func(adapter, status, addressingreset)
 
 	if (ifp->if_flags & IFF_DEBUG)
 		device_printf (sc->ndis_dev, "reset done...\n");
-	wakeup(ifp);
+	wakeup(sc);
 	return;
 }
 
@@ -1153,29 +1151,46 @@ ndis_set_info(arg, oid, buf, buflen)
 	int			error;
 	uint8_t			irql;
 
+	/*
+	 * According to the NDIS spec, MiniportQueryInformation()
+	 * and MiniportSetInformation() requests are handled serially:
+	 * once one request has been issued, we must wait for it to
+ 	 * finish before allowing another request to proceed.
+	 */
+
+	mtx_lock(&ndis_req_mtx);
 	sc = arg;
-	NDIS_LOCK(sc);
+
+	if (sc->ndis_block->nmb_pendingreq != NULL)
+		panic("ndis_set_info() called while other request pending");
+	else
+		sc->ndis_block->nmb_pendingreq = (ndis_request *)sc;
+
 	setfunc = sc->ndis_chars->nmc_setinfo_func;
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
-	NDIS_UNLOCK(sc);
 
-	if (adapter == NULL || setfunc == NULL)
+	if (adapter == NULL || setfunc == NULL) {
+		mtx_unlock(&ndis_req_mtx);
 		return(ENXIO);
+	}
 
-	KeAcquireSpinLock(&sc->ndis_block->nmb_lock, &irql);
+	irql = KeRaiseIrql(DISPATCH_LEVEL);
 	rval = MSCALL6(setfunc, adapter, oid, buf, *buflen,
 	    &byteswritten, &bytesneeded);
-	KeReleaseSpinLock(&sc->ndis_block->nmb_lock, irql);
+	KeLowerIrql(irql);
 
 	if (rval == NDIS_STATUS_PENDING) {
-		mtx_lock(&ndis_req_mtx);
 		error = msleep(&sc->ndis_block->nmb_setstat,
 		    &ndis_req_mtx,
-		    curthread->td_priority|PDROP,
+		    curthread->td_priority,
 		    "ndisset", 5 * hz);
 		rval = sc->ndis_block->nmb_setstat;
 	}
 
+	sc->ndis_block->nmb_pendingreq = NULL;
+
+	mtx_unlock(&ndis_req_mtx);
+
 	if (byteswritten)
 		*buflen = byteswritten;
 	if (bytesneeded)
@@ -1211,7 +1226,7 @@ ndis_send_packets(arg, packets, cnt)
 	__stdcall ndis_senddone_func		senddonefunc;
 	int			i;
 	ndis_packet		*p;
-	uint8_t			irql;
+	int			irql;
 
 	sc = arg;
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
@@ -1219,9 +1234,12 @@ ndis_send_packets(arg, packets, cnt)
 		return(ENXIO);
 	sendfunc = sc->ndis_chars->nmc_sendmulti_func;
 	senddonefunc = sc->ndis_block->nmb_senddone_func;
-	irql = KeRaiseIrql(DISPATCH_LEVEL);
+
+	if (!(sc->ndis_block->nmb_flags & NDIS_ATTRIBUTE_DESERIALIZE))
+		KeAcquireSpinLock(&sc->ndis_block->nmb_lock, &irql);
 	MSCALL3(sendfunc, adapter, packets, cnt);
-	KeLowerIrql(irql);
+	if (!(sc->ndis_block->nmb_flags & NDIS_ATTRIBUTE_DESERIALIZE))
+		KeReleaseSpinLock(&sc->ndis_block->nmb_lock, irql);
 
 	for (i = 0; i < cnt; i++) {
 		p = packets[i];
@@ -1258,10 +1276,12 @@ ndis_send_packet(arg, packet)
 	sendfunc = sc->ndis_chars->nmc_sendsingle_func;
 	senddonefunc = sc->ndis_block->nmb_senddone_func;
 
-	irql = KeRaiseIrql(DISPATCH_LEVEL);
+	if (!(sc->ndis_block->nmb_flags & NDIS_ATTRIBUTE_DESERIALIZE))
+		KeAcquireSpinLock(&sc->ndis_block->nmb_lock, &irql);
 	status = MSCALL3(sendfunc, adapter, packet,
 	    packet->np_private.npp_flags);
-	KeLowerIrql(irql);
+	if (!(sc->ndis_block->nmb_flags & NDIS_ATTRIBUTE_DESERIALIZE))
+		KeReleaseSpinLock(&sc->ndis_block->nmb_lock, irql);
 
 	if (status == NDIS_STATUS_PENDING)
 		return(0);
@@ -1341,10 +1361,10 @@ ndis_reset_nic(arg)
 
 	sc = arg;
 	ifp = &sc->arpcom.ac_if;
-	NDIS_LOCK(sc);
+
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
 	resetfunc = sc->ndis_chars->nmc_reset_func;
-	NDIS_UNLOCK(sc);
+
 	if (adapter == NULL || resetfunc == NULL)
 		return(EIO);
 
@@ -1519,10 +1539,12 @@ ndis_isr(arg, ourintr, callhandler)
 	sc = arg;
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
 	isrfunc = sc->ndis_chars->nmc_isr_func;
+
 	if (adapter == NULL || isrfunc == NULL)
 		return(ENXIO);
 
 	MSCALL3(isrfunc, &accepted, &queue, adapter);
+
 	*ourintr = accepted;
 	*callhandler = queue;
 
@@ -1541,10 +1563,10 @@ ndis_intrhand(arg)
 		return(EINVAL);
 
 	sc = arg;
-	NDIS_LOCK(sc);
+
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
 	intrfunc = sc->ndis_chars->nmc_interrupt_func;
-	NDIS_UNLOCK(sc);
+
 	if (adapter == NULL || intrfunc == NULL)
 		return(EINVAL);
 
@@ -1567,32 +1589,42 @@ ndis_get_info(arg, oid, buf, buflen)
 	uint32_t		byteswritten = 0, bytesneeded = 0;
 	int			error;
 	uint8_t			irql;
+	
+	mtx_lock(&ndis_req_mtx);
 
 	sc = arg;
-	NDIS_LOCK(sc);
+	if (sc->ndis_block->nmb_pendingreq != NULL)
+		panic("ndis_get_info() called while other request pending");
+	else
+		sc->ndis_block->nmb_pendingreq = (ndis_request *)sc;
+
 	queryfunc = sc->ndis_chars->nmc_queryinfo_func;
 	adapter = sc->ndis_block->nmb_miniportadapterctx;
-	NDIS_UNLOCK(sc);
 
-	if (adapter == NULL || queryfunc == NULL)
+	if (adapter == NULL || queryfunc == NULL) {
+		mtx_unlock(&ndis_req_mtx);
 		return(ENXIO);
+	}
 
-	KeAcquireSpinLock(&sc->ndis_block->nmb_lock, &irql);
+	irql = KeRaiseIrql(DISPATCH_LEVEL);
 	rval = MSCALL6(queryfunc, adapter, oid, buf, *buflen,
 	    &byteswritten, &bytesneeded);
-	KeReleaseSpinLock(&sc->ndis_block->nmb_lock, irql);
+	KeLowerIrql(irql);
 
 	/* Wait for requests that block. */
 
 	if (rval == NDIS_STATUS_PENDING) {
-		mtx_lock(&ndis_req_mtx);
 		error = msleep(&sc->ndis_block->nmb_getstat,
 		    &ndis_req_mtx,
-		    curthread->td_priority|PDROP,
+		    curthread->td_priority,
 		    "ndisget", 5 * hz);
 		rval = sc->ndis_block->nmb_getstat;
 	}
 
+	sc->ndis_block->nmb_pendingreq = NULL;
+
+	mtx_unlock(&ndis_req_mtx);
+
 	if (byteswritten)
 		*buflen = byteswritten;
 	if (bytesneeded)
@@ -1655,6 +1687,7 @@ NdisAddDevice(drv, pdo)
 	block->nmb_querydone_func = kernndis_functbl[3].ipt_wrap;
 	block->nmb_resetdone_func = kernndis_functbl[4].ipt_wrap;
 	block->nmb_sendrsrc_func = kernndis_functbl[5].ipt_wrap;
+	block->nmb_pendingreq = NULL;
 
 	ndis_enlarge_thrqueue(8);
 
diff --git a/sys/compat/ndis/subr_ntoskrnl.c b/sys/compat/ndis/subr_ntoskrnl.c
index bcd1e70..c7f197a 100644
--- a/sys/compat/ndis/subr_ntoskrnl.c
+++ b/sys/compat/ndis/subr_ntoskrnl.c
@@ -2423,14 +2423,11 @@ ntoskrnl_run_dpc(arg)
 {
 	__stdcall kdpc_func	dpcfunc;
 	kdpc			*dpc;
-	uint8_t			irql;
 
 	dpc = arg;
 	dpcfunc = dpc->k_deferedfunc;
-	irql = KeRaiseIrql(DISPATCH_LEVEL);
 	MSCALL4(dpcfunc, dpc, dpc->k_deferredctx,
 	    dpc->k_sysarg1, dpc->k_sysarg2);
-	KeLowerIrql(irql);
 
 	return;
 }
diff --git a/sys/dev/if_ndis/if_ndis.c b/sys/dev/if_ndis/if_ndis.c
index e59b8b6..fbf9170 100644
--- a/sys/dev/if_ndis/if_ndis.c
+++ b/sys/dev/if_ndis/if_ndis.c
@@ -44,6 +44,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/socket.h>
 #include <sys/queue.h>
 #include <sys/module.h>
+#include <sys/proc.h>
 #if __FreeBSD_version < 502113
 #include <sys/sysctl.h>
 #endif
@@ -1080,9 +1081,14 @@ ndis_linksts(adapter, status, sbuf, slen)
 	uint32_t		slen;
 {
 	ndis_miniport_block	*block;
+	struct ndis_softc	*sc;
 
 	block = adapter;
+	sc = device_get_softc(block->nmb_physdeviceobj->do_devext);
+
+	NDIS_LOCK(sc);
 	block->nmb_getstat = status;
+	NDIS_UNLOCK(sc);
 
 	return;
 }
@@ -1113,6 +1119,10 @@ ndis_linksts_done(adapter)
 	case NDIS_STATUS_MEDIA_DISCONNECT:
 		if (sc->ndis_link)
 			ndis_sched(ndis_ticktask, sc, NDIS_TASKQUEUE);
+		else {
+			if (sc->ndis_80211)
+				wakeup(&block->nmb_getstat);
+		}
 		break;
 	default:
 		break;
@@ -1128,14 +1138,11 @@ ndis_intrtask(arg)
 {
 	struct ndis_softc	*sc;
 	struct ifnet		*ifp;
-	uint8_t			irql;
 
 	sc = arg;
 	ifp = &sc->arpcom.ac_if;
 
-	irql = KeRaiseIrql(DISPATCH_LEVEL);
 	ndis_intrhand(sc);
-	KeLowerIrql(irql);
 
 	ndis_enable_intr(sc);
 
@@ -1170,7 +1177,7 @@ ndis_intr(arg)
 	KeReleaseSpinLock(&intr->ni_dpccountlock, irql);
 
 	if ((is_our_intr || call_isr))
-		ndis_sched(ndis_intrtask, ifp->if_softc, NDIS_SWI);
+		ndis_sched(ndis_intrtask, sc, NDIS_SWI);
 
 	return;
 }
@@ -1459,9 +1466,36 @@ ndis_init(xsc)
 	 * Cancel pending I/O and free all RX/TX buffers.
 	 */
 	ndis_stop(sc);
+
+	NDIS_LOCK(sc);
+	sc->ndis_block->nmb_getstat = 0;
 	if (ndis_init_nic(sc))
 		return;
 
+	/*
+	 * 802.11 NDIS drivers are supposed to generate a link
+	 * down event right when you initialize them. You wait
+	 * until this event occurs before trying to futz with
+	 * the device. Some drivers will actually set the event
+	 * during the course of MiniportInitialize(), which means
+	 * by the time it completes, the device is ready for us
+	 * to interact with it. But some drivers don't signal the
+	 * event until after MiniportInitialize() (they probably
+	 * need to wait for a device interrupt to occur first).
+	 * We have to be careful to handle both cases. After we
+	 * call ndis_init_nic(), we have to see if a status event
+	 * was triggered. If it wasn't, we have to wait for it
+	 * to occur before we can proceed.
+	 */
+	if (sc->ndis_80211 & !sc->ndis_block->nmb_getstat) {
+		error = msleep(&sc->ndis_block->nmb_getstat,
+		    &sc->ndis_mtx, curthread->td_priority,
+		    "ndiswait", 5 * hz);
+	}
+
+	sc->ndis_block->nmb_getstat = 0;
+	NDIS_UNLOCK(sc);
+
 	/* Init our MAC address */
 
 	/* Program the packet filter */
diff --git a/sys/kern/subr_witness.c b/sys/kern/subr_witness.c
index bc3ed26..064d95c 100644
--- a/sys/kern/subr_witness.c
+++ b/sys/kern/subr_witness.c
@@ -372,6 +372,7 @@ static struct witness_order_list_entry order_lists[] = {
 #endif
 #if defined(__i386__) || defined(__amd64__)
 	{ "pcicfg", &lock_class_mtx_spin },
+	{ "NDIS thread lock", &lock_class_mtx_spin },
 #endif
 	{ NULL, NULL },
 	{ NULL, NULL }