o change a lock model based on HAL preemption lock to a normal mtx.

Based on the HAL preemption lock there is a problem on SMP machines
  and causes a panic.
o When a device detached the current tactic to detach NDIS USB driver is
  to call SURPRISE_REMOVED event.  So it don't need to call
  ndis_halt_nic() again.  This fixes some page faults when some drivers
  work abnormal.
o it assumes now that URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER is in
  DISPATCH_LEVEL (non-sleepable) and as further work
  URB_FUNCTION_VENDOR_XXX and URB_FUNCTION_CLASS_XXX should be.

Reviewed by:	Hans Petter Selasky <hselasky_at_freebsd.org>
Tested by:	Paul B. Mahol <onemda_at_gmail.com>

sys/compat/ndis/hal_var.h

@@ -48,7 +48,6 @@ extern image_patch_table hal_functbl[];
 __BEGIN_DECLS
 extern int hal_libinit(void);
 extern int hal_libfini(void);
-extern struct mtx *hal_getdisplock(void);
 extern uint8_t KfAcquireSpinLock(kspin_lock *);
 extern void KfReleaseSpinLock(kspin_lock *, uint8_t);
 extern uint8_t KfRaiseIrql(uint8_t);

sys/compat/ndis/subr_hal.c

@@ -124,13 +124,6 @@ hal_libfini()
 	return(0);
 }
 
-struct mtx *
-hal_getdisplock()
-{
-
-	return &disp_lock[curthread->td_oncpu];
-}
-
 static void
 KeStallExecutionProcessor(usecs)
 	uint32_t		usecs;

sys/compat/ndis/subr_usbd.c

@@ -93,6 +93,8 @@ static int32_t		 usbd_power(device_object *, irp *);
 static void		 usbd_irpcancel(device_object *, irp *);
 static int32_t		 usbd_submit_urb(irp *);
 static int32_t		 usbd_urb2nt(int32_t);
+static void		 usbd_task(device_object *, void *);
+static int32_t		 usbd_taskadd(irp *, unsigned);
 static void		 usbd_xfertask(device_object *, void *);
 static void		 dummy(void);
 
@@ -118,6 +120,7 @@ static funcptr usbd_ioinvalid_wrap;
 static funcptr usbd_pnp_wrap;
 static funcptr usbd_power_wrap;
 static funcptr usbd_irpcancel_wrap;
+static funcptr usbd_task_wrap;
 static funcptr usbd_xfertask_wrap;
 
 int
@@ -144,6 +147,8 @@ usbd_libinit(void)
 	    (funcptr *)&usbd_power_wrap, 2, WINDRV_WRAP_STDCALL);
 	windrv_wrap((funcptr)usbd_irpcancel,
 	    (funcptr *)&usbd_irpcancel_wrap, 2, WINDRV_WRAP_STDCALL);
+	windrv_wrap((funcptr)usbd_task,
+	    (funcptr *)&usbd_task_wrap, 2, WINDRV_WRAP_STDCALL);
 	windrv_wrap((funcptr)usbd_xfertask,
 	    (funcptr *)&usbd_xfertask_wrap, 2, WINDRV_WRAP_STDCALL);
 
@@ -184,6 +189,7 @@ usbd_libfini(void)
 	windrv_unwrap(usbd_pnp_wrap);
 	windrv_unwrap(usbd_power_wrap);
 	windrv_unwrap(usbd_irpcancel_wrap);
+	windrv_unwrap(usbd_task_wrap);
 	windrv_unwrap(usbd_xfertask_wrap);
 
 	free(usbd_driver.dro_drivername.us_buf, M_DEVBUF);
@@ -417,7 +423,6 @@ usbd_func_getdesc(ip)
 	device_t dev = IRP_NDIS_DEV(ip);
 	struct ndis_softc *sc = device_get_softc(dev);
 	struct usbd_urb_control_descriptor_request *ctldesc;
-	uint8_t irql;
 	uint16_t actlen;
 	uint32_t len;
 	union usbd_urb *urb;
