[src/trunk]: src/sys usb: Hold pipe lock across upm_transfer and upm_start.

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/224d1bcd5a99
branches:  trunk
changeset: 362570:224d1bcd5a99
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Thu Mar 03 06:12:11 2022 +0000

description:
usb: Hold pipe lock across upm_transfer and upm_start.

This simplifies the code and fixes races with abort.  Access to the
pipe's queue is now done exclusively while the pipe is locked.

diffstat:

 sys/arch/mips/adm5120/dev/ahci.c    |  20 +++++++----
 sys/dev/ic/sl811hs.c                |  18 ++++------
 sys/dev/usb/ehci.c                  |  48 +++++++++--------------------
 sys/dev/usb/motg.c                  |  31 ++++++++----------
 sys/dev/usb/ohci.c                  |  47 +++++++++-------------------
 sys/dev/usb/uhci.c                  |  50 ++++++++++--------------------
 sys/dev/usb/usbdi.c                 |  11 ++----
 sys/dev/usb/usbdivar.h              |   8 ++--
 sys/dev/usb/usbroothub.c            |  13 +++++--
 sys/dev/usb/vhci.c                  |  20 +++--------
 sys/dev/usb/xhci.c                  |  61 +++++++++++-------------------------
 sys/external/bsd/dwc2/dwc2.c        |  47 +++++++---------------------
 sys/rump/dev/lib/libugenhc/ugenhc.c |  18 ++++------
 13 files changed, 142 insertions(+), 250 deletions(-)

diffs (truncated from 1438 to 300 lines):

diff -r 2e925d36ffd1 -r 224d1bcd5a99 sys/arch/mips/adm5120/dev/ahci.c
--- a/sys/arch/mips/adm5120/dev/ahci.c  Thu Mar 03 06:09:57 2022 +0000
+++ b/sys/arch/mips/adm5120/dev/ahci.c  Thu Mar 03 06:12:11 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ahci.c,v 1.29 2022/03/03 06:04:31 riastradh Exp $      */
+/*     $NetBSD: ahci.c,v 1.30 2022/03/03 06:12:11 riastradh Exp $      */
 
 /*-
  * Copyright (c) 2007 Ruslan Ermilov and Vsevolod Lobko.
@@ -64,7 +64,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ahci.c,v 1.29 2022/03/03 06:04:31 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ahci.c,v 1.30 2022/03/03 06:12:11 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -569,6 +569,8 @@
 
        DPRINTF(D_TRACE, ("SLRCstart "));
 
+       KASSERT(bus->ub_polling || mutex_owned(bus->ub_lock));
+
        len = UGETW(req->wLength);
        value = UGETW(req->wValue);
        index = UGETW(req->wIndex);
@@ -743,13 +745,13 @@
 
        DPRINTF(D_TRACE, ("SLRIstart "));
 
-       mutex_enter(&sc->sc_lock);
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock);
+
        KASSERT(sc->sc_intr_xfer == NULL);
        sc->sc_interval = MS_TO_TICKS(xfer->ux_pipe->up_endpoint->ue_edesc->bInterval);
        callout_schedule(&sc->sc_poll_handle, sc->sc_interval);
        sc->sc_intr_xfer = xfer;
        xfer->ux_status = USBD_IN_PROGRESS;
-       mutex_exit(&sc->sc_lock);
 
        return USBD_IN_PROGRESS;
 }
@@ -834,11 +836,11 @@
        struct ahci_softc *sc = AHCI_XFER2SC(xfer);
        int len, isread;
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock);
 
 #if 0
        struct ahci_pipe *apipe = (struct ahci_pipe *)xfer->ux_pipe;
 #endif
-       mutex_enter(&sc->sc_lock);
 /*     printf("ctrl_start>>>\n"); */
 
 #ifdef DIAGNOSTIC
@@ -968,7 +970,6 @@
 /*     printf("ctrl_start<<<\n"); */
 
        usb_transfer_complete(xfer);
-       mutex_exit(&sc->sc_lock);
 
        usb_freemem(&reqdma);
 
