Commit 08e78191 authored by Warner Losh's avatar Warner Losh
Browse files

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
parent d8c1d7b6
...@@ -157,9 +157,7 @@ int drm_open(struct cdev *kdev, int flags, int fmt, DRM_STRUCTPROC *p) ...@@ -157,9 +157,7 @@ int drm_open(struct cdev *kdev, int flags, int fmt, DRM_STRUCTPROC *p)
return 0; return 0;
err_undo: err_undo:
mtx_lock(&Giant); /* FIXME: Giant required? */
device_unbusy(dev->dev); device_unbusy(dev->dev);
mtx_unlock(&Giant);
dev->open_count--; dev->open_count--;
sx_xunlock(&drm_global_mutex); sx_xunlock(&drm_global_mutex);
return -retcode; return -retcode;
...@@ -273,9 +271,7 @@ static int drm_open_helper(struct cdev *kdev, int flags, int fmt, ...@@ -273,9 +271,7 @@ static int drm_open_helper(struct cdev *kdev, int flags, int fmt,
list_add(&priv->lhead, &dev->filelist); list_add(&priv->lhead, &dev->filelist);
DRM_UNLOCK(dev); DRM_UNLOCK(dev);
mtx_lock(&Giant); /* FIXME: Giant required? */
device_busy(dev->dev); device_busy(dev->dev);
mtx_unlock(&Giant);
ret = devfs_set_cdevpriv(priv, drm_release); ret = devfs_set_cdevpriv(priv, drm_release);
if (ret != 0) if (ret != 0)
...@@ -453,9 +449,7 @@ void drm_release(void *data) ...@@ -453,9 +449,7 @@ void drm_release(void *data)
*/ */
atomic_inc(&dev->counts[_DRM_STAT_CLOSES]); atomic_inc(&dev->counts[_DRM_STAT_CLOSES]);
mtx_lock(&Giant);
device_unbusy(dev->dev); device_unbusy(dev->dev);
mtx_unlock(&Giant);
if (!--dev->open_count) { if (!--dev->open_count) {
if (atomic_read(&dev->ioctl_count)) { if (atomic_read(&dev->ioctl_count)) {
DRM_ERROR("Device busy: %d\n", DRM_ERROR("Device busy: %d\n",
......
...@@ -73,9 +73,7 @@ gpiopps_open(struct cdev *dev, int flags, int fmt, struct thread *td) ...@@ -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. */ /* We can't be unloaded while open, so mark ourselves BUSY. */
mtx_lock(&sc->pps_mtx); 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); mtx_unlock(&sc->pps_mtx);
return 0; return 0;
...@@ -86,10 +84,6 @@ gpiopps_close(struct cdev *dev, int flags, int fmt, struct thread *td) ...@@ -86,10 +84,6 @@ gpiopps_close(struct cdev *dev, int flags, int fmt, struct thread *td)
{ {
struct pps_softc *sc = dev->si_drv1; 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); mtx_lock(&sc->pps_mtx);
device_unbusy(sc->dev); device_unbusy(sc->dev);
mtx_unlock(&sc->pps_mtx); mtx_unlock(&sc->pps_mtx);
...@@ -113,6 +107,7 @@ gpiopps_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int flags, struct thre ...@@ -113,6 +107,7 @@ gpiopps_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int flags, struct thre
static struct cdevsw pps_cdevsw = { static struct cdevsw pps_cdevsw = {
.d_version = D_VERSION, .d_version = D_VERSION,
.d_flags = D_TRACKCLOSE,
.d_open = gpiopps_open, .d_open = gpiopps_open,
.d_close = gpiopps_close, .d_close = gpiopps_close,
.d_ioctl = gpiopps_ioctl, .d_ioctl = gpiopps_ioctl,
......
...@@ -342,7 +342,7 @@ pccard_detach_card(device_t dev) ...@@ -342,7 +342,7 @@ pccard_detach_card(device_t dev)
if (pf->dev == NULL) if (pf->dev == NULL)
continue; continue;
state = device_get_state(pf->dev); state = device_get_state(pf->dev);
if (state == DS_ATTACHED || state == DS_BUSY) if (state == DS_ATTACHED)
device_detach(pf->dev); device_detach(pf->dev);
if (pf->cfe != NULL) if (pf->cfe != NULL)
pccard_function_disable(pf); pccard_function_disable(pf);
......
...@@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); ...@@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$");
#include <sys/queue.h> #include <sys/queue.h>
#include <machine/bus.h> #include <machine/bus.h>
#include <sys/random.h> #include <sys/random.h>
#include <sys/refcount.h>
#include <sys/rman.h> #include <sys/rman.h>
#include <sys/sbuf.h> #include <sys/sbuf.h>
#include <sys/selinfo.h> #include <sys/selinfo.h>
...@@ -140,7 +141,7 @@ struct _device { ...@@ -140,7 +141,7 @@ struct _device {
int unit; /**< current unit number */ int unit; /**< current unit number */
char* nameunit; /**< name+unit e.g. foodev0 */ char* nameunit; /**< name+unit e.g. foodev0 */
char* desc; /**< driver specific description */ 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 */ device_state_t state; /**< current device state */
uint32_t devflags; /**< api level flags for device_get_flags() */ uint32_t devflags; /**< api level flags for device_get_flags() */
u_int flags; /**< internal device flags */ u_int flags; /**< internal device flags */
...@@ -2634,13 +2635,13 @@ device_disable(device_t dev) ...@@ -2634,13 +2635,13 @@ device_disable(device_t dev)
void void
device_busy(device_t dev) 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); device_busy(dev->parent);
dev->busy++;
if (dev->state == DS_ATTACHED)
dev->state = DS_BUSY;
} }
/** /**
...@@ -2649,17 +2650,12 @@ device_busy(device_t dev) ...@@ -2649,17 +2650,12 @@ device_busy(device_t dev)
void void
device_unbusy(device_t dev) 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", * Mark the device as unbsy, recursively if this is the last busy count.
device_get_nameunit(dev)); */
dev->busy--; if (refcount_release(&dev->busy) && dev->parent != NULL)
if (dev->busy == 0) { device_unbusy(dev->parent);
if (dev->parent)
device_unbusy(dev->parent);
if (dev->state == DS_BUSY)
dev->state = DS_ATTACHED;
}
} }
/** /**
...@@ -3004,10 +3000,7 @@ device_attach(device_t dev) ...@@ -3004,10 +3000,7 @@ device_attach(device_t dev)
attachentropy = (uint16_t)(get_cyclecount() - attachtime); attachentropy = (uint16_t)(get_cyclecount() - attachtime);
random_harvest_direct(&attachentropy, sizeof(attachentropy), RANDOM_ATTACH); random_harvest_direct(&attachentropy, sizeof(attachentropy), RANDOM_ATTACH);
device_sysctl_update(dev); device_sysctl_update(dev);
if (dev->busy) dev->state = DS_ATTACHED;
dev->state = DS_BUSY;
else
dev->state = DS_ATTACHED;
dev->flags &= ~DF_DONENOMATCH; dev->flags &= ~DF_DONENOMATCH;
EVENTHANDLER_DIRECT_INVOKE(device_attach, dev); EVENTHANDLER_DIRECT_INVOKE(device_attach, dev);
devadded(dev); devadded(dev);
...@@ -3038,7 +3031,7 @@ device_detach(device_t dev) ...@@ -3038,7 +3031,7 @@ device_detach(device_t dev)
GIANT_REQUIRED; GIANT_REQUIRED;
PDEBUG(("%s", DEVICENAME(dev))); PDEBUG(("%s", DEVICENAME(dev)));
if (dev->state == DS_BUSY) if (dev->busy > 0)
return (EBUSY); return (EBUSY);
if (dev->state == DS_ATTACHING) { if (dev->state == DS_ATTACHING) {
device_printf(dev, "device in attaching state! Deferring detach.\n"); device_printf(dev, "device in attaching state! Deferring detach.\n");
...@@ -3090,7 +3083,7 @@ int ...@@ -3090,7 +3083,7 @@ int
device_quiesce(device_t dev) device_quiesce(device_t dev)
{ {
PDEBUG(("%s", DEVICENAME(dev))); PDEBUG(("%s", DEVICENAME(dev)));
if (dev->state == DS_BUSY) if (dev->busy > 0)
return (EBUSY); return (EBUSY);
if (dev->state != DS_ATTACHED) if (dev->state != DS_ATTACHED)
return (0); return (0);
......
...@@ -58,7 +58,6 @@ typedef enum device_state { ...@@ -58,7 +58,6 @@ typedef enum device_state {
DS_ALIVE = 20, /**< @brief probe succeeded */ DS_ALIVE = 20, /**< @brief probe succeeded */
DS_ATTACHING = 25, /**< @brief currently attaching */ DS_ATTACHING = 25, /**< @brief currently attaching */
DS_ATTACHED = 30, /**< @brief attach method called */ DS_ATTACHED = 30, /**< @brief attach method called */
DS_BUSY = 40 /**< @brief device is open */
} device_state_t; } device_state_t;
/** /**
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment