Use workqueues so that we don't call into the scsipi subsystem via

a softint from the network stack.
Don't recurse through scsipi_command() when we have multiple packets
in the send queue - use a loop instead.  This means that we no longer
need sestart(), as we can now handle everything in sedone().
Fix a couple of XXX's.
Rework the locking logic slightly from the previous revision.
Now this works with DIAGNOSTIC+LOCKDEBUG.

sys/dev/scsipi/if_se.c

@@ -1,4 +1,4 @@
-/*	$NetBSD: if_se.c,v 1.105 2020/06/19 10:30:27 jdc Exp $	*/
+/*	$NetBSD: if_se.c,v 1.106 2020/06/22 16:05:20 jdc Exp $	*/
 
 /*
  * Copyright (c) 1997 Ian W. Dall <ian.dall@dsto.defence.gov.au>
@@ -49,7 +49,7 @@
  * This driver is also a bit unusual. It must look like a network
  * interface and it must also appear to be a scsi device to the scsi
  * system. Hence there are cases where there are two entry points. eg
- * sestart is to be called from the scsi subsytem and se_ifstart from
+ * sedone is to be called from the scsi subsytem and se_ifstart from
  * the network interface subsystem.  In addition, to facilitate scsi
  * commands issued by userland programs, there are open, close and
  * ioctl entry points. This allows a user program to, for example,
@@ -59,10 +59,11 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_se.c,v 1.105 2020/06/19 10:30:27 jdc Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_se.c,v 1.106 2020/06/22 16:05:20 jdc Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
+#include "opt_net_mpsafe.h"
 #include "opt_atalk.h"
 #endif
 
@@ -85,6 +86,7 @@ __KERNEL_RCSID(0, "$NetBSD: if_se.c,v 1.105 2020/06/19 10:30:27 jdc Exp $");
 #include <sys/conf.h>
 #include <sys/mutex.h>
 #include <sys/pcq.h>
+#include <sys/workqueue.h>
 
 #include <dev/scsipi/scsipi_all.h>
 #include <dev/scsipi/scsi_ctron_ether.h>
@@ -177,10 +179,12 @@ struct se_softc {
 	struct ethercom sc_ethercom;	/* Ethernet common part */
 	struct scsipi_periph *sc_periph;/* contains our targ, lun, etc. */
 
-	struct callout sc_ifstart_ch;
 	struct callout sc_recv_ch;
 	struct kmutex sc_iflock;
 	struct if_percpuq *sc_ipq;
+	struct workqueue *sc_recv_wq, *sc_send_wq;
+	struct work sc_recv_work, sc_send_work;
+	int sc_recv_work_pending, sc_send_work_pending;
 
 	char *sc_tbuf;
 	char *sc_rbuf;
@@ -200,7 +204,6 @@ static int	sematch(device_t, cfdata_t, void *);
 static void	seattach(device_t, device_t, void *);
 
 static void	se_ifstart(struct ifnet *);
-static void	sestart(struct scsipi_periph *);
 
 static void	sedone(struct scsipi_xfer *, int);
 static int	se_ioctl(struct ifnet *, u_long, void *);
@@ -209,10 +212,12 @@ static void	sewatchdog(struct ifnet *);
 #if 0
 static inline uint16_t ether_cmp(void *, void *);
 #endif
-static void	se_recv(void *);
+static void	se_recv_callout(void *);
+static void	se_recv_worker(struct work *wk, void *cookie);
+static void	se_recv(struct se_softc *);
 static struct mbuf *se_get(struct se_softc *, char *, int);
 static int	se_read(struct se_softc *, char *, int);
-static int	se_reset(struct se_softc *);
+static void	se_reset(struct se_softc *);
 static int	se_add_proto(struct se_softc *, int);
 static int	se_get_addr(struct se_softc *, uint8_t *);
 static int	se_set_media(struct se_softc *, int);
@@ -228,7 +233,7 @@ static inline int se_scsipi_cmd(struct scsipi_periph *periph,
 			int cmdlen, u_char *data_addr, int datalen,
 			int retries, int timeout, struct buf *bp,
 			int flags);
