Rework zyd(4)'s register read/write methods to eliminate race conditions.

Read commands were issued via asynchronous transfers and replies were
expected after a fixed tsleep() timeout. Upon timeout zyd simply freed
the xfer even if it was still in-flight within the USB stack. This could
cause havoc such as making all USB ports on the system unusable until reboot.
  ehci_freex: xfer=0xfffffe811e63e9d8 not busy, 0x4f4e5155
  ("busy" here would indicate the xfer is done and marked for being freed)

To fix this, issue read commands with synchronous transfers so the xfer
can always complete. Split read/write code paths into separate methods.
Add a flag that tells us if a reply was received in interrupt context
while the read path waited in tsleep().

With and ok mpi@

diff --git a/sys/dev/usb/if_zyd.c b/sys/dev/usb/if_zyd.c
index 58f2d21..e2245dc 100644 (file)
--- a/sys/dev/usb/if_zyd.c
+++ b/sys/dev/usb/if_zyd.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_zyd.c,v 1.101 2014/07/12 07:59:23 mpi Exp $        */
+/*     $OpenBSD: if_zyd.c,v 1.102 2014/07/12 15:26:54 stsp Exp $       */
 
 /*-
  * Copyright (c) 2006 by Damien Bergamini <damien.bergamini@free.fr>
@@ -180,10 +180,10 @@ int               zyd_media_change(struct ifnet *);
 void           zyd_next_scan(void *);
 void           zyd_task(void *);
 int            zyd_newstate(struct ieee80211com *, enum ieee80211_state, int);
-int            zyd_cmd(struct zyd_softc *, uint16_t, const void *, int,
-                   void *, int, u_int);
+int            zyd_cmd_read(struct zyd_softc *, const void *, size_t, int);
 int            zyd_read16(struct zyd_softc *, uint16_t, uint16_t *);
 int            zyd_read32(struct zyd_softc *, uint16_t, uint32_t *);
+int            zyd_cmd_write(struct zyd_softc *, u_int16_t, const void *, int);
 int            zyd_write16(struct zyd_softc *, uint16_t, uint16_t);
 int            zyd_write32(struct zyd_softc *, uint16_t, uint32_t);
 int            zyd_rfwrite(struct zyd_softc *, uint32_t);
@@ -756,107 +756,134 @@ zyd_newstate(struct ieee80211com *ic, enum ieee80211_state nstate, int arg)
        return 0;
 }
 
+/*
+ * Issue a read command for the specificed register (of size regsize)
+ * and await a reply of olen bytes in sc->odata.
+ */
 int
