Attempt to fix a problem with receiving packets on USB ethernet interfaces.

Packets are received inside USB bulk transfer callbacks, which run at
splusb() (actually splbio()). The packet input queues are meant to be
manipulated at splimp(). However the locking apparently breaks down under
certain circumstances and the input queues can get trampled.

There's a similar problem with if_ppp, which is driven by hardware/tty
interrupts from the serial driver, but which must also manipulate the
packet input queues at splimp(). The fix there is to use a netisr, and
that's the fix I used here. (I can hear you groaning back there. Hush up.)

The usb_ethersubr module maintains a single queue of its own. When a
packet is received in the USB callback routine, it's placed on this
queue with usb_ether_input(). This routine also schedules a soft net
interrupt with schednetisr(). The ISR routine then runs later, at
splnet, outside of the USB callback/interrupt context, and passes the
packet to ether_input(), hopefully in a safe manner.

The reason this is implemented as a separate module is that there are
a limited number of NETISRs that we can use, and snarfing one up for
each driver that needs it is wasteful (there will be three once I get
the CATC driver done). It also reduces code duplication to a certain
small extent. Unfortunately, it also needs to be linked in with the
usb.ko module in order for the USB ethernet drivers to share it.

Also removed some uneeded includes from if_aue.c and if_kue.c

Fix suggested by: peter
Not rejected as a hairbrained idea by: n_hibma

7 files changed, 180 insertions, 45 deletions

diff --git a/sys/conf/files b/sys/conf/files
index 6427b6e..e256518 100644
--- a/sys/conf/files
+++ b/sys/conf/files
@@ -884,6 +884,7 @@ dev/usb/usb.c		optional usb
 dev/usb/usbdi.c		optional usb
 dev/usb/usbdi_util.c	optional usb
 #dev/usb/usb_mem.c	optional usb
+dev/usb/usb_ethersubr.c	optional usb
 dev/usb/usb_subr.c	optional usb
 dev/usb/usb_quirks.c	optional usb
 dev/usb/hid.c		optional usb
diff --git a/sys/dev/usb/if_aue.c b/sys/dev/usb/if_aue.c
index c8c204c..66ef16b 100644
--- a/sys/dev/usb/if_aue.c
+++ b/sys/dev/usb/if_aue.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 1998, 1999
+ * Copyright (c) 1997, 1998, 1999, 2000
  *	Bill Paul <wpaul@ee.columbia.edu>.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -77,21 +77,15 @@
 
 #include <net/bpf.h>
 
-#include <vm/vm.h>              /* for vtophys */
-#include <vm/pmap.h>            /* for vtophys */
 #include <machine/clock.h>      /* for DELAY */
-#include <machine/bus_pio.h>
-#include <machine/bus_memio.h>
-#include <machine/bus.h>
-#include <machine/resource.h>
 #include <sys/bus.h>
-#include <sys/rman.h>
 
 #include <dev/usb/usb.h>
 #include <dev/usb/usbdi.h>
 #include <dev/usb/usbdi_util.h>
 #include <dev/usb/usbdivar.h>
 #include <dev/usb/usbdevs.h>
+#include <dev/usb/usb_ethersubr.h>
 
 #include <dev/mii/mii.h>
 #include <dev/mii/miivar.h>
@@ -688,6 +682,7 @@ USB_ATTACH(aue)
 	ether_ifattach(ifp);
 	callout_handle_init(&sc->aue_stat_ch);
 	bpfattach(ifp, DLT_EN10MB, sizeof(struct ether_header));
+	usb_register_netisr();
 
 	splx(s);
 	USB_ATTACH_SUCCESS_RETURN;
@@ -879,24 +874,17 @@ static void aue_rxeof(xfer, priv, status)
         struct ifnet		*ifp;
 	int			total_len = 0;
 	struct aue_rxpkt	r;
-	int			s;
-
-	s = splimp();
 
 	c = priv;
 	sc = c->aue_sc;
 	ifp = &sc->arpcom.ac_if;
 
-	if (!(ifp->if_flags & IFF_RUNNING)) {
+	if (!(ifp->if_flags & IFF_RUNNING))
 		return;
-		splx(s);
-	}
 
 	if (status != USBD_NORMAL_COMPLETION) {
-		if (status == USBD_NOT_STARTED || status == USBD_CANCELLED) {
-			splx(s);
+		if (status == USBD_NOT_STARTED || status == USBD_CANCELLED)
 			return;
-		}
 		printf("aue%d: usb error on rx: %s\n", sc->aue_unit,
 		    usbd_errstr(status));
 		if (status == USBD_STALLED)
@@ -938,7 +926,6 @@ static void aue_rxeof(xfer, priv, status)
 		    AUE_CUTOFF, USBD_SHORT_XFER_OK,
 		    USBD_NO_TIMEOUT, aue_rxeof);
 		usbd_transfer(xfer);