-static void	se_delayed_ifstart(void *);
+static void	se_send_worker(struct work *wk, void *cookie);
 static int	se_set_mode(struct se_softc *, int, int);
 
 int	se_enable(struct se_softc *);
@@ -260,7 +265,7 @@ const struct cdevsw se_cdevsw = {
 
 const struct scsipi_periphsw se_switch = {
 	NULL,			/* Use default error handler */
-	sestart,		/* have a queue, served by this */
+	NULL,			/* have no queue */
 	NULL,			/* have no async handler */
 	sedone,			/* deal with stats at interrupt time */
 };
@@ -317,6 +322,7 @@ seattach(device_t parent, device_t self, void *aux)
 	struct scsipi_periph *periph = sa->sa_periph;
 	struct ifnet *ifp = &sc->sc_ethercom.ec_if;
 	uint8_t myaddr[ETHER_ADDR_LEN];
+	char wqname[MAXCOMLEN];
 	int rv;
 
 	sc->sc_dev = self;
@@ -324,7 +330,6 @@ seattach(device_t parent, device_t self, void *aux)
 	printf("\n");
 	SC_DEBUG(periph, SCSIPI_DB2, ("seattach: "));
 
-	callout_init(&sc->sc_ifstart_ch, CALLOUT_MPSAFE);
 	callout_init(&sc->sc_recv_ch, CALLOUT_MPSAFE);
 	mutex_init(&sc->sc_iflock, MUTEX_DEFAULT, IPL_SOFTNET);
 
@@ -335,21 +340,17 @@ seattach(device_t parent, device_t self, void *aux)
 	periph->periph_dev = sc->sc_dev;
 	periph->periph_switch = &se_switch;
 
-	/* XXX increase openings? */
-
 	se_poll = (SE_POLL * hz) / 1000;
 	se_poll = se_poll? se_poll: 1;
 	se_poll0 = (SE_POLL0 * hz) / 1000;
 	se_poll0 = se_poll0? se_poll0: 1;
 
 	/*
-	 * Initialize and attach a buffer
+	 * Initialize and attach send and receive buffers
 	 */
 	sc->sc_tbuf = malloc(ETHERMTU + sizeof(struct ether_header),
 			     M_DEVBUF, M_WAITOK);
-	sc->sc_rbuf = malloc(RBUF_LEN, M_DEVBUF, M_WAITOK);/* A Guess */
+	sc->sc_rbuf = malloc(RBUF_LEN, M_DEVBUF, M_WAITOK);
-
-	se_get_addr(sc, myaddr);
 
 	/* Initialize ifnet structure. */
 	strlcpy(ifp->if_xname, device_xname(sc->sc_dev), sizeof(ifp->if_xname));
@@ -358,17 +359,44 @@ seattach(device_t parent, device_t self, void *aux)
 	ifp->if_ioctl = se_ioctl;
 	ifp->if_watchdog = sewatchdog;
 	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
+	ifp->if_extflags = IFEF_MPSAFE;
 	IFQ_SET_READY(&ifp->if_snd);
 
+	se_get_addr(sc, myaddr);
+
 	/* Attach the interface. */
 	rv = if_initialize(ifp);
 	if (rv != 0) {
 		free(sc->sc_tbuf, M_DEVBUF);
-		callout_destroy(&sc->sc_ifstart_ch);
 		callout_destroy(&sc->sc_recv_ch);
 		mutex_destroy(&sc->sc_iflock);
 		return; /* Error */
 	}
+	snprintf(wqname, sizeof(wqname), "%sRx", device_xname(sc->sc_dev));
+	rv = workqueue_create(&sc->sc_recv_wq, wqname, se_recv_worker, sc,
+	    PRI_SOFTNET, IPL_NET, WQ_MPSAFE);
+	if (rv != 0) {
+		aprint_error_dev(sc->sc_dev,
+		    "unable to create recv workqueue\n");
+		free(sc->sc_tbuf, M_DEVBUF);
+		callout_destroy(&sc->sc_recv_ch);
+		mutex_destroy(&sc->sc_iflock);
+		return; /* Error */
+	}
+	sc->sc_recv_work_pending = false;
+	snprintf(wqname, sizeof(wqname), "%sTx", device_xname(sc->sc_dev));
+	rv = workqueue_create(&sc->sc_send_wq, wqname, se_send_worker, ifp,
+	    PRI_SOFTNET, IPL_NET, WQ_MPSAFE);
+	if (rv != 0) {
+		aprint_error_dev(sc->sc_dev,
+		    "unable to create send workqueue\n");
+		free(sc->sc_tbuf, M_DEVBUF);
+		callout_destroy(&sc->sc_recv_ch);
+		mutex_destroy(&sc->sc_iflock);
+		workqueue_destroy(sc->sc_send_wq);
+		return; /* Error */
+	}
+	sc->sc_send_work_pending = false;
 	sc->sc_ipq = if_percpuq_create(&sc->sc_ethercom.ec_if);
 	ether_ifattach(ifp, myaddr);
 	if_register(ifp);
@@ -384,108 +412,108 @@ se_scsipi_cmd(struct scsipi_periph *periph, struct scsipi_generic *cmd,
 {
 	int error;
 
-	KASSERT(!mutex_owned(chan_mtx(periph->periph_channel)));
-
 	error = scsipi_command(periph, cmd, cmdlen, data_addr,
 	    datalen, retries, timeout, bp, flags);
 	return (error);
 }
 
 /*
- * Start routine for calling from scsi sub system
+ * Start routine for calling from network sub system
- * Called with the channel lock held
- */
-static void
-sestart(struct scsipi_periph *periph)
-{
-	struct se_softc *sc = device_private(periph->periph_dev);
-	struct ifnet *ifp = &sc->sc_ethercom.ec_if;
-
-	se_ifstart(ifp);
-}
-
-static void
-se_delayed_ifstart(void *v)
-{
-	struct ifnet *ifp = v;
-	struct se_softc *sc = ifp->if_softc;
-
-	mutex_enter(chan_mtx(sc->sc_periph->periph_channel));
-	if (sc->sc_enabled) {
-		ifp->if_flags &= ~IFF_OACTIVE;
-		se_ifstart(ifp);
-	}
-	mutex_exit(chan_mtx(sc->sc_periph->periph_channel));
-}
-
-/*
- * Start transmission on the interface.
- * Must be called with the scsipi channel lock held
  */
 static void
 se_ifstart(struct ifnet *ifp)
 {
+	struct se_softc *sc = ifp->if_softc;
+	int i = 100;
+
+	mutex_enter(&sc->sc_iflock);
+	while (i && sc->sc_send_work_pending == true) {
+		i--;
+		delay(10);
+	}
+	if (i) {
+		sc->sc_send_work_pending = true;
+		workqueue_enqueue(sc->sc_send_wq, &sc->sc_send_work, NULL);
+	} else
+		if_statinc(ifp, if_oerrors);
+	mutex_exit(&sc->sc_iflock);
+}
+
+/*
+ * Invoke the transmit workqueue and transmission on the interface.
+ */
+static void
+se_send_worker(struct work *wk, void *cookie)
+{
+	struct ifnet *ifp = cookie;
 	struct se_softc *sc = ifp->if_softc;
 	struct scsi_ctron_ether_generic send_cmd;
 	struct mbuf *m, *m0;
 	int len, error;
 	u_char *cp;
 
+	mutex_enter(&sc->sc_iflock);
+	sc->sc_send_work_pending = false;
+	mutex_exit(&sc->sc_iflock);
+
+	KASSERT(if_is_mpsafe(ifp));
+
 	/* Don't transmit if interface is busy or not running */
 	if ((ifp->if_flags & (IFF_RUNNING | IFF_OACTIVE)) != IFF_RUNNING)
 		return;
 
-	IFQ_DEQUEUE(&ifp->if_snd, m0);
+	while (1) {
-	if (m0 == 0)
+		IFQ_DEQUEUE(&ifp->if_snd, m0);
-		return;
+		if (m0 == 0)
+			break;
 
-	KASSERT(mutex_owned(chan_mtx(sc->sc_periph->periph_channel)));
+		/* If BPF is listening on this interface, let it see the
+		 * packet before we commit it to the wire.
+		 */
+		bpf_mtap(ifp, m0, BPF_D_OUT);
 
-	/* If BPF is listening on this interface, let it see the
+		/* We need to use m->m_pkthdr.len, so require the header */
-	 * packet before we commit it to the wire.
+		if ((m0->m_flags & M_PKTHDR) == 0)
-	 */
+			panic("ctscstart: no header mbuf");
-	bpf_mtap(ifp, m0, BPF_D_OUT);
+		len = m0->m_pkthdr.len;
 
-	/* We need to use m->m_pkthdr.len, so require the header */
+		/* Mark the interface busy. */
-	if ((m0->m_flags & M_PKTHDR) == 0)
+		ifp->if_flags |= IFF_OACTIVE;
-		panic("ctscstart: no header mbuf");
-	len = m0->m_pkthdr.len;
 
-	/* Mark the interface busy. */
+		/* Chain; copy into linear buffer allocated at attach time. */
-	ifp->if_flags |= IFF_OACTIVE;
+		cp = sc->sc_tbuf;
-
+		for (m = m0; m != NULL; ) {
-	/* Chain; copy into linear buffer we allocated at attach time. */
+			memcpy(cp, mtod(m, u_char *), m->m_len);
-	cp = sc->sc_tbuf;
+			cp += m->m_len;
-	for (m = m0; m != NULL; ) {
+			m = m0 = m_free(m);
-		memcpy(cp, mtod(m, u_char *), m->m_len);
+		}
-		cp += m->m_len;
+		if (len < SEMINSIZE) {
-		m = m0 = m_free(m);
-	}
-	if (len < SEMINSIZE) {
 #ifdef SEDEBUG
-		if (sc->sc_debug)
+			if (sc->sc_debug)
-			printf("se: packet size %d (%zu) < %d\n", len,
+				printf("se: packet size %d (%zu) < %d\n", len,
-			    cp - (u_char *)sc->sc_tbuf, SEMINSIZE);
+				    cp - (u_char *)sc->sc_tbuf, SEMINSIZE);
 #endif
-		memset(cp, 0, SEMINSIZE - len);
+			memset(cp, 0, SEMINSIZE - len);
-		len = SEMINSIZE;
+			len = SEMINSIZE;
+		}
+
+		/* Fill out SCSI command. */
+		PROTOCMD(ctron_ether_send, send_cmd);
+		_lto2b(len, send_cmd.length);
+
+		/* Send command to device. */
+		error = se_scsipi_cmd(sc->sc_periph,
+		    (void *)&send_cmd, sizeof(send_cmd),
+		    sc->sc_tbuf, len, SERETRIES,
+		    SETIMEOUT, NULL, XS_CTL_NOSLEEP | XS_CTL_DATA_OUT);
+		if (error) {
+			aprint_error_dev(sc->sc_dev,
+			    "not queued, error %d\n", error);
+			if_statinc(ifp, if_oerrors);
+			ifp->if_flags &= ~IFF_OACTIVE;
+		} else
+			if_statinc(ifp, if_opackets);
 	}
-
-	/* Fill out SCSI command. */
-	PROTOCMD(ctron_ether_send, send_cmd);
-	_lto2b(len, send_cmd.length);
-
-	/* Send command to device. */
-	error = se_scsipi_cmd(sc->sc_periph,
-	    (void *)&send_cmd, sizeof(send_cmd),
-	    sc->sc_tbuf, len, SERETRIES,
-	    SETIMEOUT, NULL, XS_CTL_NOSLEEP | XS_CTL_DATA_OUT);
-	if (error) {
-		aprint_error_dev(sc->sc_dev, "not queued, error %d\n", error);
-		if_statinc(ifp, if_oerrors);
-		ifp->if_flags &= ~IFF_OACTIVE;
-	} else
-		if_statinc(ifp, if_opackets);
 }
 
 
@@ -500,16 +528,7 @@ sedone(struct scsipi_xfer *xs, int error)
 	struct ifnet *ifp = &sc->sc_ethercom.ec_if;
 
 	if (IS_SEND(cmd)) {
-		if (xs->error == XS_BUSY) {
+		ifp->if_flags &= ~IFF_OACTIVE;
-			printf("se: busy, retry txmit\n");
-			callout_reset(&sc->sc_ifstart_ch, hz,
-			    se_delayed_ifstart, ifp);
-		} else {
-			ifp->if_flags &= ~IFF_OACTIVE;
-			/* the generic scsipi_done will call
-			 * sestart (through scsipi_free_xs).
-			 */
-		}
 	} else if (IS_RECV(cmd)) {
 		/* RECV complete */
 		/* pass data up. reschedule a recv */
@@ -517,7 +536,7 @@ sedone(struct scsipi_xfer *xs, int error)
 		if (error) {
 			/* Reschedule after a delay */
 			callout_reset(&sc->sc_recv_ch, se_poll,
-			    se_recv, (void *)sc);
+			    se_recv_callout, (void *)sc);
 		} else {
 			int n, ntimeo;
 			n = se_read(sc, xs->data, xs->datalen - xs->resid);
@@ -539,22 +558,61 @@ sedone(struct scsipi_xfer *xs, int error)
 			}
 			sc->sc_last_timeout = ntimeo;
 			callout_reset(&sc->sc_recv_ch, ntimeo,
-			    se_recv, (void *)sc);
+			    se_recv_callout, (void *)sc);
 		}
 	}
 }
 
