> Date: Wed, 10 May 2023 01:08:27 +0000
> From: Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net>
>
> I propose to add new config_*_acquire/release functions: [...]
Updated patch so that the legacy config_* operations require the
caller to hold the kernel lock, while the new config_*_acquire/release
ones do not (but take it internally until we make autoconf internals
MP-safe).
This way you can eliminate KERNEL_LOCKs around finding and detaching
children, without introducing races with concurrent drvctl -d:
// no KERNEL_LOCK(1, NULL);
dev = config_found_acquire(...);
mutex_enter(&sc->sc_childlock);
sc->sc_dev = dev;
mutex_exit(&sc->sc_childlock);
device_release(dev);
// no KERNEL_UNLOCK_ONE(NULL);
// no KERNEL_LOCK(1, NULL);
mutex_enter(&sc->sc_childlock);
if ((dev = sc->sc_dev) != NULL)
device_acquire(dev);
mutex_exit(&sc->sc_childlock);
config_detach_release(dev, ...);
// no KERNEL_UNLOCK_ONE(NULL);
Also included:
- Conversion of dk(4) to config_*_acquire/release -- lightly tested.
- Conversion of usb(4) to config_*_acquire/release -- compile-tested.
(Coming up is a series of other changes to fix related races in dk(4)
and simplify some of the logic. Also need to think about how devices
like vnd(4), cgd(4), &c., can safely work -- these logical drivers
sometimes do config_attach_pseudo in open, and sometimes do
config_detach in close, so their needs are a bit different from
drivers for physical hardware devices.)
>From baba80e17fbb254e50b242d71dc7e1de9bdc5520 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastr...@netbsd.org>
Date: Fri, 26 Aug 2022 10:35:08 +0000
Subject: [PATCH] autoconf(9): New functions for referenced attach/detach.
New functions:
- config_found_acquire(dev, aux, print, cfargs)
- config_attach_acquire(parent, cf, aux, print, cfargs)
- config_attach_pseudo_acquire(cf, aux)
- config_detach_release(dev, flags)
- device_acquire(dev)
The config_*_acquire functions are like the non-acquire versions, but
they return a referenced device_t, which is guaranteed to be safe to
use until released. The device's detach function may run while it is
referenced, but the device_t will not be freed and the parent's
.ca_childdetached routine will not be called.
=> config_attach_pseudo_acquire additionally lets you pass an aux
argument to the device's .ca_attach routine, unlike
config_attach_pseudo which always passes NULL.
=> Eventually, config_found, config_attach, and config_attach_pseudo
should be made to return void, because use of the device_t they
return is unsafe without the kernel lock and difficult to use
safely even with the kernel lock or in a UP system. For now,
they require the caller to hold the kernel lock, while
config_*_acquire do not.
config_detach_release is like device_release and then config_detach,
but avoids the race inherent with that sequence.
=> Eventually, config_detach should be eliminated, because getting at
the device_t it needs is unsafe without the kernel lock and
difficult to use safely even with the kernel lock or in a UP
system. For now, it requires the caller to hold the kernel lock,
while config_detach_release does not.
device_acquire acquires a reference to a device. It never fails and
can be used in thread context (but not interrupt context, hard or
soft). Caller is responsible for ensuring that the device_t cannot
be freed; in other words, the device_t must be made unavailable to
any device_acquire callers before the .ca_detach function returns.
Typically device_acquire will be used in a read section (mutex,
rwlock, pserialize, &c.) in a data structure lookup, with
corresponding logic in the .ca_detach function to remove the device
from the data structure and wait for all read sections to complete.
---
sys/kern/subr_autoconf.c | 185 ++++++++++++++++++++++++++++++++++++---
sys/sys/device.h | 7 ++
2 files changed, 181 insertions(+), 11 deletions(-)
diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c
index e60be2d7aad8..55c04ff8ca9e 100644
--- a/sys/kern/subr_autoconf.c
+++ b/sys/kern/subr_autoconf.c
@@ -1259,17 +1259,21 @@ static const char * const msgs[] = {
* not configured, call the given `print' function and return NULL.
*/
device_t
-config_found(device_t parent, void *aux, cfprint_t print,
+config_found_acquire(device_t parent, void *aux, cfprint_t print,
const struct cfargs * const cfargs)
{
cfdata_t cf;
struct cfargs_internal store;
const struct cfargs_internal * const args =
cfargs_canonicalize(cfargs, &store);
+ device_t dev;
+
+ KERNEL_LOCK(1, NULL);
cf = config_search_internal(parent, aux, args);
if (cf != NULL) {
- return config_attach_internal(parent, cf, aux, print, args);
+ dev = config_attach_internal(parent, cf, aux, print, args);
+ goto out;
}
if (print) {
@@ -1283,7 +1287,39 @@ config_found(device_t parent, void *aux, cfprint_t print,
aprint_normal("%s", msgs[pret]);
}
- return NULL;
+ dev = NULL;
+
+out: KERNEL_UNLOCK_ONE(NULL);
+ return dev;
+}
+
+/*
+ * config_found(parent, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_found_acquire with a matching device_release once they
+ * have finished with the returned device_t.
+ */
+device_t
+config_found(device_t parent, void *aux, cfprint_t print,
+ const struct cfargs * const cfargs)
+{
+ device_t dev;
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ dev = config_found_acquire(parent, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
}
/*
@@ -1708,6 +1744,8 @@ config_add_attrib_dict(device_t dev)
/*
* Attach a found device.
+ *
+ * Returns the device referenced, to be released with device_release.
*/
static device_t
config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t
print,
@@ -1778,6 +1816,12 @@ config_attach_internal(device_t parent, cfdata_t cf,
void *aux, cfprint_t print,
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
(*dev->dv_cfattach->ca_attach)(parent, dev, aux);
@@ -1813,15 +1857,47 @@ config_attach_internal(device_t parent, cfdata_t cf,
void *aux, cfprint_t print,
}
device_t
-config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+config_attach_acquire(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
const struct cfargs *cfargs)
{
struct cfargs_internal store;
+ device_t dev;
+
+ KERNEL_LOCK(1, NULL);
+ dev = config_attach_internal(parent, cf, aux, print,
+ cfargs_canonicalize(cfargs, &store));
+ KERNEL_UNLOCK_ONE(NULL);
+
+ return dev;
+}
+
+/*
+ * config_attach(parent, cf, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_attach_acquire with a matching device_release once they
+ * have finished with the returned device_t.
+ */
+device_t
+config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+ const struct cfargs *cfargs)
+{
+ device_t dev;
KASSERT(KERNEL_LOCKED_P());
- return config_attach_internal(parent, cf, aux, print,
- cfargs_canonicalize(cfargs, &store));
+ dev = config_attach_acquire(parent, cf, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
}
/*
@@ -1834,7 +1910,7 @@ config_attach(device_t parent, cfdata_t cf, void *aux,
cfprint_t print,
* name by the attach routine.
*/
device_t
-config_attach_pseudo(cfdata_t cf)
+config_attach_pseudo_acquire(cfdata_t cf, void *aux)
{
device_t dev;
@@ -1867,8 +1943,14 @@ config_attach_pseudo(cfdata_t cf)
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
- (*dev->dv_cfattach->ca_attach)(ROOT, dev, NULL);
+ (*dev->dv_cfattach->ca_attach)(ROOT, dev, aux);
/*
* Allow other threads to acquire references to the device now
@@ -1892,6 +1974,36 @@ out: KERNEL_UNLOCK_ONE(NULL);
return dev;
}
+/*
+ * config_attach_pseudo(cf)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should ignore the return value or be converted to
+ * config_attach_pseudo_acquire with a matching device_release
+ * once they have finished with the returned device_t. As a
+ * bonus, config_attach_pseudo_acquire can pass a non-null aux
+ * argument into the driver's attach routine.
+ */
+device_t
+config_attach_pseudo(cfdata_t cf)
+{
+ device_t dev;
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ dev = config_attach_pseudo_acquire(cf, NULL);
+ if (dev == NULL)
+ return dev;
+ device_release(dev);
+
+ return dev;
+}
+
/*
* Caller must hold alldevs_lock.
*/
@@ -2000,9 +2112,12 @@ config_detach_exit(device_t dev)
* Note that this code wants to be run from a process context, so
* that the detach can sleep to allow processes which have a device
* open to run and unwind their stacks.
+ *
+ * Caller must hold a reference with device_acquire or
+ * device_lookup_acquire.
*/
int
-config_detach(device_t dev, int flags)
+config_detach_release(device_t dev, int flags)
{
struct alldevs_foray af;
struct cftable *ct;
@@ -2031,6 +2146,7 @@ config_detach(device_t dev, int flags)
* attached.
*/
rv = config_detach_enter(dev);
+ device_release(dev);
if (rv) {
KERNEL_UNLOCK_ONE(NULL);
return rv;
@@ -2185,6 +2301,32 @@ out:
return rv;
}
+/*
+ * config_detach(dev, flags)
+ *
+ * Legacy entry point for callers that have not acquired a
+ * reference to dev.
+ *
+ * The caller is required to hold the kernel lock as a fragile
+ * defence against races.
+ *
+ * Callers should be converted to use device_acquire under a lock
+ * taken also by .ca_childdetached to synchronize access to the
+ * device_t, and then config_detach_release ouside the lock.
+ * Alternatively, most drivers detach children only in their own
+ * detach routines, which can be done with config_detach_children
+ * instead.
+ */
+int
+config_detach(device_t dev, int flags)
+{
+
+ KASSERT(KERNEL_LOCKED_P());
+
+ device_acquire(dev);
+ return config_detach_release(dev, flags);
+}
+
/*
* config_detach_commit(dev)
*
@@ -2740,7 +2882,7 @@ retry: if (unit < 0 || unit >= cd->cd_ndevs ||
mutex_enter(&alldevs_lock);
goto retry;
}
- localcount_acquire(dv->dv_localcount);
+ device_acquire(dv);
}
mutex_exit(&alldevs_lock);
mutex_exit(&config_misc_lock);
@@ -2748,10 +2890,31 @@ retry: if (unit < 0 || unit >= cd->cd_ndevs ||
return dv;
}
+/*
+ * device_acquire:
+ *
+ * Acquire a reference to a device. It is the caller's
+ * responsibility to ensure that the device's .ca_detach routine
+ * cannot return before calling this. Caller must release the
+ * reference with device_release or config_detach_release.
+ */
+void
+device_acquire(device_t dv)
+{
+
+ /*
+ * No lock because the caller has promised that this can't
+ * change concurrently with device_acquire.
+ */
+ KASSERTMSG(!dv->dv_detach_done, "%s",
+ dv == NULL ? "(null)" : device_xname(dv));
+ localcount_acquire(dv->dv_localcount);
+}
+
/*
* device_release:
*
- * Release a reference to a device acquired with
+ * Release a reference to a device acquired with device_acquire or
* device_lookup_acquire.
*/
void
diff --git a/sys/sys/device.h b/sys/sys/device.h
index 4f47e063d0a6..91d994a9ff88 100644
--- a/sys/sys/device.h
+++ b/sys/sys/device.h
@@ -554,14 +554,20 @@ device_t config_found(device_t, void *, cfprint_t, const
struct cfargs *);
device_t config_rootfound(const char *, void *);
device_t config_attach(device_t, cfdata_t, void *, cfprint_t,
const struct cfargs *);
+device_t config_found_acquire(device_t, void *, cfprint_t,
+ const struct cfargs *);
+device_t config_attach_acquire(device_t, cfdata_t, void *, cfprint_t,
+ const struct cfargs *);
int config_match(device_t, cfdata_t, void *);
int config_probe(device_t, cfdata_t, void *);
bool ifattr_match(const char *, const char *);
device_t config_attach_pseudo(cfdata_t);
+device_t config_attach_pseudo_acquire(cfdata_t, void *);
int config_detach(device_t, int);
+int config_detach_release(device_t, int);
int config_detach_children(device_t, int flags);
void config_detach_commit(device_t);
bool config_detach_all(int);
@@ -588,6 +594,7 @@ device_t device_lookup(cfdriver_t, int);
void *device_lookup_private(cfdriver_t, int);
device_t device_lookup_acquire(cfdriver_t, int);
+void device_acquire(device_t);
void device_release(device_t);
void device_register(device_t, void *);
>From 4b990a07a20cef83d21eb1619ca3463e936ed634 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastr...@netbsd.org>
Date: Sat, 29 Apr 2023 13:16:02 +0000
Subject: [PATCH] dk(4): Use config_attach_pseudo_acquire to create wedges.
This way, indexing of the dkwedges array coincides with numbering of
autoconf dk(4) instances.
As a side effect, this plugs a race in dkwedge_add with concurrent
drvctl -r. There are a lot of such races in dk(4) left -- to be
addressed with more device references.
---
sys/dev/dkwedge/dk.c | 91 ++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 38 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index 48bfe907fb86..764a2bf1d87f 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -99,6 +99,8 @@ static int dkwedge_match(device_t, cfdata_t, void *);
static void dkwedge_attach(device_t, device_t, void *);
static int dkwedge_detach(device_t, int);
+static void dk_set_geometry(struct dkwedge_softc *, struct disk *);
+
static void dkstart(struct dkwedge_softc *);
static void dkiodone(struct buf *);
static void dkrestart(void *);
@@ -190,9 +192,34 @@ dkwedge_match(device_t parent, cfdata_t match, void *aux)
static void
dkwedge_attach(device_t parent, device_t self, void *aux)
{
+ struct dkwedge_softc *sc = aux;
+ struct disk *pdk = sc->sc_parent;
+ int unit = device_unit(self);
+
+ KASSERTMSG(unit >= 0, "unit=%d", unit);
if (!pmf_device_register(self, NULL, NULL))
aprint_error_dev(self, "couldn't establish power handler\n");
+
+ mutex_enter(&pdk->dk_openlock);
+ rw_enter(&dkwedges_lock, RW_WRITER);
+ KASSERTMSG(unit < ndkwedges, "unit=%d ndkwedges=%u", unit, ndkwedges);
+ KASSERTMSG(sc == dkwedges[unit], "sc=%p dkwedges[%d]=%p",
+ sc, unit, dkwedges[unit]);
+ KASSERTMSG(sc->sc_dev == NULL, "sc=%p sc->sc_dev=%p", sc, sc->sc_dev);
+ sc->sc_dev = self;
+ rw_exit(&dkwedges_lock);
+ mutex_exit(&pdk->dk_openlock);
+
+ disk_init(&sc->sc_dk, device_xname(sc->sc_dev), NULL);
+ mutex_enter(&pdk->dk_openlock);
+ dk_set_geometry(sc, pdk);
+ mutex_exit(&pdk->dk_openlock);
+ disk_attach(&sc->sc_dk);
+
+ /* Disk wedge is ready for use! */
+ device_set_private(self, sc);
+ sc->sc_state = DKW_STATE_RUNNING;
}
/*
@@ -364,6 +391,7 @@ dkwedge_add(struct dkwedge_info *dkw)
u_int unit;
int error;
dev_t pdev;
+ device_t dev __diagused;
dkw->dkw_parent[sizeof(dkw->dkw_parent) - 1] = '\0';
pdk = disk_find(dkw->dkw_parent);
@@ -393,8 +421,11 @@ dkwedge_add(struct dkwedge_info *dkw)
break;
if (dkwedge_size(lsc) > dkw->dkw_size)
break;
+ if (lsc->sc_dev == NULL)
+ break;
sc = lsc;
+ device_acquire(sc->sc_dev);
dkwedge_size_increase(sc, dkw->dkw_size);
dk_set_geometry(sc, pdk);
@@ -477,7 +508,7 @@ dkwedge_add(struct dkwedge_info *dkw)
sc->sc_cfdata.cf_name = dk_cd.cd_name;
sc->sc_cfdata.cf_atname = dk_ca.ca_name;
/* sc->sc_cfdata.cf_unit set below */
- sc->sc_cfdata.cf_fstate = FSTATE_STAR;
+ sc->sc_cfdata.cf_fstate = FSTATE_NOTFOUND; /* use chosen cf_unit */
/* Insert the larval wedge into the array. */
rw_enter(&dkwedges_lock, RW_WRITER);
@@ -538,7 +569,7 @@ dkwedge_add(struct dkwedge_info *dkw)
* This should never fail, unless we're almost totally out of
* memory.
*/
- if ((sc->sc_dev = config_attach_pseudo(&sc->sc_cfdata)) == NULL) {
+ if ((dev = config_attach_pseudo_acquire(&sc->sc_cfdata, sc)) == NULL) {
aprint_error("%s%u: unable to attach pseudo-device\n",
sc->sc_cfdata.cf_name, sc->sc_cfdata.cf_unit);
@@ -559,19 +590,7 @@ dkwedge_add(struct dkwedge_info *dkw)
return ENOMEM;
}
- /*
- * XXX Really ought to make the disk_attach() and the changing
- * of state to RUNNING atomic.
- */
-
- disk_init(&sc->sc_dk, device_xname(sc->sc_dev), NULL);
- mutex_enter(&pdk->dk_openlock);
- dk_set_geometry(sc, pdk);
- mutex_exit(&pdk->dk_openlock);
- disk_attach(&sc->sc_dk);
-
- /* Disk wedge is ready for use! */
- sc->sc_state = DKW_STATE_RUNNING;
+ KASSERT(dev == sc->sc_dev);
announce:
/* Announce our arrival. */
@@ -586,11 +605,12 @@ announce:
strlcpy(dkw->dkw_devname, device_xname(sc->sc_dev),
sizeof(dkw->dkw_devname));
+ device_release(sc->sc_dev);
return 0;
}
/*
- * dkwedge_find:
+ * dkwedge_find_acquire:
*
* Lookup a disk wedge based on the provided information.
* NOTE: We look up the wedge based on the wedge devname,
@@ -598,10 +618,11 @@ announce:
*
* Return NULL if the wedge is not found, otherwise return
* the wedge's softc. Assign the wedge's unit number to unitp
- * if unitp is not NULL.
+ * if unitp is not NULL. The wedge's sc_dev is referenced and
+ * must be released by device_release or equivalent.
*/
static struct dkwedge_softc *
-dkwedge_find(struct dkwedge_info *dkw, u_int *unitp)
+dkwedge_find_acquire(struct dkwedge_info *dkw, u_int *unitp)
{
struct dkwedge_softc *sc = NULL;
u_int unit;
@@ -611,8 +632,10 @@ dkwedge_find(struct dkwedge_info *dkw, u_int *unitp)
rw_enter(&dkwedges_lock, RW_READER);
for (unit = 0; unit < ndkwedges; unit++) {
if ((sc = dkwedges[unit]) != NULL &&
+ sc->sc_dev != NULL &&
strcmp(device_xname(sc->sc_dev), dkw->dkw_devname) == 0 &&
strcmp(sc->sc_parent->dk_name, dkw->dkw_parent) == 0) {
+ device_acquire(sc->sc_dev);
break;
}
}
@@ -646,10 +669,10 @@ dkwedge_del1(struct dkwedge_info *dkw, int flags)
struct dkwedge_softc *sc = NULL;
/* Find our softc. */
- if ((sc = dkwedge_find(dkw, NULL)) == NULL)
+ if ((sc = dkwedge_find_acquire(dkw, NULL)) == NULL)
return ESRCH;
- return config_detach(sc->sc_dev, flags);
+ return config_detach_release(sc->sc_dev, flags);
}
/*
@@ -660,26 +683,16 @@ dkwedge_del1(struct dkwedge_info *dkw, int flags)
static int
dkwedge_detach(device_t self, int flags)
{
- struct dkwedge_softc *sc = NULL;
- u_int unit;
- int bmaj, cmaj, rc;
+ struct dkwedge_softc *const sc = device_private(self);
+ const u_int unit = device_unit(self);
+ int bmaj, cmaj, error;
- rw_enter(&dkwedges_lock, RW_WRITER);
- for (unit = 0; unit < ndkwedges; unit++) {
- if ((sc = dkwedges[unit]) != NULL && sc->sc_dev == self)
- break;
- }
- if (unit == ndkwedges)
- rc = ENXIO;
- else if ((rc = disk_begindetach(&sc->sc_dk, /*lastclose*/NULL, self,
- flags)) == 0) {
- /* Mark the wedge as dying. */
- sc->sc_state = DKW_STATE_DYING;
- }
- rw_exit(&dkwedges_lock);
+ error = disk_begindetach(&sc->sc_dk, /*lastclose*/NULL, self, flags);
+ if (error)
+ return error;
- if (rc != 0)
- return rc;
+ /* Mark the wedge as dying. */
+ sc->sc_state = DKW_STATE_DYING;
pmf_device_deregister(self);
@@ -1147,6 +1160,8 @@ dkwedge_read(struct disk *pdk, struct vnode *vp, daddr_t
blkno,
* dkwedge_lookup:
*
* Look up a dkwedge_softc based on the provided dev_t.
+ *
+ * Caller must guarantee the wedge is referenced.
*/
static struct dkwedge_softc *
dkwedge_lookup(dev_t dev)
>From e28162e9479a86afa1b60c13f4aac2ed9b01a9e1 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastr...@netbsd.org>
Date: Wed, 10 May 2023 00:20:24 +0000
Subject: [PATCH] usb(4): Use config_found_acquire/config_detach_release.
New lock ud_subdevlock serializes access to ud_subdevs and
ud_subdevlen. Rules:
- ud_subdevlock to read or write ud_subdevs[i].
- ud_subdevlock, in the event thread only, to write ud_subdevs or
ud_subdevlen.
- ud_subdevlock or event thread to read ud_subdevs or ud_subdevlen.
---
sys/dev/usb/uhub.c | 17 +++++++--
sys/dev/usb/usb.c | 4 +-
sys/dev/usb/usb_subr.c | 84 +++++++++++++++++++++++++++++++-----------
sys/dev/usb/usbdivar.h | 1 +
sys/dev/usb/xhci.c | 1 +
5 files changed, 82 insertions(+), 25 deletions(-)
diff --git a/sys/dev/usb/uhub.c b/sys/dev/usb/uhub.c
index ffc81ee162ad..158fd9208fc6 100644
--- a/sys/dev/usb/uhub.c
+++ b/sys/dev/usb/uhub.c
@@ -1002,9 +1002,13 @@ uhub_childdet(device_t self, device_t child)
nports = devhub->ud_hub->uh_hubdesc.bNbrPorts;
for (port = 1; port <= nports; port++) {
+ device_t *subdevs = NULL;
+ unsigned subdevlen = 0;
+
dev = devhub->ud_hub->uh_ports[port - 1].up_dev;
+ mutex_enter(&dev->ud_subdevlock);
if (!dev || dev->ud_subdevlen == 0)
- continue;
+ goto next;
for (i = 0; i < dev->ud_subdevlen; i++) {
if (dev->ud_subdevs[i] == child) {
dev->ud_subdevs[i] = NULL;
@@ -1012,11 +1016,18 @@ uhub_childdet(device_t self, device_t child)
}
}
if (dev->ud_nifaces_claimed == 0) {
- kmem_free(dev->ud_subdevs,
- dev->ud_subdevlen * sizeof(device_t));
+ subdevs = dev->ud_subdevs;
+ subdevlen = dev->ud_subdevlen;
dev->ud_subdevs = NULL;
dev->ud_subdevlen = 0;
}
+next: mutex_exit(&dev->ud_subdevlock);
+
+ if (subdevs) {
+ kmem_free(subdevs, subdevlen * sizeof(subdevs[0]));
+ subdevs = NULL;
+ subdevlen = 0;
+ }
}
}
diff --git a/sys/dev/usb/usb.c b/sys/dev/usb/usb.c
index ac93cda2a576..43e373d7c520 100644
--- a/sys/dev/usb/usb.c
+++ b/sys/dev/usb/usb.c
@@ -1386,12 +1386,14 @@ usb_childdet(device_t self, device_t child)
struct usb_softc *sc = device_private(self);
struct usbd_device *dev;
- if ((dev = sc->sc_port.up_dev) == NULL || dev->ud_subdevlen == 0)
+ if ((dev = sc->sc_port.up_dev) == NULL)
return;
+ mutex_enter(&dev->ud_subdevlock);
for (i = 0; i < dev->ud_subdevlen; i++)
if (dev->ud_subdevs[i] == child)
dev->ud_subdevs[i] = NULL;
+ mutex_exit(&dev->ud_subdevlock);
}
int
diff --git a/sys/dev/usb/usb_subr.c b/sys/dev/usb/usb_subr.c
index e14fec7e30be..029c0558f30f 100644
--- a/sys/dev/usb/usb_subr.c
+++ b/sys/dev/usb/usb_subr.c
@@ -1047,6 +1047,8 @@ usbd_attach_roothub(device_t parent, struct usbd_device
*dev)
usb_device_descriptor_t *dd = &dev->ud_ddesc;
device_t dv;
+ KASSERT(usb_in_event_thread(parent));
+
uaa.uaa_device = dev;
uaa.uaa_usegeneric = 0;
uaa.uaa_port = 0;
@@ -1057,14 +1059,18 @@ usbd_attach_roothub(device_t parent, struct usbd_device
*dev)
uaa.uaa_subclass = dd->bDeviceSubClass;
uaa.uaa_proto = dd->bDeviceProtocol;
- KERNEL_LOCK(1, curlwp);
- dv = config_found(parent, &uaa, NULL,
+ dv = config_found_acquire(parent, &uaa, NULL,
CFARGS(.iattr = "usbroothubif"));
- KERNEL_UNLOCK_ONE(curlwp);
if (dv) {
- dev->ud_subdevs = kmem_alloc(sizeof(dv), KM_SLEEP);
+ device_t *subdevs = kmem_alloc(sizeof(subdevs[0]), KM_SLEEP);
+
+ mutex_enter(&dev->ud_subdevlock);
+ dev->ud_subdevs = subdevs;
dev->ud_subdevs[0] = dv;
dev->ud_subdevlen = 1;
+ mutex_exit(&dev->ud_subdevlock);
+
+ device_release(dv);
}
return USBD_NORMAL_COMPLETION;
}
@@ -1135,19 +1141,24 @@ usbd_attachwholedevice(device_t parent, struct
usbd_device *dev, int port,
config_pending_incr(parent);
- KERNEL_LOCK(1, curlwp);
- dv = config_found(parent, &uaa, usbd_print,
+ dv = config_found_acquire(parent, &uaa, usbd_print,
CFARGS(.submatch = config_stdsubmatch,
.iattr = "usbdevif",
.locators = dlocs));
- KERNEL_UNLOCK_ONE(curlwp);
if (dv) {
- dev->ud_subdevs = kmem_alloc(sizeof(dv), KM_SLEEP);
+ device_t *subdevs = kmem_alloc(sizeof(subdevs[0]), KM_SLEEP);
+
+ mutex_enter(&dev->ud_subdevlock);
+ dev->ud_subdevs = subdevs;
dev->ud_subdevs[0] = dv;
dev->ud_subdevlen = 1;
dev->ud_nifaces_claimed = 1; /* XXX */
+ mutex_exit(&dev->ud_subdevlock);
+
usbd_properties(dv, dev);
+ device_release(dv);
}
+
config_pending_decr(parent);
return USBD_NORMAL_COMPLETION;
}
@@ -1169,13 +1180,14 @@ usbd_attachinterfaces(device_t parent, struct
usbd_device *dev,
nifaces = dev->ud_cdesc->bNumInterface;
ifaces = kmem_zalloc(nifaces * sizeof(*ifaces), KM_SLEEP);
+ mutex_enter(&dev->ud_subdevlock);
for (i = 0; i < nifaces; i++) {
if (!dev->ud_subdevs[i]) {
ifaces[i] = &dev->ud_ifaces[i];
}
DPRINTF("interface %jd %#jx", i, (uintptr_t)ifaces[i], 0, 0);
}
-
+ mutex_exit(&dev->ud_subdevlock);
uiaa.uiaa_device = dev;
uiaa.uiaa_port = port;
@@ -1217,12 +1229,10 @@ usbd_attachinterfaces(device_t parent, struct
usbd_device *dev,
loc != uiaa.uiaa_ifaceno)
continue;
}
- KERNEL_LOCK(1, curlwp);
- dv = config_found(parent, &uiaa, usbd_ifprint,
+ dv = config_found_acquire(parent, &uiaa, usbd_ifprint,
CFARGS(.submatch = config_stdsubmatch,
.iattr = "usbifif",
.locators = ilocs));
- KERNEL_UNLOCK_ONE(curlwp);
if (!dv)
continue;
@@ -1232,14 +1242,16 @@ usbd_attachinterfaces(device_t parent, struct
usbd_device *dev,
ifaces[i] = NULL;
/* account for ifaces claimed by the driver behind our back */
for (j = 0; j < nifaces; j++) {
-
+ mutex_enter(&dev->ud_subdevlock);
if (!ifaces[j] && !dev->ud_subdevs[j]) {
DPRINTF("interface %jd claimed behind our back",
j, 0, 0, 0);
dev->ud_subdevs[j] = dv;
dev->ud_nifaces_claimed++;
}
+ mutex_exit(&dev->ud_subdevlock);
}
+ device_release(dv);
}
kmem_free(ifaces, nifaces * sizeof(*ifaces));
@@ -1253,6 +1265,8 @@ usbd_probe_and_attach(device_t parent, struct usbd_device
*dev,
USBHIST_FUNC();
USBHIST_CALLARGS(usbdebug, "trying device specific drivers", 0, 0, 0,
0);
usb_device_descriptor_t *dd = &dev->ud_ddesc;
+ device_t *subdevs;
+ unsigned subdevlen;
int confi, nifaces;
usbd_status err;
@@ -1277,18 +1291,30 @@ usbd_probe_and_attach(device_t parent, struct
usbd_device *dev,
return err;
}
nifaces = dev->ud_cdesc->bNumInterface;
- dev->ud_subdevs = kmem_zalloc(nifaces * sizeof(device_t),
- KM_SLEEP);
+
+ subdevs = kmem_zalloc(nifaces * sizeof(subdevs[0]), KM_SLEEP);
+
+ mutex_enter(&dev->ud_subdevlock);
+ KASSERT(dev->ud_subdevs == NULL);
+ dev->ud_subdevs = subdevs;
+ subdevs = NULL;
dev->ud_subdevlen = nifaces;
+ mutex_exit(&dev->ud_subdevlock);
err = usbd_attachinterfaces(parent, dev, port, NULL);
+ mutex_enter(&dev->ud_subdevlock);
if (dev->ud_subdevs && dev->ud_nifaces_claimed == 0) {
- kmem_free(dev->ud_subdevs,
- dev->ud_subdevlen * sizeof(device_t));
+ subdevs = dev->ud_subdevs;
+ subdevlen = dev->ud_subdevlen;
dev->ud_subdevs = 0;
dev->ud_subdevlen = 0;
}
+ mutex_exit(&dev->ud_subdevlock);
+ if (subdevs) {
+ kmem_free(subdevs, subdevlen * sizeof(subdevs[0]));
+ subdevs = NULL;
+ }
if (dev->ud_nifaces_claimed || err)
return err;
}
@@ -1354,11 +1380,13 @@ usbd_reattach_device(device_t parent, struct
usbd_device *dev,
return USBD_NORMAL_COMPLETION;
}
/* Does the device have unconfigured interfaces? */
+ mutex_enter(&dev->ud_subdevlock);
for (i = 0; i < dev->ud_subdevlen; i++) {
if (dev->ud_subdevs[i] == NULL) {
break;
}
}
+ mutex_exit(&dev->ud_subdevlock);
if (i >= dev->ud_subdevlen)
return USBD_NORMAL_COMPLETION;
return usbd_attachinterfaces(parent, dev, port, locators);
@@ -1401,6 +1429,7 @@ usbd_new_device(device_t parent, struct usbd_bus *bus,
int depth, int speed,
dev = kmem_zalloc(sizeof(*dev), KM_SLEEP);
dev->ud_bus = bus;
+ mutex_init(&dev->ud_subdevlock, MUTEX_DEFAULT, IPL_NONE);
/* Set up default endpoint handle. */
dev->ud_ep0.ue_edesc = &dev->ud_ep0desc;
@@ -1652,6 +1681,11 @@ usbd_remove_device(struct usbd_device *dev, struct
usbd_port *up)
up->up_dev = NULL;
dev->ud_bus->ub_devices[usb_addr2dindex(dev->ud_addr)] = NULL;
+ KASSERTMSG(dev->ud_subdevlen == 0,
+ "usbd_device %p [addr %"PRIu8"] still has %u subdevs",
+ dev, dev->ud_addr, dev->ud_subdevlen);
+ KASSERT(dev->ud_subdevs == NULL);
+
if (dev->ud_vendor != NULL) {
kmem_free(dev->ud_vendor, USB_MAX_ENCODED_STRING_LEN);
}
@@ -1661,6 +1695,7 @@ usbd_remove_device(struct usbd_device *dev, struct
usbd_port *up)
if (dev->ud_serial != NULL) {
kmem_free(dev->ud_serial, USB_MAX_ENCODED_STRING_LEN);
}
+ mutex_destroy(&dev->ud_subdevlock);
kmem_free(dev, sizeof(*dev));
}
@@ -1762,19 +1797,22 @@ usbd_fill_deviceinfo(struct usbd_device *dev, struct
usb_device_info *di,
di->udi_power = dev->ud_selfpowered ? 0 : dev->ud_power;
di->udi_speed = dev->ud_speed;
+ mutex_enter(&dev->ud_subdevlock);
if (dev->ud_subdevlen > 0) {
for (i = 0, j = 0; i < dev->ud_subdevlen &&
j < USB_MAX_DEVNAMES; i++) {
if (!dev->ud_subdevs[i])
continue;
strncpy(di->udi_devnames[j],
- device_xname(dev->ud_subdevs[i]),
USB_MAX_DEVNAMELEN);
+ device_xname(dev->ud_subdevs[i]),
+ USB_MAX_DEVNAMELEN);
di->udi_devnames[j][USB_MAX_DEVNAMELEN-1] = '\0';
j++;
}
} else {
j = 0;
}
+ mutex_exit(&dev->ud_subdevlock);
for (/* j is set */; j < USB_MAX_DEVNAMES; j++)
di->udi_devnames[j][0] = 0; /* empty */
@@ -1848,6 +1886,7 @@ usb_free_device(struct usbd_device *dev)
if (dev->ud_serial) {
kmem_free(dev->ud_serial, USB_MAX_ENCODED_STRING_LEN);
}
+ mutex_destroy(&dev->ud_subdevlock);
kmem_free(dev, sizeof(*dev));
}
@@ -1886,25 +1925,28 @@ usb_disconnect_port(struct usbd_port *up, device_t
parent, int flags)
}
usbd_suspend_pipe(dev->ud_pipe0);
+ mutex_enter(&dev->ud_subdevlock);
if (dev->ud_subdevlen > 0) {
DPRINTFN(3, "disconnect subdevs", 0, 0, 0, 0);
for (i = 0; i < dev->ud_subdevlen; i++) {
if ((subdev = dev->ud_subdevs[i]) == NULL)
continue;
+ device_acquire(subdev);
+ mutex_exit(&dev->ud_subdevlock);
strlcpy(subdevname, device_xname(subdev),
sizeof(subdevname));
- KERNEL_LOCK(1, curlwp);
- rc = config_detach(subdev, flags);
- KERNEL_UNLOCK_ONE(curlwp);
+ rc = config_detach_release(subdev, flags);
if (rc != 0)
return rc;
printf("%s: at %s", subdevname, hubname);
if (up->up_portno != 0)
printf(" port %d", up->up_portno);
printf(" (addr %d) disconnected\n", dev->ud_addr);
+ mutex_enter(&dev->ud_subdevlock);
}
KASSERT(!dev->ud_nifaces_claimed);
}
+ mutex_exit(&dev->ud_subdevlock);
mutex_enter(dev->ud_bus->ub_lock);
dev->ud_bus->ub_devices[usb_addr2dindex(dev->ud_addr)] = NULL;
diff --git a/sys/dev/usb/usbdivar.h b/sys/dev/usb/usbdivar.h
index 2ff3cbe6cfbd..5808288b0b13 100644
--- a/sys/dev/usb/usbdivar.h
+++ b/sys/dev/usb/usbdivar.h
@@ -220,6 +220,7 @@ struct usbd_device {
const struct usbd_quirks
*ud_quirks; /* device quirks, always set */
struct usbd_hub *ud_hub; /* only if this is a hub */
+ kmutex_t ud_subdevlock;
u_int ud_subdevlen; /* array length of following */
device_t *ud_subdevs; /* sub-devices */
int ud_nifaces_claimed; /* number of ifaces in use
*/
diff --git a/sys/dev/usb/xhci.c b/sys/dev/usb/xhci.c
index 5b8916973b5d..333fa0f1667e 100644
--- a/sys/dev/usb/xhci.c
+++ b/sys/dev/usb/xhci.c
@@ -2812,6 +2812,7 @@ xhci_new_device(device_t parent, struct usbd_bus *bus,
int depth,
dev = kmem_zalloc(sizeof(*dev), KM_SLEEP);
dev->ud_bus = bus;
+ mutex_init(&dev->ud_subdevlock, MUTEX_DEFAULT, IPL_NONE);
dev->ud_quirks = &usbd_no_quirk;
dev->ud_addr = 0;
dev->ud_ddesc.bMaxPacketSize = 0;