@@ -450,13 +455,13 @@ usbd_func_getdesc(ip)
 		actlen = len;
 		status = USB_ERR_NORMAL_COMPLETION;
 	} else {
-		KeRaiseIrql(DISPATCH_LEVEL, &irql);
-		status = usb2_req_get_desc(sc->ndisusb_dev, hal_getdisplock(),
+		NDISUSB_LOCK(sc);
+		status = usb2_req_get_desc(sc->ndisusb_dev, &sc->ndisusb_mtx,
 		    &actlen, ctldesc->ucd_trans_buf, 2,
 		    ctldesc->ucd_trans_buflen, ctldesc->ucd_langid,
 		    ctldesc->ucd_desctype, ctldesc->ucd_idx,
 		    NDISUSB_GETDESC_MAXRETRIES);
-		KeLowerIrql(irql);
+		NDISUSB_UNLOCK(sc);
 	}
 exit:
 	if (status != USB_ERR_NORMAL_COMPLETION) {
@@ -497,11 +502,6 @@ usbd_func_selconf(ip)
 		return usbd_usb2urb(USB_ERR_NORMAL_COMPLETION);
 	}
 
-	if (conf->bConfigurationValue > NDISUSB_CONFIG_NO)
-		device_printf(dev,
-		    "warning: config_no is larger than default (%#x/%#x)\n",
-		    conf->bConfigurationValue, NDISUSB_CONFIG_NO);
-
 	intf = &selconf->usc_intf;
 	for (i = 0; i < conf->bNumInterface && intf->uii_len > 0; i++) {
 		ret = usb2_set_alt_interface_index(udev,
@@ -594,7 +594,7 @@ usbd_setup_endpoint(ip, ifidx, ep)
 		cfg.mh.flags.short_xfer_ok = 1;
 
 	status = usb2_transfer_setup(sc->ndisusb_dev, &ifidx, ne->ne_xfer,
-	    &cfg, 1, sc, hal_getdisplock());
+	    &cfg, 1, sc, &sc->ndisusb_mtx);
 	if (status != USB_ERR_NORMAL_COMPLETION) {
 		device_printf(dev, "couldn't setup xfer: %s\n",
 		    usb2_errstr(status));
@@ -618,8 +618,9 @@ static int32_t
 usbd_func_abort_pipe(ip)
 	irp			*ip;
 {
+	device_t dev = IRP_NDIS_DEV(ip);
+	struct ndis_softc *sc = device_get_softc(dev);
 	struct ndisusb_ep *ne;
-	uint8_t irql;
 	union usbd_urb *urb;
 
 	urb = usbd_geturb(ip);
@@ -629,10 +630,10 @@ usbd_func_abort_pipe(ip)
 		return (USBD_STATUS_INVALID_PIPE_HANDLE);
 	}
 
-	KeRaiseIrql(DISPATCH_LEVEL, &irql);
+	NDISUSB_LOCK(sc);
 	usb2_transfer_stop(ne->ne_xfer[0]);
 	usb2_transfer_start(ne->ne_xfer[0]);
-	KeLowerIrql(irql);
+	NDISUSB_UNLOCK(sc);
 
 	return (USBD_STATUS_SUCCESS);
 }
@@ -644,7 +645,7 @@ usbd_func_vendorclass(ip)
 	device_t dev = IRP_NDIS_DEV(ip);
 	struct ndis_softc *sc = device_get_softc(dev);
 	struct usbd_urb_vendor_or_class_request *vcreq;
-	uint8_t irql, type = 0;
+	uint8_t type = 0;
 	union usbd_urb *urb;
 	struct usb2_device_request req;
 	usb2_error_t status;
@@ -692,10 +693,10 @@ usbd_func_vendorclass(ip)
 	USETW(req.wValue, vcreq->uvc_value);
 	USETW(req.wLength, vcreq->uvc_trans_buflen);
 
-	KeRaiseIrql(DISPATCH_LEVEL, &irql);
-	status = usb2_do_request(sc->ndisusb_dev, hal_getdisplock(), &req,
+	NDISUSB_LOCK(sc);
+	status = usb2_do_request(sc->ndisusb_dev, &sc->ndisusb_mtx, &req,
 	    vcreq->uvc_trans_buf);
-	KeLowerIrql(irql);
+	NDISUSB_UNLOCK(sc);
 
 	return usbd_usb2urb(status);
 }
@@ -755,19 +756,18 @@ static struct ndisusb_xfer *
 usbd_aq_getfirst(struct ndis_softc *sc, struct ndisusb_ep *ne)
 {
 	struct ndisusb_xfer *nx;
-	uint8_t irql;
 
-	KeAcquireSpinLock(&ne->ne_lock, &irql);
+	KeAcquireSpinLockAtDpcLevel(&ne->ne_lock);
 	if (IsListEmpty(&ne->ne_active)) {
 		device_printf(sc->ndis_dev,
 		    "%s: the active queue can't be empty.\n", __func__);
-		KeReleaseSpinLock(&ne->ne_lock, irql);
+		KeReleaseSpinLockFromDpcLevel(&ne->ne_lock);
 		return (NULL);
 	}
 	nx = CONTAINING_RECORD(ne->ne_active.nle_flink, struct ndisusb_xfer,
 	    nx_next);
 	RemoveEntryList(&nx->nx_next);
-	KeReleaseSpinLock(&ne->ne_lock, irql);
+	KeReleaseSpinLockFromDpcLevel(&ne->ne_lock);
 
 	return (nx);
 }
@@ -881,10 +881,8 @@ usbd_get_ndisep(ip, ep)
 
 	ne = &sc->ndisusb_ep[NDISUSB_GET_ENDPT(ep->bEndpointAddress)];
 
-	IoAcquireCancelSpinLock(&ip->irp_cancelirql);
 	IRP_NDISUSB_EP(ip) = ne;
 	ip->irp_cancelfunc = (cancel_func)usbd_irpcancel_wrap;
-	IoReleaseCancelSpinLock(ip->irp_cancelirql);
 
 	return (ne);
 }
@@ -902,7 +900,6 @@ usbd_xfertask(dobj, arg)
 	struct ndisusb_xferdone *nd;
 	struct ndisusb_xfer *nq;
 	struct usbd_urb_bulk_or_intr_transfer *ubi;
-	uint8_t irql;
 	union usbd_urb *urb;
 	usb2_error_t status;
 	void *priv;
@@ -912,7 +909,7 @@ usbd_xfertask(dobj, arg)
 	if (IsListEmpty(&sc->ndisusb_xferdonelist))
 		return;
 
-	KeAcquireSpinLock(&sc->ndisusb_xferdonelock, &irql);
+	KeAcquireSpinLockAtDpcLevel(&sc->ndisusb_xferdonelock);
 	l = sc->ndisusb_xferdonelist.nle_flink;
 	while (l != &sc->ndisusb_xferdonelist) {
 		nd = CONTAINING_RECORD(l, struct ndisusb_xferdone, nd_donelist);
@@ -928,8 +925,6 @@ usbd_xfertask(dobj, arg)
 		    ("function(%d) isn't for bulk or interrupt",
 			urb->uu_hdr.uuh_func));
 
-		IoAcquireCancelSpinLock(&ip->irp_cancelirql);
-
 		ip->irp_cancelfunc = NULL;
 		IRP_NDISUSB_EP(ip) = NULL;
 
@@ -954,28 +949,114 @@ usbd_xfertask(dobj, arg)
 			break;
 		}
 
