diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c index e244eb6d1bc3..856b253b7ca9 100644 --- a/sys/dev/dkwedge/dk.c +++ b/sys/dev/dkwedge/dk.c @@ -91,8 +91,6 @@ struct dkwedge_softc { struct callout sc_restart_ch; /* callout to restart I/O */ kmutex_t sc_iolock; - kcondvar_t sc_dkdrn; - u_int sc_iopend; /* I/Os pending */ bool sc_iostop; /* don't schedule restart */ int sc_mode; /* parent open mode */ }; @@ -101,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 *); @@ -114,8 +114,6 @@ static int dkwedge_del1(struct dkwedge_info *, int); static int dk_open_parent(dev_t, int, struct vnode **); static int dk_close_parent(struct vnode *, int); -static int dkunit(dev_t); - static dev_type_open(dkopen); static dev_type_close(dkclose); static dev_type_cancel(dkcancel); @@ -142,7 +140,7 @@ const struct bdevsw dk_bdevsw = { .d_psize = dksize, .d_discard = dkdiscard, .d_cfdriver = &dk_cd, - .d_devtounit = dkunit, + .d_devtounit = dev_minor_unit, .d_flag = D_DISK | D_MPSAFE }; @@ -160,7 +158,7 @@ const struct cdevsw dk_cdevsw = { .d_kqfilter = nokqfilter, .d_discard = dkdiscard, .d_cfdriver = &dk_cd, - .d_devtounit = dkunit, + .d_devtounit = dev_minor_unit, .d_flag = D_DISK | D_MPSAFE }; @@ -192,24 +190,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"); -} -/* - * dkwedge_wait_drain: - * - * Wait for I/O on the wedge to drain. - */ -static void -dkwedge_wait_drain(struct dkwedge_softc *sc) -{ + 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); - mutex_enter(&sc->sc_iolock); - while (sc->sc_iopend != 0) - cv_wait(&sc->sc_dkdrn, &sc->sc_iolock); - mutex_exit(&sc->sc_iolock); + 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; } /* @@ -381,6 +389,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); @@ -410,8 +419,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); @@ -441,7 +453,6 @@ dkwedge_add(struct dkwedge_info *dkw) callout_setfunc(&sc->sc_restart_ch, dkrestart, sc); mutex_init(&sc->sc_iolock, MUTEX_DEFAULT, IPL_BIO); - cv_init(&sc->sc_dkdrn, "dkdrn"); /* * Wedge will be added; increment the wedge count for the parent. @@ -484,7 +495,6 @@ dkwedge_add(struct dkwedge_info *dkw) } mutex_exit(&pdk->dk_openlock); if (error) { - cv_destroy(&sc->sc_dkdrn); mutex_destroy(&sc->sc_iolock); bufq_free(sc->sc_bufq); dkwedge_size_fini(sc); @@ -496,7 +506,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); @@ -542,7 +552,6 @@ dkwedge_add(struct dkwedge_info *dkw) LIST_REMOVE(sc, sc_plink); mutex_exit(&pdk->dk_openlock); - cv_destroy(&sc->sc_dkdrn); mutex_destroy(&sc->sc_iolock); bufq_free(sc->sc_bufq); dkwedge_size_fini(sc); @@ -558,7 +567,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); @@ -572,7 +581,6 @@ dkwedge_add(struct dkwedge_info *dkw) LIST_REMOVE(sc, sc_plink); mutex_exit(&pdk->dk_openlock); - cv_destroy(&sc->sc_dkdrn); mutex_destroy(&sc->sc_iolock); bufq_free(sc->sc_bufq); dkwedge_size_fini(sc); @@ -580,19 +588,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. */ @@ -607,11 +603,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, @@ -619,10 +616,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; @@ -632,8 +630,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; } } @@ -667,10 +667,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); } /* @@ -681,26 +681,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); @@ -710,14 +700,6 @@ dkwedge_detach(device_t self, int flags) mutex_exit(&sc->sc_iolock); callout_halt(&sc->sc_restart_ch, NULL); - /* - * dkstart() will kill any queued buffers now that the - * state of the wedge is not RUNNING. Once we've done - * that, wait for any other pending I/O to complete. - */ - dkstart(sc); - dkwedge_wait_drain(sc); - /* Locate the wedge major numbers. */ bmaj = bdevsw_lookup_major(&dk_bdevsw); cmaj = cdevsw_lookup_major(&dk_cdevsw); @@ -763,7 +745,6 @@ dkwedge_detach(device_t self, int flags) rw_exit(&dkwedges_lock); mutex_destroy(&sc->sc_iolock); - cv_destroy(&sc->sc_dkdrn); dkwedge_size_fini(sc); free(sc, M_DKWEDGE); @@ -787,7 +768,6 @@ dkwedge_delall(struct disk *pdk) static void dkwedge_delall1(struct disk *pdk, bool idleonly) { - struct dkwedge_info dkw; struct dkwedge_softc *sc; int flags; @@ -799,8 +779,18 @@ dkwedge_delall1(struct disk *pdk, bool idleonly) mutex_enter(&pdk->dk_rawlock); /* for sc->sc_dk.dk_openmask */ mutex_enter(&pdk->dk_openlock); LIST_FOREACH(sc, &pdk->dk_wedges, sc_plink) { - if (!idleonly || sc->sc_dk.dk_openmask == 0) + /* + * Wedge is not yet created. This is a race -- + * it may as well have been added just after we + * deleted all the wedges, so pretend it's not + * here yet. + */ + if (sc->sc_dev == NULL) + continue; + if (!idleonly || sc->sc_dk.dk_openmask == 0) { + device_acquire(sc->sc_dev); break; + } } if (sc == NULL) { KASSERT(idleonly || pdk->dk_nwedges == 0); @@ -808,12 +798,9 @@ dkwedge_delall1(struct disk *pdk, bool idleonly) mutex_exit(&pdk->dk_rawlock); return; } - strlcpy(dkw.dkw_parent, pdk->dk_name, sizeof(dkw.dkw_parent)); - strlcpy(dkw.dkw_devname, device_xname(sc->sc_dev), - sizeof(dkw.dkw_devname)); mutex_exit(&pdk->dk_openlock); mutex_exit(&pdk->dk_rawlock); - (void) dkwedge_del1(&dkw, flags); + (void)config_detach_release(sc->sc_dev, flags); } } @@ -849,7 +836,7 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l) if (uio.uio_resid < sizeof(dkw)) break; - if (sc->sc_state != DKW_STATE_RUNNING) + if (sc->sc_dev == NULL) continue; strlcpy(dkw.dkw_devname, device_xname(sc->sc_dev), @@ -862,9 +849,19 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l) dkw.dkw_size = dkwedge_size(sc); strlcpy(dkw.dkw_ptype, sc->sc_ptype, sizeof(dkw.dkw_ptype)); + /* + * Acquire a device reference so this wedge doesn't go + * away before our next iteration in LIST_FOREACH, and + * then release the lock for uiomove. + */ + device_acquire(sc->sc_dev); + mutex_exit(&pdk->dk_openlock); error = uiomove(&dkw, sizeof(dkw), &uio); + mutex_enter(&pdk->dk_openlock); + device_release(sc->sc_dev); if (error) break; + dkwl->dkwl_ncopied++; } dkwl->dkwl_nwedges = pdk->dk_nwedges; @@ -873,8 +870,8 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l) return error; } -device_t -dkwedge_find_by_wname(const char *wname) +static device_t +dkwedge_find_by_wname_acquire(const char *wname) { device_t dv = NULL; struct dkwedge_softc *sc; @@ -882,7 +879,7 @@ dkwedge_find_by_wname(const char *wname) rw_enter(&dkwedges_lock, RW_READER); for (i = 0; i < ndkwedges; i++) { - if ((sc = dkwedges[i]) == NULL) + if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL) continue; if (strcmp(sc->sc_wname, wname) == 0) { if (dv != NULL) { @@ -892,6 +889,7 @@ dkwedge_find_by_wname(const char *wname) device_xname(sc->sc_dev)); continue; } + device_acquire(sc->sc_dev); dv = sc->sc_dev; } } @@ -899,17 +897,18 @@ dkwedge_find_by_wname(const char *wname) return dv; } -device_t -dkwedge_find_by_parent(const char *name, size_t *i) +static device_t +dkwedge_find_by_parent_acquire(const char *name, size_t *i) { rw_enter(&dkwedges_lock, RW_READER); for (; *i < (size_t)ndkwedges; (*i)++) { struct dkwedge_softc *sc; - if ((sc = dkwedges[*i]) == NULL) + if ((sc = dkwedges[*i]) == NULL || sc->sc_dev == NULL) continue; if (strcmp(sc->sc_parent->dk_name, name) != 0) continue; + device_acquire(sc->sc_dev); rw_exit(&dkwedges_lock); return sc->sc_dev; } @@ -917,6 +916,30 @@ dkwedge_find_by_parent(const char *name, size_t *i) return NULL; } +/* XXX unsafe */ +device_t +dkwedge_find_by_wname(const char *wname) +{ + device_t dv; + + if ((dv = dkwedge_find_by_wname_acquire(wname)) == NULL) + return NULL; + device_release(dv); + return dv; +} + +/* XXX unsafe */ +device_t +dkwedge_find_by_parent(const char *name, size_t *i) +{ + device_t dv; + + if ((dv = dkwedge_find_by_parent_acquire(name, i)) == NULL) + return NULL; + device_release(dv); + return dv; +} + void dkwedge_print_wnames(void) { @@ -925,7 +948,7 @@ dkwedge_print_wnames(void) rw_enter(&dkwedges_lock, RW_READER); for (i = 0; i < ndkwedges; i++) { - if ((sc = dkwedges[i]) == NULL) + if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL) continue; printf(" wedge:%s", sc->sc_wname); } @@ -1164,21 +1187,24 @@ 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) { - const int unit = minor(dev); - struct dkwedge_softc *sc; - rw_enter(&dkwedges_lock, RW_READER); - if (unit < 0 || unit >= ndkwedges) - sc = NULL; - else - sc = dkwedges[unit]; - rw_exit(&dkwedges_lock); + return device_lookup_private(&dk_cd, minor(dev)); +} - return sc; +static struct dkwedge_softc * +dkwedge_lookup_acquire(dev_t dev) +{ + device_t dv = device_lookup_acquire(&dk_cd, minor(dev)); + + if (dv == NULL) + return NULL; + return device_private(dv); } static int @@ -1225,36 +1251,6 @@ dk_close_parent(struct vnode *vp, int mode) return error; } -/* - * dkunit: [devsw entry point] - * - * Return the autoconf device_t unit number of a wedge by its - * devsw dev_t number, or -1 if there is none. - * - * XXX This is a temporary hack until dkwedge numbering is made to - * correspond 1:1 to autoconf device numbering. - */ -static int -dkunit(dev_t dev) -{ - int mn = minor(dev); - struct dkwedge_softc *sc; - device_t dv; - int unit = -1; - - if (mn < 0) - return -1; - - rw_enter(&dkwedges_lock, RW_READER); - if (mn < ndkwedges && - (sc = dkwedges[minor(dev)]) != NULL && - (dv = sc->sc_dev) != NULL) - unit = device_unit(dv); - rw_exit(&dkwedges_lock); - - return unit; -} - /* * dkopen: [devsw entry point] * @@ -1268,8 +1264,8 @@ dkopen(dev_t dev, int flags, int fmt, struct lwp *l) if (sc == NULL) return ENXIO; - if (sc->sc_state != DKW_STATE_RUNNING) - return ENXIO; + KASSERT(sc->sc_dev != NULL); + KASSERT(sc->sc_state == DKW_STATE_RUNNING); /* * We go through a complicated little dance to only open the parent @@ -1470,7 +1466,6 @@ dkstrategy(struct buf *bp) /* Place it in the queue and start I/O on the unit. */ mutex_enter(&sc->sc_iolock); - sc->sc_iopend++; disk_wait(&sc->sc_dk); bufq_put(sc->sc_bufq, bp); mutex_exit(&sc->sc_iolock); @@ -1500,8 +1495,6 @@ dkstart(struct dkwedge_softc *sc) while ((bp = bufq_peek(sc->sc_bufq)) != NULL) { if (sc->sc_iostop) { (void) bufq_get(sc->sc_bufq); - if (--sc->sc_iopend == 0) - cv_broadcast(&sc->sc_dkdrn); mutex_exit(&sc->sc_iolock); bp->b_error = ENXIO; bp->b_resid = bp->b_bcount; @@ -1587,9 +1580,6 @@ dkiodone(struct buf *bp) putiobuf(bp); mutex_enter(&sc->sc_iolock); - if (--sc->sc_iopend == 0) - cv_broadcast(&sc->sc_dkdrn); - disk_unbusy(&sc->sc_dk, obp->b_bcount - obp->b_resid, obp->b_flags & B_READ); mutex_exit(&sc->sc_iolock); @@ -1865,8 +1855,9 @@ dkdump(dev_t dev, daddr_t blkno, void *va, size_t size) * Find wedge corresponding to the specified parent name * and offset/length. */ -device_t -dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks) +static device_t +dkwedge_find_partition_acquire(device_t parent, daddr_t startblk, + uint64_t nblks) { struct dkwedge_softc *sc; int i; @@ -1874,11 +1865,11 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks) rw_enter(&dkwedges_lock, RW_READER); for (i = 0; i < ndkwedges; i++) { - if ((sc = dkwedges[i]) == NULL) + if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL) continue; if (strcmp(sc->sc_parent->dk_name, device_xname(parent)) == 0 && sc->sc_offset == startblk && - dkwedge_size(sc) == nblks) { + dkwedge_size(sc) >= nblks) { if (wedge) { printf("WARNING: double match for boot wedge " "(%s, %s)\n", @@ -1887,6 +1878,7 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks) continue; } wedge = sc->sc_dev; + device_acquire(wedge); } } rw_exit(&dkwedges_lock); @@ -1894,6 +1886,20 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks) return wedge; } +/* XXX unsafe */ +device_t +dkwedge_find_partition(device_t parent, daddr_t startblk, + uint64_t nblks) +{ + device_t dv; + + if ((dv = dkwedge_find_partition_acquire(parent, startblk, nblks)) + == NULL) + return NULL; + device_release(dv); + return dv; +} + const char * dkwedge_get_parent_name(dev_t dev) { @@ -1903,8 +1909,11 @@ dkwedge_get_parent_name(dev_t dev) if (major(dev) != bmaj && major(dev) != cmaj) return NULL; - struct dkwedge_softc *sc = dkwedge_lookup(dev); + + struct dkwedge_softc *const sc = dkwedge_lookup_acquire(dev); if (sc == NULL) return NULL; - return sc->sc_parent->dk_name; + const char *const name = sc->sc_parent->dk_name; + device_release(sc->sc_dev); + return name; } diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c index e60be2d7aad8..c8cfae8ab114 100644 --- a/sys/kern/subr_autoconf.c +++ b/sys/kern/subr_autoconf.c @@ -1259,7 +1259,7 @@ 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; @@ -1286,6 +1286,29 @@ config_found(device_t parent, void *aux, cfprint_t print, return NULL; } +/* + * config_found(parent, aux, print, cfargs) + * + * Legacy entry point for callers whose use of the returned + * device_t is not delimited by device_release. This is + * inherently racy -- either they must ignore the return value, or + * they must be converted to config_found_acquire with a matching + * device_release. + */ +device_t +config_found(device_t parent, void *aux, cfprint_t print, + const struct cfargs * const cfargs) +{ + device_t dev; + + dev = config_found_acquire(parent, aux, print, cfargs); + if (dev == NULL) + return NULL; + device_release(dev); + + return dev; +} + /* * As above, but for root devices. */ @@ -1708,6 +1731,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 +1803,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,7 +1844,7 @@ 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; @@ -1824,6 +1855,29 @@ config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print, cfargs_canonicalize(cfargs, &store)); } +/* + * 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. This is + * inherently racy -- either they must ignore the return value, or + * they must be converted to config_attach_acquire with a matching + * device_release. + */ +device_t +config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print, + const struct cfargs *cfargs) +{ + device_t dev; + + dev = config_attach_acquire(parent, cf, aux, print, cfargs); + if (dev == NULL) + return NULL; + device_release(dev); + + return dev; +} + /* * As above, but for pseudo-devices. Pseudo-devices attached in this * way are silently inserted into the device tree, and their children @@ -1834,7 +1888,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 +1921,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 +1952,28 @@ 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. This is + * inherently racy -- either they must ignore the return value, or + * they must be converted to config_attach_pseudo_acquire with a + * matching device_release. + */ +device_t +config_attach_pseudo(cfdata_t cf) +{ + device_t dev; + + dev = config_attach_pseudo_acquire(cf, NULL); + if (dev == NULL) + return dev; + device_release(dev); + + return dev; +} + /* * Caller must hold alldevs_lock. */ @@ -2000,9 +2082,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 +2116,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 +2271,22 @@ out: return rv; } +/* + * config_detach(dev, flags) + * + * Legacy entry point for callers that have not acquired a + * reference to dev. This is inherently racy -- you must convert + * such callers to acquire a reference and then use + * config_detach_release instead. + */ +int +config_detach(device_t dev, int flags) +{ + + device_acquire(dev); + return config_detach_release(dev, flags); +} + /* * config_detach_commit(dev) * @@ -2740,7 +2842,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 +2850,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/kern/subr_device.c b/sys/kern/subr_device.c index b00538790be2..073620ce1289 100644 --- a/sys/kern/subr_device.c +++ b/sys/kern/subr_device.c @@ -30,6 +30,8 @@ __KERNEL_RCSID(0, "$NetBSD: subr_device.c,v 1.13 2022/03/28 12:38:59 riastradh Exp $"); #include + +#include #include #include #include @@ -276,7 +278,7 @@ device_private(device_t dev) * It avoids having them test for it to be NULL and only then calling * device_private. */ - return dev == NULL ? NULL : dev->dv_private; + return dev == NULL ? NULL : atomic_load_consume(&dev->dv_private); } void @@ -287,7 +289,7 @@ device_set_private(device_t dev, void *private) " device %s already has private set to %p", dev, private, device_xname(dev), device_private(dev)); KASSERT(private != NULL); - dev->dv_private = private; + atomic_store_release(&dev->dv_private, private); } prop_dictionary_t 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 *);