-		splx(s);
 		return;
 	}
 
@@ -976,9 +963,8 @@ static void aue_rxeof(xfer, priv, status)
 		}
 	}
 
-	/* Remove header from mbuf and pass it on. */
-	m_adj(m, sizeof(struct ether_header));
-	ether_input(ifp, eh, m);
+	/* Put the packet on the special USB input queue. */
+	usb_ether_input(m);
 
 done:
 
@@ -988,8 +974,6 @@ done:
 	    USBD_NO_TIMEOUT, aue_rxeof);
 	usbd_transfer(xfer);
 
-	splx(s);
-
 	return;
 }
 
diff --git a/sys/dev/usb/if_kue.c b/sys/dev/usb/if_kue.c
index 0167d9e..2371a87 100644
--- a/sys/dev/usb/if_kue.c
+++ b/sys/dev/usb/if_kue.c
@@ -80,15 +80,8 @@
 
 #include <net/bpf.h>
 
-#include <vm/vm.h>              /* for vtophys */
-#include <vm/pmap.h>            /* for vtophys */
 #include <machine/clock.h>      /* for DELAY */
-#include <machine/bus_pio.h>
-#include <machine/bus_memio.h>
-#include <machine/bus.h>
-#include <machine/resource.h>
 #include <sys/bus.h>
-#include <sys/rman.h>
 
 #include <dev/usb/usb.h>
 #include <dev/usb/usbdi.h>
@@ -96,6 +89,7 @@
 #include <dev/usb/usbdivar.h>
 #include <dev/usb/usbdevs.h>
 #include <dev/usb/usb_quirks.h>
+#include <dev/usb/usb_ethersubr.h>
 
 #include <dev/usb/if_kuereg.h>
 #include <dev/usb/kue_fw.h>
@@ -516,6 +510,7 @@ USB_ATTACH(kue)
 	if_attach(ifp);
 	ether_ifattach(ifp);
 	bpfattach(ifp, DLT_EN10MB, sizeof(struct ether_header));
+	usb_register_netisr();
 
 	splx(s);
 	USB_ATTACH_SUCCESS_RETURN;
@@ -654,24 +649,17 @@ static void kue_rxeof(xfer, priv, status)
         struct ifnet		*ifp;
 	int			total_len = 0;
 	u_int16_t		len;
-	int			s;
-
-	s = splimp();
 
 	c = priv;
 	sc = c->kue_sc;
 	ifp = &sc->arpcom.ac_if;
 
-	if (!(ifp->if_flags & IFF_RUNNING)) {
+	if (!(ifp->if_flags & IFF_RUNNING))
 		return;
-		splx(s);
-	}
 
 	if (status != USBD_NORMAL_COMPLETION) {
-		if (status == USBD_NOT_STARTED || status == USBD_CANCELLED) {
-			splx(s);
+		if (status == USBD_NOT_STARTED || status == USBD_CANCELLED)
 			return;
-		}
 		printf("kue%d: usb error on rx: %s\n", sc->kue_unit,
 		    usbd_errstr(status));
 		if (status == USBD_STALLED)
@@ -715,9 +703,8 @@ static void kue_rxeof(xfer, priv, status)
 		}
 	}
 
-	/* Remove header from mbuf and pass it on. */
-	m_adj(m, sizeof(struct ether_header));
-	ether_input(ifp, eh, m);
+	/* Put the packet on the special USB input queue. */
+	usb_ether_input(m);
 
 done:
 
@@ -727,8 +714,6 @@ done:
 	    USBD_NO_TIMEOUT, kue_rxeof);
 	usbd_transfer(xfer);
 
-	splx(s);
-
 	return;
 }
 
