The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=08e781915363f98f4318a864b3b5a52bd99424c6
commit 08e781915363f98f4318a864b3b5a52bd99424c6 Author: Warner Losh <[email protected]> AuthorDate: 2021-11-30 22:03:26 +0000 Commit: Warner Losh <[email protected]> CommitDate: 2021-11-30 22:03:26 +0000 Make device_busy/unbusy work w/o Giant held The vast majority of the busy/unbusy users in the tree don't acquire Giant before calling device_busy/unbusy. However, if multiple threads are opening a file, say, that causes the device to busy/unbusy, then we can race to the root marking things busy. Create a new device_busy_locked and device_unbusy_locked that are the current implemntations of device_busy and device_unbusy. Make device_busy and unbusy acquire Giant before calling the _locked versrions. Since we never sleep in the busy/unbusy path, Giant's single threaded semantics suffice to keep this safe. Sponsored by: Netflix Reviewed by: hselasky, jhb Differential Revision: https://reviews.freebsd.org/D26284 --- sys/dev/drm2/drm_fops.c | 6 ------ sys/dev/gpio/gpiopps.c | 9 ++------- sys/dev/pccard/pccard.c | 2 +- sys/kern/subr_bus.c | 41 +++++++++++++++++------------------------ sys/sys/bus.h | 1 - 5 files changed, 20 insertions(+), 39 deletions(-) diff --git a/sys/dev/drm2/drm_fops.c b/sys/dev/drm2/drm_fops.c index 3b3be06345e5..0c8b71759292 100644 --- a/sys/dev/drm2/drm_fops.c +++ b/sys/dev/drm2/drm_fops.c @@ -157,9 +157,7 @@ int drm_open(struct cdev *kdev, int flags, int fmt, DRM_STRUCTPROC *p) return 0; err_undo: - mtx_lock(&Giant); /* FIXME: Giant required? */ device_unbusy(dev->dev); - mtx_unlock(&Giant); dev->open_count--; sx_xunlock(&drm_global_mutex); return -retcode; @@ -273,9 +271,7 @@ static int drm_open_helper(struct cdev *kdev, int flags, int fmt, list_add(&priv->lhead, &dev->filelist); DRM_UNLOCK(dev); - mtx_lock(&Giant); /* FIXME: Giant required? */ device_busy(dev->dev); - mtx_unlock(&Giant); ret = devfs_set_cdevpriv(priv, drm_release); if (ret != 0) @@ -453,9 +449,7 @@ void drm_release(void *data) */ atomic_inc(&dev->counts[_DRM_STAT_CLOSES]); - mtx_lock(&Giant); device_unbusy(dev->dev); - mtx_unlock(&Giant); if (!--dev->open_count) { if (atomic_read(&dev->ioctl_count)) { DRM_ERROR("Device busy: %d\n", diff --git a/sys/dev/gpio/gpiopps.c b/sys/dev/gpio/gpiopps.c index 4700acf19bcd..741bfa4498a6 100644 --- a/sys/dev/gpio/gpiopps.c +++ b/sys/dev/gpio/gpiopps.c @@ -73,9 +73,7 @@ gpiopps_open(struct cdev *dev, int flags, int fmt, struct thread *td) /* We can't be unloaded while open, so mark ourselves BUSY. */ mtx_lock(&sc->pps_mtx); - if (device_get_state(sc->dev) < DS_BUSY) { - device_busy(sc->dev); - } + device_busy(sc->dev); mtx_unlock(&sc->pps_mtx); return 0; @@ -86,10 +84,6 @@ gpiopps_close(struct cdev *dev, int flags, int fmt, struct thread *td) { struct pps_softc *sc = dev->si_drv1; - /* - * Un-busy on last close. We rely on the vfs counting stuff to only call - * this routine on last-close, so we don't need any open-count logic. - */ mtx_lock(&sc->pps_mtx); device_unbusy(sc->dev); mtx_unlock(&sc->pps_mtx); @@ -113,6 +107,7 @@ gpiopps_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int flags, struct thre static struct cdevsw pps_cdevsw = { .d_version = D_VERSION, + .d_flags = D_TRACKCLOSE, .d_open = gpiopps_open, .d_close = gpiopps_close, .d_ioctl = gpiopps_ioctl, diff --git a/sys/dev/pccard/pccard.c b/sys/dev/pccard/pccard.c index 8f19eb84725c..48d2e6973a38 100644 --- a/sys/dev/pccard/pccard.c +++ b/sys/dev/pccard/pccard.c @@ -342,7 +342,7 @@ pccard_detach_card(device_t dev) if (pf->dev == NULL) continue; state = device_get_state(pf->dev); - if (state == DS_ATTACHED || state == DS_BUSY) + if (state == DS_ATTACHED) device_detach(pf->dev); if (pf->cfe != NULL) pccard_function_disable(pf); diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c index 09072e1a23de..ab7de881d57d 100644 --- a/sys/kern/subr_bus.c +++ b/sys/kern/subr_bus.c @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); #include <sys/queue.h> #include <machine/bus.h> #include <sys/random.h> +#include <sys/refcount.h> #include <sys/rman.h> #include <sys/sbuf.h> #include <sys/selinfo.h> @@ -140,7 +141,7 @@ struct _device { int unit; /**< current unit number */ char* nameunit; /**< name+unit e.g. foodev0 */ char* desc; /**< driver specific description */ - int busy; /**< count of calls to device_busy() */ + u_int busy; /**< count of calls to device_busy() */ device_state_t state; /**< current device state */ uint32_t devflags; /**< api level flags for device_get_flags() */ u_int flags; /**< internal device flags */ @@ -2634,13 +2635,13 @@ device_disable(device_t dev) void device_busy(device_t dev) { - if (dev->state < DS_ATTACHING) - panic("device_busy: called for unattached device"); - if (dev->busy == 0 && dev->parent) + + /* + * Mark the device as busy, recursively up the tree if this busy count + * goes 0->1. + */ + if (refcount_acquire(&dev->busy) == 0 && dev->parent != NULL) device_busy(dev->parent); - dev->busy++; - if (dev->state == DS_ATTACHED) - dev->state = DS_BUSY; } /** @@ -2649,17 +2650,12 @@ device_busy(device_t dev) void device_unbusy(device_t dev) { - if (dev->busy != 0 && dev->state != DS_BUSY && - dev->state != DS_ATTACHING) - panic("device_unbusy: called for non-busy device %s", - device_get_nameunit(dev)); - dev->busy--; - if (dev->busy == 0) { - if (dev->parent) - device_unbusy(dev->parent); - if (dev->state == DS_BUSY) - dev->state = DS_ATTACHED; - } + + /* + * Mark the device as unbsy, recursively if this is the last busy count. + */ + if (refcount_release(&dev->busy) && dev->parent != NULL) + device_unbusy(dev->parent); } /** @@ -3004,10 +3000,7 @@ device_attach(device_t dev) attachentropy = (uint16_t)(get_cyclecount() - attachtime); random_harvest_direct(&attachentropy, sizeof(attachentropy), RANDOM_ATTACH); device_sysctl_update(dev); - if (dev->busy) - dev->state = DS_BUSY; - else - dev->state = DS_ATTACHED; + dev->state = DS_ATTACHED; dev->flags &= ~DF_DONENOMATCH; EVENTHANDLER_DIRECT_INVOKE(device_attach, dev); devadded(dev); @@ -3038,7 +3031,7 @@ device_detach(device_t dev) GIANT_REQUIRED; PDEBUG(("%s", DEVICENAME(dev))); - if (dev->state == DS_BUSY) + if (dev->busy > 0) return (EBUSY); if (dev->state == DS_ATTACHING) { device_printf(dev, "device in attaching state! Deferring detach.\n"); @@ -3090,7 +3083,7 @@ int device_quiesce(device_t dev) { PDEBUG(("%s", DEVICENAME(dev))); - if (dev->state == DS_BUSY) + if (dev->busy > 0) return (EBUSY); if (dev->state != DS_ATTACHED) return (0); diff --git a/sys/sys/bus.h b/sys/sys/bus.h index 0942b7246ae4..4a1afa864c2f 100644 --- a/sys/sys/bus.h +++ b/sys/sys/bus.h @@ -58,7 +58,6 @@ typedef enum device_state { DS_ALIVE = 20, /**< @brief probe succeeded */ DS_ATTACHING = 25, /**< @brief currently attaching */ DS_ATTACHED = 30, /**< @brief attach method called */ - DS_BUSY = 40 /**< @brief device is open */ } device_state_t; /**
