Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/pad pad(4): Some incomplete tidying.



details:   https://anonhg.NetBSD.org/src/rev/b321711d5aa1
branches:  trunk
changeset: 1021709:b321711d5aa1
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Mon Jun 14 00:21:09 2021 +0000

description:
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:

 sys/dev/pad/pad.c |  152 ++++++++++++++++++++++-------------------------------
 1 files changed, 62 insertions(+), 90 deletions(-)

diffs (252 lines):

diff -r fa808af68851 -r b321711d5aa1 sys/dev/pad/pad.c
--- a/sys/dev/pad/pad.c Sun Jun 13 23:09:22 2021 +0000
+++ b/sys/dev/pad/pad.c Mon Jun 14 00:21:09 2021 +0000
@@ -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@localhost>
@@ -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 @@
 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;
 
-       sc = device_private(self);
+       error = config_detach_children(self, flags);
+       if (error)
+               return error;
+
        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 @@
 {
        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 @@
                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 @@
 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;
-       }
+       int rc __diagused;
 
-       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);
+       /* XXX Defend against concurrent drvctl detach.  */
+       rc = config_detach(sc->sc_dev, DETACH_FORCE);
+       KASSERT(rc == 0);
 
-       mutex_destroy(&sc->sc_cond_lock);
-       mutex_destroy(&sc->sc_lock);
-       mutex_destroy(&sc->sc_intr_lock);
-       cv_destroy(&sc->sc_condvar);
-
-       rc = config_detach(sc->sc_dev, 0);
-       mutex_exit(&padconfig);
-
-       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 @@
 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 @@
 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 @@
 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);
 }



Home | Main Index | Thread Index | Old Index