-zyd_cmd(struct zyd_softc *sc, uint16_t code, const void *idata, int ilen,
-    void *odata, int olen, u_int flags)
+zyd_cmd_read(struct zyd_softc *sc, const void *reg, size_t regsize, int olen)
 {
        struct usbd_xfer *xfer;
        struct zyd_cmd cmd;
-       uint16_t xferflags;
        usbd_status error;
        int s;
 
        if ((xfer = usbd_alloc_xfer(sc->sc_udev)) == NULL)
                return ENOMEM;
 
-       cmd.code = htole16(code);
-       bcopy(idata, cmd.data, ilen);
-
-       xferflags = USBD_FORCE_SHORT_XFER;
-       if (!(flags & ZYD_CMD_FLAG_READ))
-               xferflags |= USBD_SYNCHRONOUS;
-       else
-               s = splusb();
+       bzero(&cmd, sizeof(cmd));
+       cmd.code = htole16(ZYD_CMD_IORD);
+       bcopy(reg, cmd.data, regsize);
 
-       sc->odata = odata;
-       sc->olen  = olen;
+       bzero(sc->odata, sizeof(sc->odata));
+       sc->olen = olen;
 
-       usbd_setup_xfer(xfer, sc->zyd_ep[ZYD_ENDPT_IOUT], 0, &cmd,
-           sizeof (uint16_t) + ilen, xferflags, ZYD_INTR_TIMEOUT, NULL);
+       usbd_setup_xfer(xfer, sc->zyd_ep[ZYD_ENDPT_IOUT], 0,
+           &cmd, sizeof(cmd.code) + regsize,
+           USBD_FORCE_SHORT_XFER | USBD_SYNCHRONOUS,
+           ZYD_INTR_TIMEOUT, NULL);
+       s = splusb();
+       sc->odone = 0;
        error = usbd_transfer(xfer);
-       if (error != USBD_IN_PROGRESS && error != 0) {
-               if (flags & ZYD_CMD_FLAG_READ)
-                       splx(s);
-               printf("%s: could not send command (error=%s)\n",
+       splx(s);
+       if (error) {
+               printf("%s: could not send command: %s\n",
                    sc->sc_dev.dv_xname, usbd_errstr(error));
                usbd_free_xfer(xfer);
                return EIO;
        }
-       if (!(flags & ZYD_CMD_FLAG_READ)) {
-               usbd_free_xfer(xfer);
-               return 0;       /* write: don't wait for reply */
-       }
-       /* wait at most one second for command reply */
-       error = tsleep(sc, PCATCH, "zydcmd", hz);
-       sc->odata = NULL;       /* in case answer is received too late */
-       splx(s);
 
+       if (!sc->odone) {
+               /* wait for ZYD_NOTIF_IORD interrupt */
+               if (tsleep(sc, PWAIT, "zydcmd", ZYD_INTR_TIMEOUT) != 0)
+                       printf("%s: read command failed\n",
+                           sc->sc_dev.dv_xname);
+       }
        usbd_free_xfer(xfer);
+
        return error;
 }
 
 int
 zyd_read16(struct zyd_softc *sc, uint16_t reg, uint16_t *val)
 {
-       struct zyd_pair tmp;
+       struct zyd_io *odata;
        int error;
 
        reg = htole16(reg);
-       error = zyd_cmd(sc, ZYD_CMD_IORD, &reg, sizeof reg, &tmp, sizeof tmp,
-           ZYD_CMD_FLAG_READ);
-       if (error == 0)
-               *val = letoh16(tmp.val);
+       error = zyd_cmd_read(sc, &reg, sizeof(reg), sizeof(*odata));
+       if (error == 0) {
+               odata = (struct zyd_io *)sc->odata;
+               *val = letoh16(odata[0].val);
+       }
        return error;
 }
 
 int
 zyd_read32(struct zyd_softc *sc, uint16_t reg, uint32_t *val)
 {
-       struct zyd_pair tmp[2];
+       struct zyd_io *odata;
        uint16_t regs[2];
        int error;
 
        regs[0] = htole16(ZYD_REG32_HI(reg));
        regs[1] = htole16(ZYD_REG32_LO(reg));
-       error = zyd_cmd(sc, ZYD_CMD_IORD, regs, sizeof regs, tmp, sizeof tmp,
-           ZYD_CMD_FLAG_READ);
-       if (error == 0)
-               *val = letoh16(tmp[0].val) << 16 | letoh16(tmp[1].val);
+       error = zyd_cmd_read(sc, regs, sizeof(regs), sizeof(*odata) * 2);
+       if (error == 0) {
+               odata = (struct zyd_io *)sc->odata;
+               *val = letoh16(odata[0].val) << 16 | letoh16(odata[1].val);
+       }
        return error;
 }
 
 int
-zyd_write16(struct zyd_softc *sc, uint16_t reg, uint16_t val)
+zyd_cmd_write(struct zyd_softc *sc, u_int16_t code, const void *data, int len)
 {
-       struct zyd_pair pair;
+       struct usbd_xfer *xfer;
+       struct zyd_cmd cmd;
+       usbd_status error;
+
+       if ((xfer = usbd_alloc_xfer(sc->sc_udev)) == NULL)
+               return ENOMEM;
+
+       bzero(&cmd, sizeof(cmd));
+       cmd.code = htole16(code);
+       bcopy(data, cmd.data, len);
+
+       usbd_setup_xfer(xfer, sc->zyd_ep[ZYD_ENDPT_IOUT], 0,
+           &cmd, sizeof(cmd.code) + len,
+           USBD_FORCE_SHORT_XFER | USBD_SYNCHRONOUS,
+           ZYD_INTR_TIMEOUT, NULL);
+       error = usbd_transfer(xfer);
+       if (error)
+               printf("%s: could not send command: %s\n",
+                   sc->sc_dev.dv_xname, usbd_errstr(error));
 
-       pair.reg = htole16(reg);
-       pair.val = htole16(val);
+       usbd_free_xfer(xfer);
+       return error;
+}
 
-       return zyd_cmd(sc, ZYD_CMD_IOWR, &pair, sizeof pair, NULL, 0, 0);
+int
+zyd_write16(struct zyd_softc *sc, uint16_t reg, uint16_t val)
+{ 
+       struct zyd_io io;
+
+       io.reg = htole16(reg);
+       io.val = htole16(val);
+       return zyd_cmd_write(sc, ZYD_CMD_IOWR, &io, sizeof(io));
 }
 
 int
 zyd_write32(struct zyd_softc *sc, uint16_t reg, uint32_t val)
 {
-       struct zyd_pair pair[2];
+       struct zyd_io io[2];
 
-       pair[0].reg = htole16(ZYD_REG32_HI(reg));
-       pair[0].val = htole16(val >> 16);
-       pair[1].reg = htole16(ZYD_REG32_LO(reg));
-       pair[1].val = htole16(val & 0xffff);
+       io[0].reg = htole16(ZYD_REG32_HI(reg));
+       io[0].val = htole16(val >> 16);
+       io[1].reg = htole16(ZYD_REG32_LO(reg));
+       io[1].val = htole16(val & 0xffff);
 
-       return zyd_cmd(sc, ZYD_CMD_IOWR, pair, sizeof pair, NULL, 0, 0);
+       return zyd_cmd_write(sc, ZYD_CMD_IOWR, io, sizeof(io));
 }
 
 int
@@ -877,7 +904,7 @@ zyd_rfwrite(struct zyd_softc *sc, uint32_t val)
                if (val & (1 << (rf->width - 1 - i)))
                        req.bit[i] |= htole16(ZYD_RF_DATA);
        }
-       return zyd_cmd(sc, ZYD_CMD_RFCFG, &req, 4 + 2 * rf->width, NULL, 0, 0);
+       return zyd_cmd_write(sc, ZYD_CMD_RFCFG, &req, 4 + 2 * rf->width);
 }
 
 void