-		IoReleaseCancelSpinLock(ip->irp_cancelirql);
-
 		l = l->nle_flink;
 		RemoveEntryList(&nd->nd_donelist);
 		free(nq, M_USBDEV);
 		free(nd, M_USBDEV);
 		if (error)
 			continue;
+		KeReleaseSpinLockFromDpcLevel(&sc->ndisusb_xferdonelock);
 		/* NB: call after cleaning  */
 		IoCompleteRequest(ip, IO_NO_INCREMENT);
+		KeAcquireSpinLockAtDpcLevel(&sc->ndisusb_xferdonelock);
 	}
-	KeReleaseSpinLock(&sc->ndisusb_xferdonelock, irql);
+	KeReleaseSpinLockFromDpcLevel(&sc->ndisusb_xferdonelock);
+}
+
+/*
+ * this function is for mainly deferring a task to the another thread because
+ * we don't want to be in the scope of HAL lock.
+ */
+static int32_t
+usbd_taskadd(ip, type)
+	irp			*ip;
+	unsigned		 type;
+{
+	device_t dev = IRP_NDIS_DEV(ip);
+	struct ndis_softc *sc = device_get_softc(dev);
+	struct ndisusb_task *nt;
+
+	nt = malloc(sizeof(struct ndisusb_task), M_USBDEV, M_NOWAIT | M_ZERO);
+	if (nt == NULL)
+		return (USBD_STATUS_NO_MEMORY);
+	nt->nt_type = type;
+	nt->nt_ctx = ip;
+
+	KeAcquireSpinLockAtDpcLevel(&sc->ndisusb_tasklock);
+	InsertTailList((&sc->ndisusb_tasklist), (&nt->nt_tasklist));
+	KeReleaseSpinLockFromDpcLevel(&sc->ndisusb_tasklock);
+
+	IoQueueWorkItem(sc->ndisusb_taskitem,
+	    (io_workitem_func)usbd_task_wrap, WORKQUEUE_CRITICAL, sc);
+
+	return (USBD_STATUS_SUCCESS);
+}
+
+static void
+usbd_task(dobj, arg)
+	device_object		*dobj;
+	void			*arg;
+{
+	irp *ip;
+	list_entry *l;
+	struct ndis_softc *sc = arg;
+	struct ndisusb_ep *ne;
+	struct ndisusb_task *nt;
+	union usbd_urb *urb;
+
+	if (IsListEmpty(&sc->ndisusb_tasklist))
+		return;
+
+	KeAcquireSpinLockAtDpcLevel(&sc->ndisusb_tasklock);
+	l = sc->ndisusb_tasklist.nle_flink;
+	while (l != &sc->ndisusb_tasklist) {
+		nt = CONTAINING_RECORD(l, struct ndisusb_task, nt_tasklist);
+
+		ip = nt->nt_ctx;
+		urb = usbd_geturb(ip);
+
+		KeReleaseSpinLockFromDpcLevel(&sc->ndisusb_tasklock);
+		NDISUSB_LOCK(sc);
+		switch (nt->nt_type) {
+		case NDISUSB_TASK_TSTART:
+			ne = usbd_get_ndisep(ip, urb->uu_bulkintr.ubi_epdesc);
+			if (ne == NULL)
+				goto exit;
+			usb2_transfer_start(ne->ne_xfer[0]);
+			break;
+		case NDISUSB_TASK_IRPCANCEL:
+			ne = usbd_get_ndisep(ip,
+			    (nt->nt_type == NDISUSB_TASK_IRPCANCEL) ?
+			    urb->uu_bulkintr.ubi_epdesc :
+			    urb->uu_pipe.upr_handle);
+			if (ne == NULL)
+				goto exit;
+			
+			usb2_transfer_stop(ne->ne_xfer[0]);
+			usb2_transfer_start(ne->ne_xfer[0]);
+			break;
+		default:
+			break;
+		}
+exit:
+		NDISUSB_UNLOCK(sc);
+		KeAcquireSpinLockAtDpcLevel(&sc->ndisusb_tasklock);
+
+		l = l->nle_flink;
+		RemoveEntryList(&nt->nt_tasklist);
+		free(nt, M_USBDEV);
+	}
+	KeReleaseSpinLockFromDpcLevel(&sc->ndisusb_tasklock);
 }
 
 static int32_t
 usbd_func_bulkintr(ip)
 	irp			*ip;
 {
+	int32_t error;
 	struct ndisusb_ep *ne;
 	struct ndisusb_xfer *nx;
 	struct usbd_urb_bulk_or_intr_transfer *ubi;
-	uint8_t irql;
 	union usbd_urb *urb;
 	usb_endpoint_descriptor_t *ep;
 
@@ -998,9 +1079,9 @@ usbd_func_bulkintr(ip)
 	}
 	nx->nx_ep = ne;
 	nx->nx_priv = ip;
