From 667989418c13ce4cd0fe0dd7c6f6991834cfe23b Mon Sep 17 00:00:00 2001 From: jdc Date: Mon, 22 Jun 2020 16:05:20 +0000 Subject: [PATCH] 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 | 315 ++++++++++++++++++++++++----------------- 1 file changed, 187 insertions(+), 128 deletions(-) diff --git a/sys/dev/scsipi/if_se.c b/sys/dev/scsipi/if_se.c index 56aea6694631..c224f85f553e 100644 --- a/sys/dev/scsipi/if_se.c +++ b/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 @@ -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 -__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 #include #include +#include #include #include @@ -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 */ - - se_get_addr(sc, myaddr); + sc->sc_rbuf = malloc(RBUF_LEN, M_DEVBUF, M_WAITOK); /* 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 - * 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 + * Start routine for calling from network sub system */ 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); - if (m0 == 0) - return; + while (1) { + IFQ_DEQUEUE(&ifp->if_snd, m0); + 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 - * packet before we commit it to the wire. - */ - bpf_mtap(ifp, m0, BPF_D_OUT); + /* We need to use m->m_pkthdr.len, so require the header */ + if ((m0->m_flags & M_PKTHDR) == 0) + panic("ctscstart: no header mbuf"); + len = m0->m_pkthdr.len; - /* We need to use m->m_pkthdr.len, so require the header */ - if ((m0->m_flags & M_PKTHDR) == 0) - panic("ctscstart: no header mbuf"); - len = m0->m_pkthdr.len; + /* Mark the interface busy. */ + ifp->if_flags |= IFF_OACTIVE; - /* Mark the interface busy. */ - ifp->if_flags |= IFF_OACTIVE; - - /* Chain; copy into linear buffer we allocated at attach time. */ - cp = sc->sc_tbuf; - for (m = m0; m != NULL; ) { - memcpy(cp, mtod(m, u_char *), m->m_len); - cp += m->m_len; - m = m0 = m_free(m); - } - if (len < SEMINSIZE) { + /* Chain; copy into linear buffer allocated at attach time. */ + cp = sc->sc_tbuf; + for (m = m0; m != NULL; ) { + memcpy(cp, mtod(m, u_char *), m->m_len); + cp += m->m_len; + m = m0 = m_free(m); + } + if (len < SEMINSIZE) { #ifdef SEDEBUG - if (sc->sc_debug) - printf("se: packet size %d (%zu) < %d\n", len, - cp - (u_char *)sc->sc_tbuf, SEMINSIZE); + if (sc->sc_debug) + printf("se: packet size %d (%zu) < %d\n", len, + cp - (u_char *)sc->sc_tbuf, SEMINSIZE); #endif - memset(cp, 0, SEMINSIZE - len); - len = SEMINSIZE; + memset(cp, 0, SEMINSIZE - len); + 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) { - 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). - */ - } + ifp->if_flags &= ~IFF_OACTIVE; } 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); - return (error); + se_init(sc); } 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)); - se_ifstart(ifp); - mutex_exit(chan_mtx(sc->sc_periph->periph_channel)); + mutex_enter(&sc->sc_iflock); + workqueue_enqueue(sc->sc_send_wq, &sc->sc_send_work, NULL); + 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); - /* XXX sizeof(addr) is the size of the pointer. Surely it - * is too small? --dyoung - */ + _lto2b(ETHER_ADDR_LEN, set_multi_cmd.length); 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); - /* XXX sizeof(addr) is the size of the pointer. Surely it - * is too small? --dyoung - */ + _lto2b(ETHER_ADDR_LEN, remove_multi_cmd.length); 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);