+/*
+ * Setup a receive command by queuing the work.
+ * Usually called from a callout, but also from se_init().
+ */
 static void
-se_recv(void *v)
+se_recv_callout(void *v)
 {
 	/* do a recv command */
 	struct se_softc *sc = (struct se_softc *) v;
-	struct scsi_ctron_ether_recv recv_cmd;
-	int error;
 
 	if (sc->sc_enabled == 0)
 		return;
 
+	mutex_enter(&sc->sc_iflock);
+	if (sc->sc_recv_work_pending == true) {
+		callout_reset(&sc->sc_recv_ch, se_poll,
+		    se_recv_callout, (void *)sc);
+		return;
+	}
+
+	sc->sc_recv_work_pending = true;
+	workqueue_enqueue(sc->sc_recv_wq, &sc->sc_recv_work, NULL);
+	mutex_exit(&sc->sc_iflock);
+}
+
+/*
+ * Invoke the receive workqueue
+ */
+static void
+se_recv_worker(struct work *wk, void *cookie)
+{
+	struct se_softc *sc = (struct se_softc *) cookie;
+
+	mutex_enter(&sc->sc_iflock);
+	sc->sc_recv_work_pending = false;
+	mutex_exit(&sc->sc_iflock);
+	se_recv(sc);
+
+}
+
+/*
+ * Do the actual work of receiving data.
+ */
+static void
+se_recv(struct se_softc *sc)
+{
+	struct scsi_ctron_ether_recv recv_cmd;
+	int error;
+
+	/* do a recv command */
 	PROTOCMD(ctron_ether_recv, recv_cmd);
 
 	error = se_scsipi_cmd(sc->sc_periph,
@@ -562,7 +620,8 @@ se_recv(void *v)
 	    sc->sc_rbuf, RBUF_LEN, SERETRIES, SETIMEOUT, NULL,
 	    XS_CTL_NOSLEEP | XS_CTL_DATA_IN);
 	if (error)
-		callout_reset(&sc->sc_recv_ch, se_poll, se_recv, (void *)sc);
+		callout_reset(&sc->sc_recv_ch, se_poll,
+		    se_recv_callout, (void *)sc);
 }
 
 /*
@@ -692,22 +751,19 @@ sewatchdog(struct ifnet *ifp)
 	se_reset(sc);
 }
 
-static int
+static void
 se_reset(struct se_softc *sc)
 {
-	int error;
-
 #if 0
 	/* Maybe we don't *really* want to reset the entire bus
 	 * because the ctron isn't working. We would like to send a
 	 * "BUS DEVICE RESET" message, but don't think the ctron
 	 * understands it.
 	 */