@@ -1006,11 +1007,14 @@
 static usbd_status
 ahci_device_intr_start(struct usbd_xfer *xfer)
 {
+       struct ahci_softc *sc = AHCI_XFER2SC(xfer);
        struct usbd_pipe *pipe = xfer->ux_pipe;
        struct ahci_xfer *sx;
 
        DPRINTF(D_TRACE, ("INTRstart "));
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock);
+
        sx = kmem_intr_alloc(sizeof(*sx), KM_NOSLEEP);
        if (sx == NULL)
                goto reterr;
@@ -1156,6 +1160,8 @@
 #define KSEG1ADDR(x) (0xa0000000 | (((uint32_t)x) & 0x1fffffff))
        DPRINTF(D_TRACE, ("st "));
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock);
+
 #ifdef DIAGNOSTIC
        if (xfer->ux_rqflags & URQ_REQUEST) {
                /* XXX panic */
@@ -1164,7 +1170,6 @@
        }
 #endif
 
-       mutex_enter(&sc->sc_lock);
        level++;
 /*     printf("bulk_start>>>\n"); */
 
@@ -1291,7 +1296,6 @@
                    isread ? BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
 
        usb_transfer_complete(xfer);
-       mutex_exit(&sc->sc_lock);
 
        return USBD_NORMAL_COMPLETION;
 }
diff -r 2e925d36ffd1 -r 224d1bcd5a99 sys/dev/ic/sl811hs.c
--- a/sys/dev/ic/sl811hs.c      Thu Mar 03 06:09:57 2022 +0000
+++ b/sys/dev/ic/sl811hs.c      Thu Mar 03 06:12:11 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: sl811hs.c,v 1.109 2022/03/03 06:04:31 riastradh Exp $  */
+/*     $NetBSD: sl811hs.c,v 1.110 2022/03/03 06:12:11 riastradh Exp $  */
 
 /*
  * Not (c) 2007 Matthew Orgass
@@ -68,7 +68,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: sl811hs.c,v 1.109 2022/03/03 06:04:31 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: sl811hs.c,v 1.110 2022/03/03 06:12:11 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_slhci.h"
@@ -846,10 +846,6 @@
            0);
 
        /* Pipe isn't running, so start it first.  */
-
-       /*
-        * Start will take the lock.
-        */
        error = xfer->ux_pipe->up_methods->upm_start(SIMPLEQ_FIRST(&xfer->ux_pipe->up_queue));
 
        return error;
@@ -867,7 +863,7 @@
        usb_endpoint_descriptor_t *ed = pipe->up_endpoint->ue_edesc;
        unsigned int max_packet;
 
-       mutex_enter(&sc->sc_lock);
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
 
        max_packet = UGETW(ed->wMaxPacketSize);
 
@@ -989,8 +985,6 @@
 
        slhci_start_entry(sc, spipe);
 
-       mutex_exit(&sc->sc_lock);
-
        return USBD_IN_PROGRESS;
 }
 
@@ -1012,13 +1006,13 @@
        DLOG(D_TRACE, "transfer type %jd start",
            SLHCI_XFER_TYPE(xfer), 0, 0, 0);
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
        KASSERT(spipe->ptype == PT_ROOT_INTR);
 
-       mutex_enter(&sc->sc_intr_lock);
        KASSERT(t->rootintr == NULL);
        t->rootintr = xfer;
        xfer->ux_status = USBD_IN_PROGRESS;
-       mutex_exit(&sc->sc_intr_lock);
 
        return USBD_IN_PROGRESS;
 }
@@ -3201,6 +3195,8 @@
        uint8_t type;
        int actlen = 0;
 
+       KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
+
        len = UGETW(req->wLength);
        value = UGETW(req->wValue);
        index = UGETW(req->wIndex);
diff -r 2e925d36ffd1 -r 224d1bcd5a99 sys/dev/usb/ehci.c
--- a/sys/dev/usb/ehci.c        Thu Mar 03 06:09:57 2022 +0000
+++ b/sys/dev/usb/ehci.c        Thu Mar 03 06:12:11 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ehci.c,v 1.304 2022/03/03 06:08:50 riastradh Exp $ */
+/*     $NetBSD: ehci.c,v 1.305 2022/03/03 06:12:11 riastradh Exp $ */
 
 /*
  * Copyright (c) 2004-2012,2016,2020 The NetBSD Foundation, Inc.
@@ -54,7 +54,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.304 2022/03/03 06:08:50 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.305 2022/03/03 06:12:11 riastradh Exp $");
 
 #include "ohci.h"
 #include "uhci.h"
@@ -2369,6 +2369,8 @@
 
        EHCIHIST_FUNC(); EHCIHIST_CALLED();
 
+       KASSERT(bus->ub_usepolling || mutex_owned(bus->ub_lock));
+
        if (sc->sc_dying)
                return -1;
 
@@ -2757,18 +2759,15 @@
 ehci_root_intr_start(struct usbd_xfer *xfer)
 {
        ehci_softc_t *sc = EHCI_XFER2SC(xfer);
-       const bool polling = sc->sc_bus.ub_usepolling;
+
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
 
        if (sc->sc_dying)
                return USBD_IOERROR;
 
-       if (!polling)
-               mutex_enter(&sc->sc_lock);
        KASSERT(sc->sc_intrxfer == NULL);
        sc->sc_intrxfer = xfer;
        xfer->ux_status = USBD_IN_PROGRESS;
-       if (!polling)
-               mutex_exit(&sc->sc_lock);
 
        return USBD_IN_PROGRESS;
 }
@@ -3607,10 +3606,10 @@
        ehci_softc_t *sc = EHCI_XFER2SC(xfer);
        ehci_soft_qtd_t *setup, *status, *next;
        ehci_soft_qh_t *sqh;
-       const bool polling = sc->sc_bus.ub_usepolling;
 
        EHCIHIST_FUNC(); EHCIHIST_CALLED();
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
        KASSERT(xfer->ux_rqflags & URQ_REQUEST);
 
        if (sc->sc_dying)
@@ -3729,16 +3728,11 @@
        DPRINTFN(5, "--- dump end ---", 0, 0, 0, 0);
 #endif
 
-       if (!polling)
-               mutex_enter(&sc->sc_lock);
-
        /* Insert qTD in QH list - also does usb_syncmem(sqh) */
        ehci_set_qh_qtd(sqh, setup);
        usbd_xfer_schedule_timeout(xfer);
        ehci_add_intr_list(sc, exfer);
        xfer->ux_status = USBD_IN_PROGRESS;
-       if (!polling)
-               mutex_exit(&sc->sc_lock);
 
 #if 0
 #ifdef EHCI_DEBUG
@@ -3878,13 +3872,14 @@
        ehci_soft_qh_t *sqh;
        ehci_soft_qtd_t *end;
        int len, isread, endpt;
-       const bool polling = sc->sc_bus.ub_usepolling;
 
        EHCIHIST_FUNC(); EHCIHIST_CALLED();
 
        DPRINTF("xfer=%#jx len=%jd flags=%jd", (uintptr_t)xfer, xfer->ux_length,
            xfer->ux_flags, 0);
 
+       KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
        if (sc->sc_dying)
                return USBD_IOERROR;
 
@@ -3901,10 +3896,6 @@
        exfer->ex_isdone = false;
 #endif
 
-       /* Take lock here to protect nexttoggle */
-       if (!polling)
-               mutex_enter(&sc->sc_lock);
-
        ehci_reset_sqtd_chain(sc, xfer, len, isread, &epipe->nexttoggle, &end);
 
        exfer->ex_sqtdend = end;
@@ -3928,8 +3919,6 @@
        usbd_xfer_schedule_timeout(xfer);
        ehci_add_intr_list(sc, exfer);
        xfer->ux_status = USBD_IN_PROGRESS;
-       if (!polling)
-               mutex_exit(&sc->sc_lock);
 
 #if 0
 #ifdef EHCI_DEBUG
@@ -4082,13 +4071,14 @@
        ehci_soft_qtd_t *end;
        ehci_soft_qh_t *sqh;
        int len, isread, endpt;
-       const bool polling = sc->sc_bus.ub_usepolling;
 
        EHCIHIST_FUNC(); EHCIHIST_CALLED();
 
        DPRINTF("xfer=%#jx len=%jd flags=%jd", (uintptr_t)xfer, xfer->ux_length,
            xfer->ux_flags, 0);