Module Name: src
Committed By: martin
Date: Mon Mar 1 16:00:08 UTC 2021
Modified Files:
src/sys/dev/audio [netbsd-9]: audio.c
Log Message:
Pull up following revision(s) (requested by isaki in ticket #1219):
sys/dev/audio/audio.c: revision 1.89
sys/dev/audio/audio.c: revision 1.90
sys/dev/audio/audio.c: revision 1.91
Change the lock conditions to call audio_unlink().
This can remove a different copy of audio_exlock_enter() in audio_unlink()
and can use normal one. Also, in audiodetach(), this can set the exlock
at more natual order (before calling audio_unlink()).
No noticeable functional changes are intended.
Thanks for comments, riastradh@.
Protect also audioopen() and audiobellopen() from audiodetach() with
psref(9), as well as others(audioread, audiowrite, etc..).
- Rename audio_file_enter to audio_sc_acquire_fromfile, audio_file_exit
to audio_sc_release, for clarify. These are the reference counter for
this sc.
- Introduce audio_sc_acquire_foropen for audio{,bell}open.
- audio_open needs to examine sc_dying again before inserting it into
sc_files, in order to keep sc_files consistency.
The race between audiodetach and audioopen is pointed out by riastradh@.
Thank you for many advices.
Add missing curlwp_bindx() corresponding to curlwp_bind().
Pointed out by riastradh@.
To generate a diff of this commit:
cvs rdiff -u -r1.28.2.20 -r1.28.2.21 src/sys/dev/audio/audio.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/audio/audio.c
diff -u src/sys/dev/audio/audio.c:1.28.2.20 src/sys/dev/audio/audio.c:1.28.2.21
--- src/sys/dev/audio/audio.c:1.28.2.20 Sun Feb 28 07:07:38 2021
+++ src/sys/dev/audio/audio.c Mon Mar 1 16:00:08 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: audio.c,v 1.28.2.20 2021/02/28 07:07:38 martin Exp $ */
+/* $NetBSD: audio.c,v 1.28.2.21 2021/03/01 16:00:08 martin Exp $ */
/*-
* Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -138,7 +138,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.28.2.20 2021/02/28 07:07:38 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.28.2.21 2021/03/01 16:00:08 martin Exp $");
#ifdef _KERNEL_OPT
#include "audio.h"
@@ -524,8 +524,10 @@ static int audio_exlock_mutex_enter(stru
static void audio_exlock_mutex_exit(struct audio_softc *);
static int audio_exlock_enter(struct audio_softc *);
static void audio_exlock_exit(struct audio_softc *);
-static struct audio_softc *audio_file_enter(audio_file_t *, struct psref *);
-static void audio_file_exit(struct audio_softc *, struct psref *);
+static void audio_sc_acquire_foropen(struct audio_softc *, struct psref *);
+static struct audio_softc *audio_sc_acquire_fromfile(audio_file_t *,
+ struct psref *);
+static void audio_sc_release(struct audio_softc *, struct psref *);
static int audio_track_waitio(struct audio_softc *, audio_track_t *);
static int audioclose(struct file *);
@@ -1295,7 +1297,10 @@ audiodetach(device_t self, int flags)
if (error)
return error;
- /* delete sysctl nodes */
+ /*
+ * This waits currently running sysctls to finish if exists.
+ * After this, no more new sysctls will come.
+ */
sysctl_teardown(&sc->sc_log);
mutex_enter(sc->sc_lock);
@@ -1327,9 +1332,10 @@ audiodetach(device_t self, int flags)
* that hold sc, and any new calls with files that were for sc will
* fail. Thus, we now have exclusive access to the softc.
*/
+ sc->sc_exlock = 1;
/*
- * Nuke all open instances.
+ * Clean up all open instances.
* Here, we no longer need any locks to traverse sc_files.
*/
while ((file = SLIST_FIRST(&sc->sc_files)) != NULL) {
@@ -1352,7 +1358,6 @@ audiodetach(device_t self, int flags)
pmf_device_deregister(self);
/* Free resources */
- sc->sc_exlock = 1;
if (sc->sc_pmixer) {
audio_mixer_destroy(sc, sc->sc_pmixer);
kmem_free(sc->sc_pmixer, sizeof(*sc->sc_pmixer));
@@ -1524,18 +1529,41 @@ audio_exlock_exit(struct audio_softc *sc
}
/*
- * Acquire sc from file, and increment the psref count.
+ * Increment reference counter for this sc.
+ * This is intended to be used for open.
+ */
+void
+audio_sc_acquire_foropen(struct audio_softc *sc, struct psref *refp)
+{
+ int s;
+
+ /* Block audiodetach while we acquire a reference */
+ s = pserialize_read_enter();
+
+ /*
+ * We don't examine sc_dying here. However, all open methods
+ * call audio_exlock_enter() right after this, so we can examine
+ * sc_dying in it.
+ */
+
+ /* Acquire a reference */
+ psref_acquire(refp, &sc->sc_psref, audio_psref_class);
+
+ /* Now sc won't go away until we drop the reference count */
+ pserialize_read_exit(s);
+}
+
+/*
+ * Get sc from file, and increment reference counter for this sc.
+ * This is intended to be used for methods other than open.
* If successful, returns sc. Otherwise returns NULL.
*/
struct audio_softc *
-audio_file_enter(audio_file_t *file, struct psref *refp)
+audio_sc_acquire_fromfile(audio_file_t *file, struct psref *refp)
{
int s;
bool dying;
- /* psref(9) forbids to migrate CPUs */
- curlwp_bind();
-
/* Block audiodetach while we acquire a reference */
s = pserialize_read_enter();
@@ -1556,10 +1584,10 @@ audio_file_enter(audio_file_t *file, str
}
/*
- * Decrement the psref count.
+ * Decrement reference counter for this sc.
*/
void
-audio_file_exit(struct audio_softc *sc, struct psref *refp)
+audio_sc_release(struct audio_softc *sc, struct psref *refp)
{
psref_release(refp, &sc->sc_psref, audio_psref_class);
@@ -1637,6 +1665,8 @@ static int
audioopen(dev_t dev, int flags, int ifmt, struct lwp *l)
{
struct audio_softc *sc;
+ struct psref sc_ref;
+ int bound;
int error;
/* Find the device */
@@ -1644,9 +1674,12 @@ audioopen(dev_t dev, int flags, int ifmt
if (sc == NULL || sc->hw_if == NULL)
return ENXIO;
+ bound = curlwp_bind();
+ audio_sc_acquire_foropen(sc, &sc_ref);
+
error = audio_exlock_enter(sc);
if (error)
- return error;
+ goto done;
device_active(sc->sc_dev, DVA_SYSTEM);
switch (AUDIODEV(dev)) {
@@ -1666,6 +1699,9 @@ audioopen(dev_t dev, int flags, int ifmt
}
audio_exlock_exit(sc);
+done:
+ audio_sc_release(sc, &sc_ref);
+ curlwp_bindx(bound);
return error;
}
@@ -1675,6 +1711,7 @@ audioclose(struct file *fp)
struct audio_softc *sc;
struct psref sc_ref;
audio_file_t *file;
+ int bound;
int error;
dev_t dev;
@@ -1690,7 +1727,8 @@ audioclose(struct file *fp)
* - free all memory objects, regardless of sc.
*/
- sc = audio_file_enter(file, &sc_ref);
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
if (sc) {
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
@@ -1708,8 +1746,9 @@ audioclose(struct file *fp)
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
}
+ curlwp_bindx(bound);
/* Free memory objects anyway */
TRACEF(2, file, "free memory");
@@ -1730,6 +1769,7 @@ audioread(struct file *fp, off_t *offp,
struct audio_softc *sc;
struct psref sc_ref;
audio_file_t *file;
+ int bound;
int error;
dev_t dev;
@@ -1737,9 +1777,12 @@ audioread(struct file *fp, off_t *offp,
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
if (fp->f_flag & O_NONBLOCK)
ioflag |= IO_NDELAY;
@@ -1758,7 +1801,9 @@ audioread(struct file *fp, off_t *offp,
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -1769,6 +1814,7 @@ audiowrite(struct file *fp, off_t *offp,
struct audio_softc *sc;
struct psref sc_ref;
audio_file_t *file;
+ int bound;
int error;
dev_t dev;
@@ -1776,9 +1822,12 @@ audiowrite(struct file *fp, off_t *offp,
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
if (fp->f_flag & O_NONBLOCK)
ioflag |= IO_NDELAY;
@@ -1797,7 +1846,9 @@ audiowrite(struct file *fp, off_t *offp,
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -1808,6 +1859,7 @@ audioioctl(struct file *fp, u_long cmd,
struct psref sc_ref;
audio_file_t *file;
struct lwp *l = curlwp;
+ int bound;
int error;
dev_t dev;
@@ -1815,9 +1867,12 @@ audioioctl(struct file *fp, u_long cmd,
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
@@ -1840,7 +1895,9 @@ audioioctl(struct file *fp, u_long cmd,
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -1850,14 +1907,20 @@ audiostat(struct file *fp, struct stat *
struct audio_softc *sc;
struct psref sc_ref;
audio_file_t *file;
+ int bound;
+ int error;
KASSERT(fp->f_audioctx);
file = fp->f_audioctx;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
+ error = 0;
memset(st, 0, sizeof(*st));
st->st_dev = file->dev;
@@ -1865,8 +1928,10 @@ audiostat(struct file *fp, struct stat *
st->st_gid = kauth_cred_getegid(fp->f_cred);
st->st_mode = S_IFCHR;
- audio_file_exit(sc, &sc_ref);
- return 0;
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
+ return error;
}
static int
@@ -1876,6 +1941,7 @@ audiopoll(struct file *fp, int events)
struct psref sc_ref;
audio_file_t *file;
struct lwp *l = curlwp;
+ int bound;
int revents;
dev_t dev;
@@ -1883,9 +1949,12 @@ audiopoll(struct file *fp, int events)
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return POLLERR;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ revents = POLLERR;
+ goto done;
+ }
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
@@ -1901,7 +1970,9 @@ audiopoll(struct file *fp, int events)
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return revents;
}
@@ -1912,15 +1983,19 @@ audiokqfilter(struct file *fp, struct kn
struct psref sc_ref;
audio_file_t *file;
dev_t dev;
+ int bound;
int error;
KASSERT(fp->f_audioctx);
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
switch (AUDIODEV(dev)) {
case SOUND_DEVICE:
@@ -1936,7 +2011,9 @@ audiokqfilter(struct file *fp, struct kn
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -1948,15 +2025,19 @@ audiommap(struct file *fp, off_t *offp,
struct psref sc_ref;
audio_file_t *file;
dev_t dev;
+ int bound;
int error;
KASSERT(fp->f_audioctx);
file = fp->f_audioctx;
dev = file->dev;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
mutex_enter(sc->sc_lock);
device_active(sc->sc_dev, DVA_SYSTEM); /* XXXJDM */
@@ -1975,7 +2056,9 @@ audiommap(struct file *fp, off_t *offp,
break;
}
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -1991,6 +2074,8 @@ int
audiobellopen(dev_t dev, audio_file_t **filep)
{
struct audio_softc *sc;
+ struct psref sc_ref;
+ int bound;
int error;
/* Find the device */
@@ -1998,14 +2083,20 @@ audiobellopen(dev_t dev, audio_file_t **
if (sc == NULL || sc->hw_if == NULL)
return ENXIO;
+ bound = curlwp_bind();
+ audio_sc_acquire_foropen(sc, &sc_ref);
+
error = audio_exlock_enter(sc);
if (error)
- return error;
+ goto done;
device_active(sc->sc_dev, DVA_SYSTEM);
error = audio_open(dev, sc, FWRITE, 0, curlwp, filep);
audio_exlock_exit(sc);
+done:
+ audio_sc_release(sc, &sc_ref);
+ curlwp_bindx(bound);
return error;
}
@@ -2015,16 +2106,24 @@ audiobellclose(audio_file_t *file)
{
struct audio_softc *sc;
struct psref sc_ref;
+ int bound;
int error;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
-
- error = audio_close(sc, file);
-
- audio_file_exit(sc, &sc_ref);
+ error = 0;
+ /*
+ * audiobellclose() must
+ * - unplug track from the trackmixer if sc exist.
+ * - free all memory objects, regardless of sc.
+ */
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc) {
+ error = audio_close(sc, file);
+ audio_sc_release(sc, &sc_ref);
+ }
+ curlwp_bindx(bound);
+ /* Free memory objects anyway */
KASSERT(file->ptrack);
audio_track_destroy(file->ptrack);
KASSERT(file->rtrack == NULL);
@@ -2039,23 +2138,29 @@ audiobellsetrate(audio_file_t *file, u_i
struct audio_softc *sc;
struct psref sc_ref;
struct audio_info ai;
+ int bound;
int error;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done1;
+ }
AUDIO_INITINFO(&ai);
ai.play.sample_rate = sample_rate;
error = audio_exlock_enter(sc);
if (error)
- goto done;
+ goto done2;
error = audio_file_setinfo(sc, file, &ai);
audio_exlock_exit(sc);
-done:
- audio_file_exit(sc, &sc_ref);
+done2:
+ audio_sc_release(sc, &sc_ref);
+done1:
+ curlwp_bindx(bound);
return error;
}
@@ -2065,15 +2170,21 @@ audiobellwrite(audio_file_t *file, struc
{
struct audio_softc *sc;
struct psref sc_ref;
+ int bound;
int error;
- sc = audio_file_enter(file, &sc_ref);
- if (sc == NULL)
- return EIO;
+ bound = curlwp_bind();
+ sc = audio_sc_acquire_fromfile(file, &sc_ref);
+ if (sc == NULL) {
+ error = EIO;
+ goto done;
+ }
error = audio_write(sc, uio, 0, file);
- audio_file_exit(sc, &sc_ref);
+ audio_sc_release(sc, &sc_ref);
+done:
+ curlwp_bindx(bound);
return error;
}
@@ -2097,6 +2208,7 @@ audio_open(dev_t dev, struct audio_softc
bool cred_held;
bool hw_opened;
bool rmixer_started;
+ bool inserted;
int fd;
int error;
@@ -2111,6 +2223,7 @@ audio_open(dev_t dev, struct audio_softc
cred_held = false;
hw_opened = false;
rmixer_started = false;
+ inserted = false;
af = kmem_zalloc(sizeof(audio_file_t), KM_SLEEP);
af->sc = sc;
@@ -2314,22 +2427,22 @@ audio_open(dev_t dev, struct audio_softc
rmixer_started = true;
}
- if (bellfile) {
- *bellfile = af;
- } else {
- error = fd_allocfile(&fp, &fd);
- if (error)
- goto bad;
-
- error = fd_clone(fp, fd, flags, &audio_fileops, af);
- KASSERTMSG(error == EMOVEFD, "error=%d", error);
- }
-
/*
- * Count up finally.
- * Don't fail from here.
+ * This is the last sc_lock section in the function, so we have to
+ * examine sc_dying again before starting the rest tasks. Because
+ * audiodeatch() may have been invoked (and it would set sc_dying)
+ * from the time audioopen() was executed until now. If it happens,
+ * audiodetach() may already have set file->dying for all sc_files
+ * that exist at that point, so that audioopen() must abort without
+ * inserting af to sc_files, in order to keep consistency.
*/
mutex_enter(sc->sc_lock);
+ if (sc->sc_dying) {
+ mutex_exit(sc->sc_lock);
+ goto bad;
+ }
+
+ /* Count up finally */
if (af->ptrack)
sc->sc_popens++;
if (af->rtrack)
@@ -2338,13 +2451,35 @@ audio_open(dev_t dev, struct audio_softc
SLIST_INSERT_HEAD(&sc->sc_files, af, entry);
mutex_exit(sc->sc_intr_lock);
mutex_exit(sc->sc_lock);
+ inserted = true;
+
+ if (bellfile) {
+ *bellfile = af;
+ } else {
+ error = fd_allocfile(&fp, &fd);
+ if (error)
+ goto bad;
+
+ error = fd_clone(fp, fd, flags, &audio_fileops, af);
+ KASSERTMSG(error == EMOVEFD, "error=%d", error);
+ }
+
+ /* Be nothing else after fd_clone */
TRACEF(3, af, "done");
return error;
bad:
- if (fp) {
- fd_abort(curproc, fp, fd);
+ if (inserted) {
+ mutex_enter(sc->sc_lock);
+ mutex_enter(sc->sc_intr_lock);
+ SLIST_REMOVE(&sc->sc_files, af, audio_file, entry);
+ mutex_exit(sc->sc_intr_lock);
+ if (af->ptrack)
+ sc->sc_popens--;
+ if (af->rtrack)
+ sc->sc_ropens--;
+ mutex_exit(sc->sc_lock);
}
if (rmixer_started) {
@@ -2389,6 +2524,7 @@ bad:
int
audio_close(struct audio_softc *sc, audio_file_t *file)
{
+ int error;
/* Protect entering new fileops to this file */
atomic_store_relaxed(&file->dying, true);
@@ -2403,12 +2539,27 @@ audio_close(struct audio_softc *sc, audi
mutex_exit(sc->sc_lock);
}
- return audio_unlink(sc, file);
+ error = audio_exlock_enter(sc);
+ if (error) {
+ /*
+ * If EIO, this sc is about to detach. In this case, even if
+ * we don't do subsequent _unlink(), audiodetach() will do it.
+ */
+ if (error == EIO)
+ return error;
+
+ /* XXX This should not happen but what should I do ? */
+ panic("%s: can't acquire exlock: errno=%d", __func__, error);
+ }
+ error = audio_unlink(sc, file);
+ audio_exlock_exit(sc);
+
+ return error;
}
/*
* Unlink this file, but not freeing memory here.
- * Must be called without sc_lock nor sc_exlock held.
+ * Must be called with sc_exlock held and without sc_lock held.
*/
int
audio_unlink(struct audio_softc *sc, audio_file_t *file)
@@ -2425,25 +2576,6 @@ audio_unlink(struct audio_softc *sc, aud
"sc->sc_popens=%d, sc->sc_ropens=%d",
sc->sc_popens, sc->sc_ropens);
- /*
- * Acquire exlock to protect counters.
- * audio_exlock_enter() cannot be used here because we have to go
- * forward even if sc_dying is set.
- */
- while (__predict_false(sc->sc_exlock != 0)) {
- error = cv_timedwait_sig(&sc->sc_exlockcv, sc->sc_lock,
- mstohz(AUDIO_TIMEOUT));
- /* XXX what should I do on error? */
- if (error == EWOULDBLOCK) {
- mutex_exit(sc->sc_lock);
- audio_printf(sc,
- "%s: cv_timedwait_sig failed: errno=%d\n",
- __func__, error);
- return error;
- }
- }
- sc->sc_exlock = 1;
-
device_active(sc->sc_dev, DVA_SYSTEM);
mutex_enter(sc->sc_intr_lock);
@@ -2510,7 +2642,6 @@ audio_unlink(struct audio_softc *sc, aud
kauth_cred_free(sc->sc_cred);
TRACE(3, "done");
- audio_exlock_exit(sc);
return 0;
}