-	KeAcquireSpinLock(&ne->ne_lock, &irql);
+	KeAcquireSpinLockAtDpcLevel(&ne->ne_lock);
 	InsertTailList((&ne->ne_pending), (&nx->nx_next));
-	KeReleaseSpinLock(&ne->ne_lock, irql);
+	KeReleaseSpinLockFromDpcLevel(&ne->ne_lock);
 
 	/* we've done to setup xfer.  Let's transfer it.  */
 	ip->irp_iostat.isb_status = STATUS_PENDING;
@@ -1008,9 +1089,9 @@ usbd_func_bulkintr(ip)
 	USBD_URB_STATUS(urb) = USBD_STATUS_PENDING;
 	IoMarkIrpPending(ip);
 
-	KeRaiseIrql(DISPATCH_LEVEL, &irql);
-	usb2_transfer_start(ne->ne_xfer[0]);
-	KeLowerIrql(irql);
+	error = usbd_taskadd(ip, NDISUSB_TASK_TSTART);
+	if (error != USBD_STATUS_SUCCESS)
+		return (error);
 
 	return (USBD_STATUS_PENDING);
 }

sys/dev/if_ndis/if_ndis.c

@@ -557,8 +557,10 @@ ndis_attach(dev)
 	mtx_init(&sc->ndis_mtx, device_get_nameunit(dev), MTX_NETWORK_LOCK,
 	    MTX_DEF);
 	KeInitializeSpinLock(&sc->ndis_rxlock);