@@ -1863,14 +1890,13 @@ zyd_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
                if (letoh16(*(uint16_t *)cmd->data) == ZYD_CR_INTERRUPT)
                        return; /* HMAC interrupt */
 
-               if (sc->odata == NULL)
-                       return; /* unexpected IORD notification */
-
-               /* copy answer into caller-supplied buffer */
-               usbd_get_xfer_status(xfer, NULL, NULL, &len, NULL);
-               bcopy(cmd->data, sc->odata, sc->olen);
-
-               wakeup(sc);     /* wakeup caller */
+               if (!sc->odone) {
+                       /* copy answer into sc->odata buffer */
+                       usbd_get_xfer_status(xfer, NULL, NULL, &len, NULL);
+                       bcopy(cmd->data, sc->odata, sc->olen);
+                       sc->odone = 1;
+                       wakeup(sc); /* wakeup zyd_cmd_read() */
+               }
 
        } else {
                printf("%s: unknown notification %x\n", sc->sc_dev.dv_xname,

diff --git a/sys/dev/usb/if_zydreg.h b/sys/dev/usb/if_zydreg.h
index 8368151..a228b24 100644 (file)
--- a/sys/dev/usb/if_zydreg.h
+++ b/sys/dev/usb/if_zydreg.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_zydreg.h,v 1.27 2013/11/26 20:33:19 deraadt Exp $  */
+/*     $OpenBSD: if_zydreg.h,v 1.28 2014/07/12 15:26:54 stsp Exp $     */
 
 /*-
  * Copyright (c) 2006 by Damien Bergamini <damien.bergamini@free.fr>
@@ -1056,11 +1056,12 @@ struct zyd_cmd {
 #define ZYD_NOTIF_MACINTR      0x9001  /* interrupt notification */
 #define ZYD_NOTIF_RETRYSTATUS  0xa001  /* Tx retry notification */
 
-       uint8_t         data[64];
+#define ZYD_MAX_DATA   64
+       uint8_t         data[ZYD_MAX_DATA];
 } __packed;
 
-/* structure for command ZYD_CMD_IOWR */
-struct zyd_pair {
+/* structure for command ZYD_CMD_IORD/ZYD_CMD_IOWR */
+struct zyd_io {
        uint16_t        reg;
 /* helpers macros to read/write 32-bit registers */
 #define ZYD_REG32_LO(reg)      (reg)
@@ -1194,8 +1195,9 @@ struct zyd_softc {
 
        struct ieee80211_amrr           amrr;
 
-       void                            *odata;
+       uint8_t                         odata[ZYD_MAX_DATA];
        int                             olen;
+       int                             odone;
 
        uint16_t                        fwbase;
        uint8_t                         regdomain;