summaryrefslogtreecommitdiff
path: root/sys/dev
diff options
context:
space:
mode:
authorriastradh <riastradh@NetBSD.org>2021-06-14 00:21:09 +0000
committerriastradh <riastradh@NetBSD.org>2021-06-14 00:21:09 +0000
commit1a77e3cfeef1d0b447d3bf14511bc62b1e3c9a8f (patch)
tree97dd4eab8a3e7ad9d1b4af01da3ddd6832f5a02f /sys/dev
parent93d26f5d3526ced908bf64394209cb91386df1a3 (diff)
pad(4): Some incomplete tidying.
- Put pseudo-device softc setup/teardown back in pad_attach/detach, not in the cdev/fops operations which are about file descriptors. - Remove unnecessary sc_dying flag. - Omit needless config_deactivate(sc->sc_audiodev); the only effect of this is already done by config_detach anyway, which is done in the same context. - Issue config_detach_children and free softc stuff in the right order. - Omit needless `if (sc == NULL) return ENXIO'. Survives eight parallel t_mixerctl tests many times over on an 8-thread/4-core machine. XXX TODO: - Remove padconfig; it is not appropriate to hold a mutex over sleeping allocation or autoconf config_attach operations. This should be done another way. - Fix agreement of sc_condvar with locks: is it sc_cond_lock or sc_intr_lock? Can't be both; unclear why both exist. - Determine whether both cdev and fops are really needed -- it is confusing to have two types of paths into all this logic, and it seems to me only one of them should be necessary.
Diffstat (limited to 'sys/dev')
-rw-r--r--sys/dev/pad/pad.c152
1 files changed, 62 insertions, 90 deletions
diff --git a/sys/dev/pad/pad.c b/sys/dev/pad/pad.c
index 7253da979ed..4b5717e4054 100644
--- a/sys/dev/pad/pad.c
+++ b/sys/dev/pad/pad.c
@@ -1,4 +1,4 @@
-/* $NetBSD: pad.c,v 1.67 2021/06/13 23:09:22 riastradh Exp $ */
+/* $NetBSD: pad.c,v 1.68 2021/06/14 00:21:09 riastradh Exp $ */
/*-
* Copyright (c) 2007 Jared D. McNeill <jmcneill@invisible.ca>
@@ -27,7 +27,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: pad.c,v 1.67 2021/06/13 23:09:22 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: pad.c,v 1.68 2021/06/14 00:21:09 riastradh Exp $");
#include <sys/types.h>
#include <sys/param.h>
@@ -218,21 +218,50 @@ pad_match(device_t parent, cfdata_t data, void *opaque)
static void
pad_attach(device_t parent, device_t self, void *opaque)
{
+ struct pad_softc *sc = device_private(self);
aprint_normal_dev(self, "outputs: 44100Hz, 16-bit, stereo\n");
+
+ sc->sc_dev = self;
+ sc->sc_dying = false;
+
+ cv_init(&sc->sc_condvar, device_xname(sc->sc_dev));
+ mutex_init(&sc->sc_cond_lock, MUTEX_DEFAULT, IPL_NONE);
+ mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NONE);
+ mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_NONE);
+ callout_init(&sc->sc_pcallout, 0/*XXX?*/);
+
+ sc->sc_swvol = 255;
+ sc->sc_buflen = 0;
+ sc->sc_rpos = sc->sc_wpos = 0;
+ sc->sc_audiodev = audio_attach_mi(&pad_hw_if, sc, sc->sc_dev);
+
+ if (!pmf_device_register(sc->sc_dev, NULL, NULL))
+ aprint_error_dev(sc->sc_dev,
+ "couldn't establish power handler\n");
}
static int
pad_detach(device_t self, int flags)
{
- struct pad_softc *sc;
+ struct pad_softc *sc = device_private(self);
int cmaj, mn;
+ int error;
+
+ error = config_detach_children(self, flags);
+ if (error)
+ return error;
- sc = device_private(self);
cmaj = cdevsw_lookup_major(&pad_cdevsw);
mn = device_unit(sc->sc_dev);
- if (!sc->sc_dying)
- vdevgone(cmaj, mn, mn, VCHR);
+ vdevgone(cmaj, mn, mn, VCHR);
+
+ pmf_device_deregister(sc->sc_dev);
+
+ mutex_destroy(&sc->sc_cond_lock);
+ mutex_destroy(&sc->sc_lock);
+ mutex_destroy(&sc->sc_intr_lock);
+ cv_destroy(&sc->sc_condvar);
return 0;
}
@@ -242,7 +271,8 @@ pad_childdet(device_t self, device_t child)
{
struct pad_softc *sc = device_private(self);
- sc->sc_audiodev = NULL;
+ if (child == sc->sc_audiodev)
+ sc->sc_audiodev = NULL;
}
static int
@@ -340,40 +370,17 @@ cdev_pad_open(dev_t dev, int flags, int fmt, struct lwp *l)
return EBUSY;
}
- sc->sc_dev = paddev;
- sc->sc_dying = false;
-
if (PADUNIT(dev) == PADCLONER) {
error = fd_allocfile(&fp, &fd);
if (error) {
if (existing == false)
- config_detach(sc->sc_dev, 0);
+ config_detach(paddev, 0);
mutex_exit(&padconfig);
return error;
}
- }
-
- cv_init(&sc->sc_condvar, device_xname(sc->sc_dev));
- mutex_init(&sc->sc_cond_lock, MUTEX_DEFAULT, IPL_NONE);
- mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NONE);
- mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_NONE);
- callout_init(&sc->sc_pcallout, 0/*XXX?*/);
-
- sc->sc_swvol = 255;
- sc->sc_buflen = 0;
- sc->sc_rpos = sc->sc_wpos = 0;
- KERNEL_LOCK(1, NULL);
- sc->sc_audiodev = audio_attach_mi(&pad_hw_if, sc, sc->sc_dev);
- KERNEL_UNLOCK_ONE(NULL);
-
- if (!pmf_device_register(sc->sc_dev, NULL, NULL))
- aprint_error_dev(sc->sc_dev,
- "couldn't establish power handler\n");
-
- if (PADUNIT(dev) == PADCLONER) {
error = fd_clone(fp, fd, flags, &pad_fileops, sc);
KASSERT(error == EMOVEFD);
- }
+ }
sc->sc_open = 1;
mutex_exit(&padconfig);
@@ -386,65 +393,42 @@ bad:
static int
pad_close(struct pad_softc *sc)
{
- int rc;
-
- if (sc == NULL)
- return ENXIO;
-
- mutex_enter(&padconfig);
- config_deactivate(sc->sc_audiodev);
-
- /* Start draining existing accessors of the device. */
- if ((rc = config_detach_children(sc->sc_dev,
- DETACH_SHUTDOWN|DETACH_FORCE)) != 0) {
- mutex_exit(&padconfig);
- return rc;
- }
-
- mutex_enter(&sc->sc_lock);
- sc->sc_dying = true;
- cv_broadcast(&sc->sc_condvar);
- mutex_exit(&sc->sc_lock);
-
- KASSERT(sc->sc_open > 0);
- sc->sc_open = 0;
-
- pmf_device_deregister(sc->sc_dev);
-
- mutex_destroy(&sc->sc_cond_lock);
- mutex_destroy(&sc->sc_lock);
- mutex_destroy(&sc->sc_intr_lock);
- cv_destroy(&sc->sc_condvar);
+ int rc __diagused;
- rc = config_detach(sc->sc_dev, 0);
- mutex_exit(&padconfig);
+ /* XXX Defend against concurrent drvctl detach. */
+ rc = config_detach(sc->sc_dev, DETACH_FORCE);
+ KASSERT(rc == 0);
- return rc;
+ return 0;
}
static int
fops_pad_close(struct file *fp)
{
- struct pad_softc *sc;
- int error;
-
- sc = fp->f_pad;
+ struct pad_softc *sc = fp->f_pad;
- error = pad_close(sc);
+ pad_close(sc);
+ fp->f_pad = NULL;
- if (error == 0)
- fp->f_pad = NULL;
-
- return error;
+ return 0;
}
int
cdev_pad_close(dev_t dev, int flags, int ifmt, struct lwp *l)
{
struct pad_softc *sc;
- sc = device_private(device_lookup(&pad_cd, PADUNIT(dev)));
- return pad_close(sc);
+ /*
+ * XXX Defend against concurrent drvctl detach. Simply testing
+ * sc == NULL here is not enough -- it could be detached after
+ * we look it up but before we've issued our own config_detach.
+ */
+ sc = device_lookup_private(&pad_cd, PADUNIT(dev));
+ if (sc == NULL)
+ return 0;
+ pad_close(sc);
+
+ return 0;
}
static int
@@ -457,12 +441,8 @@ fops_pad_poll(struct file *fp, int events)
static int
fops_pad_kqfilter(struct file *fp, struct knote *kn)
{
- struct pad_softc *sc;
+ struct pad_softc *sc = fp->f_pad;
dev_t dev;
-
- sc = fp->f_pad;
- if (sc == NULL)
- return EIO;
dev = makedev(cdevsw_lookup_major(&pad_cdevsw),
device_unit(sc->sc_dev));
@@ -480,11 +460,7 @@ fops_pad_ioctl(struct file *fp, u_long cmd, void *data)
static int
fops_pad_stat(struct file *fp, struct stat *st)
{
- struct pad_softc *sc;
-
- sc = fp->f_pad;
- if (sc == NULL)
- return EIO;
+ struct pad_softc *sc = fp->f_pad;
memset(st, 0, sizeof(*st));
@@ -522,11 +498,7 @@ static int
fops_pad_read(struct file *fp, off_t *offp, struct uio *uio, kauth_cred_t cred,
int ioflag)
{
- struct pad_softc *sc;
-
- sc = fp->f_pad;
- if (sc == NULL)
- return ENXIO;
+ struct pad_softc *sc = fp->f_pad;
return pad_read(sc, offp, uio, cred, ioflag);
}