+	KeInitializeSpinLock(&sc->ndisusb_tasklock);
 	KeInitializeSpinLock(&sc->ndisusb_xferdonelock);
 	InitializeListHead(&sc->ndis_shlist);
+	InitializeListHead(&sc->ndisusb_tasklist);
 	InitializeListHead(&sc->ndisusb_xferdonelist);
 	callout_init(&sc->ndis_stat_callout, CALLOUT_MPSAFE);
 
@@ -625,6 +627,8 @@ ndis_attach(dev)
 	sc->ndis_inputitem = IoAllocateWorkItem(sc->ndis_block->nmb_deviceobj);
 	sc->ndisusb_xferdoneitem =
 	    IoAllocateWorkItem(sc->ndis_block->nmb_deviceobj);
+	sc->ndisusb_taskitem =
+	    IoAllocateWorkItem(sc->ndis_block->nmb_deviceobj);
 	KeInitializeDpc(&sc->ndis_rxdpc, ndis_rxeof_xfr_wrap, sc->ndis_block);
 
 	/* Call driver's init routine. */
@@ -1066,6 +1070,8 @@ ndis_detach(dev)
 		IoFreeWorkItem(sc->ndis_inputitem);
 	if (sc->ndisusb_xferdoneitem != NULL)
 		IoFreeWorkItem(sc->ndisusb_xferdoneitem);
+	if (sc->ndisusb_taskitem != NULL)
+		IoFreeWorkItem(sc->ndisusb_taskitem);
 
 	bus_generic_detach(dev);
 	ndis_unload_driver(sc);
@@ -3236,8 +3242,10 @@ ndis_stop(sc)
 	ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE);
 	NDIS_UNLOCK(sc);
 
