From nobody Tue Nov 30 22:21:04 2021 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 1F56618A28CF; Tue, 30 Nov 2021 22:21:05 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4J3cB85F5pz4SVc; Tue, 30 Nov 2021 22:21:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 954AB1E3BC; Tue, 30 Nov 2021 22:21:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 1AUML41h017861; Tue, 30 Nov 2021 22:21:04 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 1AUML4IA017860; Tue, 30 Nov 2021 22:21:04 GMT (envelope-from git) Date: Tue, 30 Nov 2021 22:21:04 GMT Message-Id: <202111302221.1AUML4IA017860@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Warner Losh Subject: git: 1c7d15b03071 - main - Make device_busy/unbusy work w/o Giant held List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: imp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 1c7d15b030718d9d8cc70916fe3216a19f30896b Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1638310864; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=vVy2s7pq9PPiUfbVXP+2iuhrwzIYN53wuXxrcir+zSA=; b=SC3nho4js6RnaP35i51Ejp3lNljBS46xwS/k3M2UwMcoiiTOGCLMaMbuj5pMEJ4kmsfv0O 13yuNoSUt6cJ0Lfr7a2mHgm6aBTOFr1LosDhY397+XZ7Km7Cgj0WjSGEGFyLSMBBE/BbEG m7Z2Nzxiucy2sL3rjpJ4X3u8s9rR7qveCZ8iVAIkOv1nR77/66rJhDowSvmMM6FcLVlN4f 1VwV9OpBZPtd1NjgWKef6O0xgdkpe/J6WOKLZn7FSXaGOvxDXNyEdWX5PN9w8AnwOMCuys SpNoBDdJyXVnknaCRbdAWcvNQuNZivXTDcS7kjqFYLx9/tDckTVhUVWR8n6u6A== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1638310864; a=rsa-sha256; cv=none; b=FbaaQuT+ATSS6Yl9wYeSEeQeYx5SBgoIyz+hu+iE3gPce1iUi5V7M2G6PcM+96f7QTvZlc PUCIm4ja4YkAbberMhOjpy9oRZLJR88dv6Wio8Nw1B/5wsxC2B02uOiQuJyCTp97zxKK7q NMt1oF7JvMxuJYrqN77GfIhcjE2YaTvHdi9Kxr7T6MmZa9hdDBbNglsBQKZFp1+EJT92ZY Q2daqo6XECtlJVZzAc2z9GOyf/V/FZFkapNbst/ocl3S2OmuojwTvtaFPLjp6CL9W9uASg 7CCVKEHjKEm9Mm8L0sHzw81rix1FW5V62H+oAAljjHJOUGsJtXeszF3Sg4SkKQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=1c7d15b030718d9d8cc70916fe3216a19f30896b commit 1c7d15b030718d9d8cc70916fe3216a19f30896b Author: Warner Losh AuthorDate: 2021-11-30 22:03:26 +0000 Commit: Warner Losh 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 #include #include +#include #include #include #include @@ -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; /**