> 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;