-	if (!(sc->ndis_iftype == PNPBus && ndisusb_halt == 0) ||
-	    sc->ndisusb_status & NDISUSB_STATUS_DETACH)
+	if (sc->ndis_iftype != PNPBus ||
+	    (sc->ndis_iftype == PNPBus &&
+	     !(sc->ndisusb_status & NDISUSB_STATUS_DETACH) &&
+	     ndisusb_halt != 0))
 		ndis_halt_nic(sc);
 
 	NDIS_LOCK(sc);

sys/dev/if_ndis/if_ndis_usb.c

@@ -168,6 +168,7 @@ ndisusb_attach(device_t self)
 	db = uaa->driver_info;
 	sc = (struct ndis_softc *)dummy;
 	sc->ndis_dev = self;
+	mtx_init(&sc->ndisusb_mtx, "NDIS USB", MTX_NETWORK_LOCK, MTX_DEF);
 	sc->ndis_dobj = db->windrv_object;
 	sc->ndis_regvals = db->windrv_regvals;
 	sc->ndis_iftype = PNPBus;
@@ -207,14 +208,17 @@ ndisusb_detach(device_t self)
 
 	sc->ndisusb_status |= NDISUSB_STATUS_DETACH;
 
+	ndis_pnpevent_nic(self, NDIS_PNP_EVENT_SURPRISE_REMOVED);
+
 	for (i = 0; i < NDISUSB_ENDPT_MAX; i++) {
 		ne = &sc->ndisusb_ep[i];
 		usb2_transfer_unsetup(ne->ne_xfer, 1);
 	}
 
-	ndis_pnpevent_nic(self, NDIS_PNP_EVENT_SURPRISE_REMOVED);
+	(void)ndis_detach(self);
 
-	return ndis_detach(self);
+	mtx_destroy(&sc->ndisusb_mtx);
+	return (0);
 }
 
 static struct resource_list *

sys/dev/if_ndis/if_ndisvar.h

@@ -142,6 +142,14 @@ struct ndisusb_xferdone {
 	list_entry		nd_donelist;
 };
 
+struct ndisusb_task {
+	unsigned		nt_type;
+#define	NDISUSB_TASK_TSTART	0
+#define	NDISUSB_TASK_IRPCANCEL	1
+	void			*nt_ctx;
+	list_entry		nt_tasklist;
+};
+
 struct ndis_softc {
 	struct ifnet		*ifp;
 	struct ifmedia		ifmedia;	/* media info */
@@ -220,6 +228,7 @@ struct ndis_softc {
 	int			ndis_hang_timer;
 
 	struct usb2_device	*ndisusb_dev;
+	struct mtx		ndisusb_mtx;
 #define	NDISUSB_GET_ENDPT(addr) \
 	((UE_GET_DIR(addr) >> 7) | (UE_GET_ADDR(addr) << 1))
 #define	NDISUSB_ENDPT_MAX	((UE_ADDR + 1) * 2)
@@ -227,6 +236,9 @@ struct ndis_softc {
 	io_workitem		*ndisusb_xferdoneitem;
 	list_entry		ndisusb_xferdonelist;
 	kspin_lock		ndisusb_xferdonelock;
+	io_workitem		*ndisusb_taskitem;
+	list_entry		ndisusb_tasklist;
+	kspin_lock		ndisusb_tasklock;
 	int			ndisusb_status;
 #define NDISUSB_STATUS_DETACH	0x1
 };
@@ -234,3 +246,7 @@ struct ndis_softc {
 #define	NDIS_LOCK(_sc)		mtx_lock(&(_sc)->ndis_mtx)
 #define	NDIS_UNLOCK(_sc)	mtx_unlock(&(_sc)->ndis_mtx)
 #define	NDIS_LOCK_ASSERT(_sc, t)	mtx_assert(&(_sc)->ndis_mtx, t)
+#define	NDISUSB_LOCK(_sc)	mtx_lock(&(_sc)->ndisusb_mtx)
+#define	NDISUSB_UNLOCK(_sc)	mtx_unlock(&(_sc)->ndisusb_mtx)
+#define	NDISUSB_LOCK_ASSERT(_sc, t)	mtx_assert(&(_sc)->ndisusb_mtx, t)
+