Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/sys/dev/usb usb(4): Tighten interface locking and pipe refer...
details: https://anonhg.NetBSD.org/src/rev/599d0a09af6a
branches: trunk
changeset: 983876:599d0a09af6a
user: riastradh <riastradh%NetBSD.org@localhost>
date: Sun Jun 13 00:13:24 2021 +0000
description:
usb(4): Tighten interface locking and pipe references.
- Just use a reference count, not a list of pipes.
- Take the reference in usbd_open_pipe*, before we even look up the
endpoint by address; the endpoint is not stable until we hold the
interface and prevent usbd_set_interface.
- Make opening pipes just fail if usbd_set_interface is in progress.
=> No need to block -- might block for a while, and this is
essentially a driver error rather than a legitimate reason to
block.
=> This should maybe be a kassert, but it's not clear that ugen(4)
doesn't have a user-triggerable path to that kassert, so let's
keep it as a graceful failure for now until someone can audit
ugen(4) and make an informed decision.
- No need for a separate interface pipe lock; just use the bus lock.
This is a little bit longer than before, but makes the bracketed
nature of the references a little clearer and introduces more
kasserts to detect mistakes with internal API usage.
diffstat:
sys/dev/usb/usb_subr.c | 146 ++++++++++++++++++++++++++++++++++++++++--------
sys/dev/usb/usbdi.c | 65 +++++++++++++++------
sys/dev/usb/usbdivar.h | 11 ++-
3 files changed, 174 insertions(+), 48 deletions(-)
diffs (truncated from 415 to 300 lines):
diff -r 7b2bc42bffc7 -r 599d0a09af6a sys/dev/usb/usb_subr.c
--- a/sys/dev/usb/usb_subr.c Sun Jun 13 00:11:57 2021 +0000
+++ b/sys/dev/usb/usb_subr.c Sun Jun 13 00:13:24 2021 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: usb_subr.c,v 1.260 2021/06/12 15:49:45 riastradh Exp $ */
+/* $NetBSD: usb_subr.c,v 1.261 2021/06/13 00:13:24 riastradh Exp $ */
/* $FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma Exp $ */
/*
@@ -32,7 +32,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.260 2021/06/12 15:49:45 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.261 2021/06/13 00:13:24 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -415,8 +415,7 @@
ifc->ui_index = 0;
ifc->ui_altindex = 0;
ifc->ui_endpoints = NULL;
- LIST_INIT(&ifc->ui_pipes);
- mutex_init(&ifc->ui_pipelock, MUTEX_DEFAULT, IPL_NONE);
+ ifc->ui_busy = 0;
}
static void
@@ -429,9 +428,100 @@
KASSERT(ifc->ui_index == 0);
KASSERT(ifc->ui_altindex == 0);
KASSERT(ifc->ui_endpoints == NULL);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERTMSG(ifc->ui_busy == 0, "%"PRId64, ifc->ui_busy);
+}
+
+/*
+ * usbd_iface_lock/locked/unlock, usbd_iface_piperef/pipeunref
+ *
+ * We lock the interface while we are setting it, and we acquire a
+ * reference to the interface for each pipe opened on it.
+ *
+ * Setting the interface while pipes are open is forbidden, and
+ * opening pipes while the interface is being set is forbidden.
+ */
+
+bool
+usbd_iface_locked(struct usbd_interface *iface)
+{
+ bool locked;
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ locked = (iface->ui_busy == -1);
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return locked;
+}
+
+static void
+usbd_iface_exlock(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy == 0, "interface is not idle,"
+ " busy=%"PRId64, iface->ui_busy);
+ iface->ui_busy = -1;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+}
+
+usbd_status
+usbd_iface_lock(struct usbd_interface *iface)
+{
+ usbd_status err;
- mutex_destroy(&ifc->ui_pipelock);
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy != -1, "interface is locked");
+ KASSERTMSG(iface->ui_busy >= 0, "%"PRId64, iface->ui_busy);
+ if (iface->ui_busy) {
+ err = USBD_IN_USE;
+ } else {
+ iface->ui_busy = -1;
+ err = 0;
+ }
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return err;
+}
+
+void
+usbd_iface_unlock(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy == -1, "interface is not locked,"
+ " busy=%"PRId64, iface->ui_busy);
+ iface->ui_busy = 0;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+}
+
+usbd_status
+usbd_iface_piperef(struct usbd_interface *iface)
+{
+ usbd_status err;
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy >= -1, "%"PRId64, iface->ui_busy);
+ if (iface->ui_busy == -1) {
+ err = USBD_IN_USE;
+ } else {
+ iface->ui_busy++;
+ err = 0;
+ }
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
+
+ return err;
+}
+
+void
+usbd_iface_pipeunref(struct usbd_interface *iface)
+{
+
+ mutex_enter(iface->ui_dev->ud_bus->ub_lock);
+ KASSERTMSG(iface->ui_busy != -1, "interface is locked");
+ KASSERTMSG(iface->ui_busy != 0, "interface not in use");
+ KASSERTMSG(iface->ui_busy >= 1, "%"PRId64, iface->ui_busy);
+ iface->ui_busy--;
+ mutex_exit(iface->ui_dev->ud_bus->ub_lock);
}
usbd_status
@@ -447,7 +537,7 @@
int endpt, nendpt;
KASSERT(ifc->ui_dev == dev);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERT(usbd_iface_locked(ifc));
idesc = usbd_find_idesc(dev->ud_cdesc, ifaceidx, altidx);
if (idesc == NULL)
@@ -547,7 +637,7 @@
KASSERT(ifc->ui_dev == dev);
KASSERT(ifc->ui_idesc != NULL);
- KASSERT(LIST_EMPTY(&ifc->ui_pipes));
+ KASSERT(usbd_iface_locked(ifc));
if (ifc->ui_endpoints) {
int nendpt = ifc->ui_idesc->bNumEndpoints;
@@ -608,7 +698,9 @@
/* Free all configuration data structures. */
nifc = dev->ud_cdesc->bNumInterface;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces, nifc * sizeof(struct usbd_interface));
@@ -783,10 +875,14 @@
dev->ud_config = cdp->bConfigurationValue;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
usbd_iface_init(dev, ifcidx);
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
err = usbd_fill_iface_data(dev, ifcidx, 0);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
if (err) {
while (--ifcidx >= 0) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces,
@@ -827,12 +923,15 @@
USBHIST_FUNC();
USBHIST_CALLARGS(usbdebug, "dev=%#jx addr=%jd iface=%#jx ep=%#jx",
(uintptr_t)dev, dev->ud_addr, (uintptr_t)iface, (uintptr_t)ep);
- struct usbd_pipe *p;
+ struct usbd_pipe *p = NULL;
+ bool ep_acquired = false;
usbd_status err;
+ /* Block exclusive use of the endpoint by later pipes. */
err = usbd_endpoint_acquire(dev, ep, flags & USBD_EXCLUSIVE_USE);
if (err)
- return err;
+ goto out;
+ ep_acquired = true;
p = kmem_alloc(dev->ud_bus->ub_pipesize, KM_SLEEP);
DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0);
@@ -848,24 +947,11 @@
p->up_flags = flags;
SIMPLEQ_INIT(&p->up_queue);
- if (iface) {
- mutex_enter(&iface->ui_pipelock);
- LIST_INSERT_HEAD(&iface->ui_pipes, p, up_next);
- mutex_exit(&iface->ui_pipelock);
- }
-
err = dev->ud_bus->ub_methods->ubm_open(p);
if (err) {
DPRINTF("endpoint=%#jx failed, error=%jd",
(uintptr_t)ep->ue_edesc->bEndpointAddress, err, 0, 0);
- if (iface) {
- mutex_enter(&iface->ui_pipelock);
- LIST_REMOVE(p, up_next);
- mutex_exit(&iface->ui_pipelock);
- }
- kmem_free(p, dev->ud_bus->ub_pipesize);
- usbd_endpoint_release(dev, ep);
- return err;
+ goto out;
}
KASSERT(p->up_methods->upm_start || p->up_serialise == false);
@@ -874,7 +960,15 @@
USB_TASKQ_MPSAFE);
DPRINTFN(1, "pipe=%#jx", (uintptr_t)p, 0, 0, 0);
*pipe = p;
- return USBD_NORMAL_COMPLETION;
+ p = NULL; /* handed off to caller */
+ ep_acquired = false; /* handed off to pipe */
+ err = USBD_NORMAL_COMPLETION;
+
+out: if (p)
+ kmem_free(p, dev->ud_bus->ub_pipesize);
+ if (ep_acquired)
+ usbd_endpoint_release(dev, ep);
+ return err;
}
usbd_status
@@ -1712,7 +1806,9 @@
if (dev->ud_ifaces != NULL) {
nifc = dev->ud_cdesc->bNumInterface;
for (ifcidx = 0; ifcidx < nifc; ifcidx++) {
+ usbd_iface_exlock(&dev->ud_ifaces[ifcidx]);
usbd_free_iface_data(dev, ifcidx);
+ usbd_iface_unlock(&dev->ud_ifaces[ifcidx]);
usbd_iface_fini(dev, ifcidx);
}
kmem_free(dev->ud_ifaces,
diff -r 7b2bc42bffc7 -r 599d0a09af6a sys/dev/usb/usbdi.c
--- a/sys/dev/usb/usbdi.c Sun Jun 13 00:11:57 2021 +0000
+++ b/sys/dev/usb/usbdi.c Sun Jun 13 00:13:24 2021 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: usbdi.c,v 1.215 2021/06/12 15:49:45 riastradh Exp $ */
+/* $NetBSD: usbdi.c,v 1.216 2021/06/13 00:13:24 riastradh Exp $ */
/*
* Copyright (c) 1998, 2012, 2015 The NetBSD Foundation, Inc.
@@ -32,7 +32,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.215 2021/06/12 15:49:45 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usbdi.c,v 1.216 2021/06/13 00:13:24 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -225,6 +225,7 @@
{
struct usbd_pipe *p;
struct usbd_endpoint *ep;
+ bool piperef = false;
usbd_status err;
int i;
@@ -232,22 +233,49 @@
USBHIST_CALLARGS(usbdebug, "iface = %#jx address = %#jx flags = %#jx",
(uintptr_t)iface, address, flags, 0);
+ /*
+ * Block usbd_set_interface so we have a snapshot of the
+ * interface endpoints. They will remain stable until we drop
+ * the reference in usbd_close_pipe (or on failure here).
+ */
+ err = usbd_iface_piperef(iface);
+ if (err)
+ goto out;
+ piperef = true;
+
+ /* Find the endpoint at this address. */
for (i = 0; i < iface->ui_idesc->bNumEndpoints; i++) {
ep = &iface->ui_endpoints[i];
- if (ep->ue_edesc == NULL)
- return USBD_IOERROR;
+ if (ep->ue_edesc == NULL) {
+ err = USBD_IOERROR;
+ goto out;
+ }
if (ep->ue_edesc->bEndpointAddress == address)
- goto found;
+ break;
}
- return USBD_BAD_ADDRESS;
- found:
Home |
Main Index |
Thread Index |
Old Index