-	error = se_scsipi_cmd(sc->sc_periph, 0, 0, 0, 0, SERETRIES, 2000, NULL,
+	se_scsipi_cmd(sc->sc_periph, 0, 0, 0, 0, SERETRIES, 2000, NULL,
 	    XS_CTL_RESET);
 #endif
-	error = se_init(sc);
+	se_init(sc);
-	return (error);
 }
 
 static int
@@ -826,11 +882,14 @@ se_init(struct se_softc *sc)
 
 	if ((ifp->if_flags & (IFF_RUNNING | IFF_UP)) == IFF_UP) {
 		ifp->if_flags |= IFF_RUNNING;
-		se_recv(sc);
+		mutex_enter(&sc->sc_iflock);
+		sc->sc_recv_work_pending = true;
+		workqueue_enqueue(sc->sc_recv_wq, &sc->sc_recv_work, NULL);
+		mutex_exit(&sc->sc_iflock);
 		ifp->if_flags &= ~IFF_OACTIVE;
-		mutex_enter(chan_mtx(sc->sc_periph->periph_channel));
+		mutex_enter(&sc->sc_iflock);
-		se_ifstart(ifp);
+		workqueue_enqueue(sc->sc_send_wq, &sc->sc_send_work, NULL);
-		mutex_exit(chan_mtx(sc->sc_periph->periph_channel));
+		mutex_exit(&sc->sc_iflock);
 	}
 	return (error);
 }