diff --git a/sys/dev/usb/usb_ethersubr.c b/sys/dev/usb/usb_ethersubr.c
new file mode 100644
index 0000000..ded747e
--- /dev/null
+++ b/sys/dev/usb/usb_ethersubr.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright (c) 1997, 1998, 1999, 2000
+ *	Bill Paul <wpaul@ee.columbia.edu>.  All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. All advertising materials mentioning features or use of this software
+ *    must display the following acknowledgement:
+ *	This product includes software developed by Bill Paul.
+ * 4. Neither the name of the author nor the names of any co-contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Bill Paul AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL Bill Paul OR THE VOICES IN HIS HEAD
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * $FreeBSD$
+ */
+
+/*
+ * Callbacks in the USB code operate at splusb() (actually splbio()
+ * in FreeBSD). However adding packets to the input queues has to be
+ * done at splimp(). It is conceivable that this arrangement could
+ * trigger a condition where the splimp() is ignored and the input
+ * queues could get trampled in spite of our best effors to prevent
+ * it. To work around this, we implement a special input queue for
+ * USB ethernet adapter drivers. Rather than passing the frames directly
+ * to ether_input(), we pass them here, then schedule a soft interrupt
+ * to hand them to ether_input() later, outside of the USB interrupt
+ * context.
+ *
+ * It's questional as to whether this code should be expanded to
+ * handle other kinds of devices, or handle USB transfer callbacks
+ * in general. Right now, I need USB network interfaces to work
+ * properly.
+ */
+
+#include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/sockio.h>
+#include <sys/mbuf.h>
+#include <sys/kernel.h>
+#include <sys/socket.h>
+
+#include <net/if.h>
+#include <net/if_types.h>
+#include <net/if_arp.h>
+#include <net/ethernet.h>
+#include <net/netisr.h>
+
+#include <dev/usb/usb_ethersubr.h>
+
+#ifndef lint
+static const char rcsid[] =
+  "$FreeBSD$";
+#endif
+
+static struct ifqueue usbq;
+
+static void usbintr		__P((void));
+
+static void usbintr()
+{
+	struct ether_header	*eh;
+	struct mbuf		*m;
+	int			s;
+
+	s = splimp();
+
+	while(1) {
+		IF_DEQUEUE(&usbq, m);
+		if (m == NULL)
+			break;
+		eh = mtod(m, struct ether_header *);
+		m_adj(m, sizeof(struct ether_header));
+		ether_input(m->m_pkthdr.rcvif, eh, m);
+	}
+
+	splx(s);
+
+	return;
+}
+
+void usb_register_netisr()
+{
+	register_netisr(NETISR_USB, usbintr);
+	return;
+}
+
+/*
+ * Must be called at splusp() (actually splbio()). This should be
+ * the case when called from a transfer callback routine.
+ */
+void usb_ether_input(m)
+	struct mbuf		*m;
+{
+	int			s;
+	s = splimp();
+	IF_ENQUEUE(&usbq, m);
+	schednetisr(NETISR_USB);
+	splx(s);
+	return;
+}
diff --git a/sys/dev/usb/usb_ethersubr.h b/sys/dev/usb/usb_ethersubr.h
new file mode 100644
index 0000000..f137c3e
--- /dev/null
+++ b/sys/dev/usb/usb_ethersubr.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 1997, 1998, 1999, 2000
+ *	Bill Paul <wpaul@ee.columbia.edu>.  All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. All advertising materials mentioning features or use of this software
+ *    must display the following acknowledgement:
+ *	This product includes software developed by Bill Paul.
+ * 4. Neither the name of the author nor the names of any co-contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Bill Paul AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL Bill Paul OR THE VOICES IN HIS HEAD
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * $FreeBSD$
+ */
+
+#ifndef _USB_ETHERSUBR_H_
+#define _USB_ETHERSUBR_H_
+
+#ifndef NETISR_USB
+#define NETISR_USB	25
+#endif
+
+void usb_register_netisr	__P((void));
+void usb_ether_input		__P((struct mbuf *));
+
+#endif
diff --git a/sys/modules/usb/Makefile b/sys/modules/usb/Makefile
index e372b73..ff49f89 100644
--- a/sys/modules/usb/Makefile
+++ b/sys/modules/usb/Makefile
@@ -17,7 +17,8 @@ SRCS		 = bus_if.h device_if.h usb_if.h usb_if.c \
 		   usb_subr.c \
 		   usbdevs.h usbdevs_data.h \
 		   usbdi.c usbdi.h usbdivar.h \
-		   usbdi_util.c usbdi_util.h
+		   usbdi_util.c usbdi_util.h \
+		   usb_ethersubr.c
 
 SRCS		+= uhci_pci.c uhci.c uhcireg.h uhcivar.h
 SRCS		+= ohci_pci.c ohci.c ohcireg.h ohcivar.h
diff --git a/sys/net/netisr.h b/sys/net/netisr.h
index b1e32f3..563c4ec 100644
--- a/sys/net/netisr.h
+++ b/sys/net/netisr.h
@@ -64,6 +64,7 @@
 #define	NETISR_ATALK    16              /* same as AF_APPLETALK */
 #define	NETISR_ARP	18		/* same as AF_LINK */
 #define	NETISR_IPX	23		/* same as AF_IPX */
+#define NETISR_USB	25		/* USB soft interrupt */
 #define	NETISR_ISDN	26		/* same as AF_E164 */
 #define	NETISR_PPP	27		/* PPP soft interrupt */
 #define	NETISR_IPV6	28		/* same as AF_INET6 */