git: 1c7d15b03071 - main - Make device_busy/unbusy work w/o Giant held

From: Warner Losh <imp_at_FreeBSD.org>
Date: Tue, 30 Nov 2021 22:21:04 UTC
The branch main has been updated by imp:

URL: https://cgit.FreeBSD.org/src/commit/?id=1c7d15b030718d9d8cc70916fe3216a19f30896b

commit 1c7d15b030718d9d8cc70916fe3216a19f30896b
Author:     Warner Losh <imp@FreeBSD.org>
AuthorDate: 2021-11-30 22:03:26 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-11-30 22:18:01 +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. Move to using a reference
    count to keep track of how many times a device_t has been made busy. Use
    that count to make the same decisions that we'd make with the old device
    state.
    
    Note: gpiopps.c uses D_TRACKCLOSE. Others do as well. However, there's a
    known race with closes that will be corrected for all the drivers that
    do this in a future commit.
    
    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;
 
 /**