@@ -846,13 +905,10 @@ se_set_multi(struct se_softc *sc, uint8_t *addr)
 		    ether_sprintf(addr));
 
 	PROTOCMD(ctron_ether_set_multi, set_multi_cmd);
-	_lto2b(sizeof(addr), set_multi_cmd.length);
+	_lto2b(ETHER_ADDR_LEN, set_multi_cmd.length);
-	/* XXX sizeof(addr) is the size of the pointer.  Surely it
-	 * is too small? --dyoung
-	 */
 	error = se_scsipi_cmd(sc->sc_periph,
 	    (void *)&set_multi_cmd, sizeof(set_multi_cmd),
-	    addr, sizeof(addr), SERETRIES, SETIMEOUT, NULL, XS_CTL_DATA_OUT);
+	    addr, ETHER_ADDR_LEN, SERETRIES, SETIMEOUT, NULL, XS_CTL_DATA_OUT);
 	return (error);
 }
 
@@ -867,13 +923,10 @@ se_remove_multi(struct se_softc *sc, uint8_t *addr)
 		    ether_sprintf(addr));
 
 	PROTOCMD(ctron_ether_remove_multi, remove_multi_cmd);
-	_lto2b(sizeof(addr), remove_multi_cmd.length);
+	_lto2b(ETHER_ADDR_LEN, remove_multi_cmd.length);
-	/* XXX sizeof(addr) is the size of the pointer.  Surely it
-	 * is too small? --dyoung
-	 */
 	error = se_scsipi_cmd(sc->sc_periph,
 	    (void *)&remove_multi_cmd, sizeof(remove_multi_cmd),
-	    addr, sizeof(addr), SERETRIES, SETIMEOUT, NULL, XS_CTL_DATA_OUT);
+	    addr, ETHER_ADDR_LEN, SERETRIES, SETIMEOUT, NULL, XS_CTL_DATA_OUT);
 	return (error);
 }
 
@@ -928,6 +981,12 @@ se_stop(struct se_softc *sc)
 	/* Don't schedule any reads */
 	callout_stop(&sc->sc_recv_ch);
 
+	/* Wait for the workqueues to finish */
+	mutex_enter(&sc->sc_iflock);
+	workqueue_wait(sc->sc_recv_wq, &sc->sc_recv_work);
+	workqueue_wait(sc->sc_send_wq, &sc->sc_send_work);
+	mutex_exit(&sc->sc_iflock);
+
 	/* Abort any scsi cmds in progress */
 	mutex_enter(chan_mtx(sc->sc_periph->periph_channel));
 	scsipi_kill_pending(sc->sc_periph);