In message <[email protected]>, Warner Losh
write
s:
> The branch main has been updated by imp:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=1c7d15b030718d9d8cc70916fe3216a1
> 9f30896b
>
> commit 1c7d15b030718d9d8cc70916fe3216a19f30896b
> Author: Warner Losh <[email protected]>
> AuthorDate: 2021-11-30 22:03:26 +0000
> Commit: Warner Losh <[email protected]>
> 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_S
> TRUCTPROC *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 t
> hread *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 cal
> l
> - * 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_fla
> gs() */
> 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_ATT
> ACH);
> 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 */
Hi Warner,
This part of this patch has caused a ports build failure, in audio/oss.
In file included from lynxone.c:20:
In file included from ./module.inc:77:
./bsdpci.inc:95:33: error: use of undeclared identifier 'DS_BUSY'
if (device_get_state(dev) == DS_BUSY)
^
1 error generated.
*** [lynxone.o] Error code 1
make[2]: stopped in /wrkdirs/usr/ports/audio/oss/work/.build/prototype/usr/l
ocal/lib/oss/build
1 error
--
Cheers,
Cy Schubert <[email protected]>
FreeBSD UNIX: <[email protected]> Web: https://FreeBSD.org
NTP: <[email protected]> Web: https://nwtime.org
The need of the many outweighs the greed of the few.
