Module Name: src Committed By: isaki Date: Thu May 23 12:20:27 UTC 2019
Modified Files: src/sys/dev/audio: audio.c audiodef.h Log Message: Remove unnecessary file lock. It has been introduced to prevent multiple syscalls entering simultaneously. But it's completely unnecessary. It fixes firefox problem in PR kern/54177. To generate a diff of this commit: cvs rdiff -u -r1.8 -r1.9 src/sys/dev/audio/audio.c cvs rdiff -u -r1.2 -r1.3 src/sys/dev/audio/audiodef.h 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.8 src/sys/dev/audio/audio.c:1.9 --- src/sys/dev/audio/audio.c:1.8 Tue May 21 12:52:57 2019 +++ src/sys/dev/audio/audio.c Thu May 23 12:20:27 2019 @@ -1,4 +1,4 @@ -/* $NetBSD: audio.c,v 1.8 2019/05/21 12:52:57 isaki Exp $ */ +/* $NetBSD: audio.c,v 1.9 2019/05/23 12:20:27 isaki Exp $ */ /*- * Copyright (c) 2008 The NetBSD Foundation, Inc. @@ -131,14 +131,7 @@ * neither lock were necessary. Currently, on the other hand, since * these may be also called after attach, the thread lock is required. * - * In addition, there are two additional locks. - * - * - file->lock. This is a variable protected by sc_lock and is similar - * to the "thread lock". This is one for each file. If any thread - * context and software interrupt context who want to access the file - * structure, they must acquire this lock before. It protects - * descriptor's consistency among multithreaded accesses. Since this - * lock uses sc_lock, don't acquire from hardware interrupt context. + * In addition, there is an additional lock. * * - track->lock. This is an atomic variable and is similar to the * "interrupt lock". This is one for each track. If any thread context @@ -149,7 +142,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.8 2019/05/21 12:52:57 isaki Exp $"); +__KERNEL_RCSID(0, "$NetBSD: audio.c,v 1.9 2019/05/23 12:20:27 isaki Exp $"); #ifdef _KERNEL_OPT #include "audio.h" @@ -507,8 +500,6 @@ static void audio_softintr_wr(void *); static int audio_enter_exclusive(struct audio_softc *); static void audio_exit_exclusive(struct audio_softc *); static int audio_track_waitio(struct audio_softc *, audio_track_t *); -static int audio_file_acquire(struct audio_softc *, audio_file_t *); -static void audio_file_release(struct audio_softc *, audio_file_t *); static int audioclose(struct file *); static int audioread(struct file *, off_t *, struct uio *, kauth_cred_t, int); @@ -1431,57 +1422,6 @@ audio_track_waitio(struct audio_softc *s } /* - * Acquire the file lock. - * If file is acquired successfully, returns 0. Otherwise returns errno. - * In both case, sc_lock is released. - */ -static int -audio_file_acquire(struct audio_softc *sc, audio_file_t *file) -{ - int error; - - KASSERT(!mutex_owned(sc->sc_lock)); - - mutex_enter(sc->sc_lock); - if (sc->sc_dying) { - mutex_exit(sc->sc_lock); - return EIO; - } - - while (__predict_false(file->lock != 0)) { - error = cv_wait_sig(&sc->sc_exlockcv, sc->sc_lock); - if (sc->sc_dying) - error = EIO; - if (error) { - mutex_exit(sc->sc_lock); - return error; - } - } - - /* Mark this file locked */ - file->lock = 1; - mutex_exit(sc->sc_lock); - - return 0; -} - -/* - * Release the file lock. - */ -static void -audio_file_release(struct audio_softc *sc, audio_file_t *file) -{ - - KASSERT(!mutex_owned(sc->sc_lock)); - - mutex_enter(sc->sc_lock); - KASSERT(file->lock); - file->lock = 0; - cv_broadcast(&sc->sc_exlockcv); - mutex_exit(sc->sc_lock); -} - -/* * Try to acquire track lock. * It doesn't block if the track lock is already aquired. * Returns true if the track lock was acquired, or false if the track @@ -1563,11 +1503,7 @@ audioclose(struct file *fp) sc = file->sc; dev = file->dev; - /* Acquire file lock and exlock */ - /* XXX what should I do when an error occurs? */ - error = audio_file_acquire(sc, file); - if (error) - return error; + /* audio_{enter,exit}_exclusive() is called by lower audio_close() */ device_active(sc->sc_dev, DVA_SYSTEM); switch (AUDIODEV(dev)) { @@ -1590,11 +1526,6 @@ audioclose(struct file *fp) fp->f_audioctx = NULL; } - /* - * Since file has already been destructed, - * audio_file_release() is not necessary. - */ - return error; } @@ -1612,10 +1543,6 @@ audioread(struct file *fp, off_t *offp, sc = file->sc; dev = file->dev; - error = audio_file_acquire(sc, file); - if (error) - return error; - if (fp->f_flag & O_NONBLOCK) ioflag |= IO_NDELAY; @@ -1632,7 +1559,6 @@ audioread(struct file *fp, off_t *offp, error = ENXIO; break; } - audio_file_release(sc, file); return error; } @@ -1651,10 +1577,6 @@ audiowrite(struct file *fp, off_t *offp, sc = file->sc; dev = file->dev; - error = audio_file_acquire(sc, file); - if (error) - return error; - if (fp->f_flag & O_NONBLOCK) ioflag |= IO_NDELAY; @@ -1671,7 +1593,6 @@ audiowrite(struct file *fp, off_t *offp, error = ENXIO; break; } - audio_file_release(sc, file); return error; } @@ -1690,10 +1611,6 @@ audioioctl(struct file *fp, u_long cmd, sc = file->sc; dev = file->dev; - error = audio_file_acquire(sc, file); - if (error) - return error; - switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1714,7 +1631,6 @@ audioioctl(struct file *fp, u_long cmd, error = ENXIO; break; } - audio_file_release(sc, file); return error; } @@ -1750,9 +1666,6 @@ audiopoll(struct file *fp, int events) sc = file->sc; dev = file->dev; - if (audio_file_acquire(sc, file) != 0) - return 0; - switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1766,7 +1679,6 @@ audiopoll(struct file *fp, int events) revents = POLLERR; break; } - audio_file_release(sc, file); return revents; } @@ -1784,10 +1696,6 @@ audiokqfilter(struct file *fp, struct kn sc = file->sc; dev = file->dev; - error = audio_file_acquire(sc, file); - if (error) - return error; - switch (AUDIODEV(dev)) { case SOUND_DEVICE: case AUDIO_DEVICE: @@ -1801,7 +1709,6 @@ audiokqfilter(struct file *fp, struct kn error = ENXIO; break; } - audio_file_release(sc, file); return error; } @@ -1820,10 +1727,6 @@ audiommap(struct file *fp, off_t *offp, sc = file->sc; dev = file->dev; - error = audio_file_acquire(sc, file); - if (error) - return error; - mutex_enter(sc->sc_lock); device_active(sc->sc_dev, DVA_SYSTEM); /* XXXJDM */ mutex_exit(sc->sc_lock); @@ -1840,7 +1743,6 @@ audiommap(struct file *fp, off_t *offp, error = ENOTSUP; break; } - audio_file_release(sc, file); return error; } @@ -1887,11 +1789,6 @@ audiobellclose(audio_file_t *file) sc = file->sc; - /* XXX what should I do when an error occurs? */ - error = audio_file_acquire(sc, file); - if (error) - return error; - device_active(sc->sc_dev, DVA_SYSTEM); error = audio_close(sc, file); @@ -1911,13 +1808,7 @@ audiobellwrite(audio_file_t *file, struc int error; sc = file->sc; - error = audio_file_acquire(sc, file); - if (error) - return error; - error = audio_write(sc, uio, 0, file); - - audio_file_release(sc, file); return error; } @@ -2179,6 +2070,9 @@ bad1: return error; } +/* + * Must NOT called with sc_lock nor sc_exlock held. + */ int audio_close(struct audio_softc *sc, audio_file_t *file) { @@ -2186,7 +2080,6 @@ audio_close(struct audio_softc *sc, audi int error; KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); TRACEF(1, file, "%spid=%d.%d po=%d ro=%d", (audiodebug >= 3) ? "start " : "", @@ -2209,10 +2102,8 @@ audio_close(struct audio_softc *sc, audi /* Then, acquire exclusive lock to protect counters. */ /* XXX what should I do when an error occurs? */ error = audio_enter_exclusive(sc); - if (error) { - audio_file_release(sc, file); + if (error) return error; - } if (file->ptrack) { /* Call hw halt_output if this is the last playback track. */ @@ -2294,7 +2185,6 @@ audio_read(struct audio_softc *sc, struc TRACET(2, track, "resid=%zd", uio->uio_resid); KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); /* I think it's better than EINVAL. */ if (track->mmapped) @@ -2367,7 +2257,6 @@ audio_read(struct audio_softc *sc, struc audio_track_lock_enter(track); audio_track_record(track); - audio_track_lock_exit(track); /* uiomove from usrbuf as much as possible. */ bytes = uimin(usrbuf->used, uio->uio_resid); @@ -2377,6 +2266,7 @@ audio_read(struct audio_softc *sc, struc error = uiomove((uint8_t *)usrbuf->mem + head, len, uio); if (error) { + audio_track_lock_exit(track); device_printf(sc->sc_dev, "uiomove(len=%d) failed with %d\n", len, error); @@ -2389,6 +2279,8 @@ audio_read(struct audio_softc *sc, struc usrbuf->head, usrbuf->used, usrbuf->capacity); bytes -= len; } + + audio_track_lock_exit(track); } abort: @@ -2425,7 +2317,6 @@ audio_write(struct audio_softc *sc, stru uio->uio_resid, (int)curproc->p_pid, (int)curlwp->l_lid, ioflag); KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); /* I think it's better than EINVAL. */ if (track->mmapped) @@ -2492,6 +2383,8 @@ audio_write(struct audio_softc *sc, stru } mutex_exit(sc->sc_lock); + audio_track_lock_enter(track); + /* uiomove to usrbuf as much as possible. */ bytes = uimin(track->usrbuf_usedhigh - usrbuf->used, uio->uio_resid); @@ -2501,6 +2394,7 @@ audio_write(struct audio_softc *sc, stru error = uiomove((uint8_t *)usrbuf->mem + tail, len, uio); if (error) { + audio_track_lock_exit(track); device_printf(sc->sc_dev, "uiomove(len=%d) failed with %d\n", len, error); @@ -2515,11 +2409,11 @@ audio_write(struct audio_softc *sc, stru } /* Convert them as much as possible. */ - audio_track_lock_enter(track); while (usrbuf->used >= track->usrbuf_blksize && outbuf->used < outbuf->capacity) { audio_track_play(track); } + audio_track_lock_exit(track); } @@ -2544,7 +2438,6 @@ audio_ioctl(dev_t dev, struct audio_soft int error; KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); #if defined(AUDIO_DEBUG) const char *ioctlnames[] = { @@ -2849,7 +2742,6 @@ audio_poll(struct audio_softc *sc, int e bool out_is_valid; KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); #if defined(AUDIO_DEBUG) #define POLLEV_BITMAP "\177\020" \ @@ -3004,7 +2896,6 @@ audio_kqfilter(struct audio_softc *sc, a struct klist *klist; KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); TRACEF(3, file, "kn=%p kn_filter=%x", kn, (int)kn->kn_filter); @@ -3042,7 +2933,6 @@ audio_mmap(struct audio_softc *sc, off_t int error; KASSERT(!mutex_owned(sc->sc_lock)); - KASSERT(file->lock); TRACEF(2, file, "off=%lld, prot=%d", (long long)(*offp), prot); Index: src/sys/dev/audio/audiodef.h diff -u src/sys/dev/audio/audiodef.h:1.2 src/sys/dev/audio/audiodef.h:1.3 --- src/sys/dev/audio/audiodef.h:1.2 Wed May 8 13:40:17 2019 +++ src/sys/dev/audio/audiodef.h Thu May 23 12:20:27 2019 @@ -1,4 +1,4 @@ -/* $NetBSD: audiodef.h,v 1.2 2019/05/08 13:40:17 isaki Exp $ */ +/* $NetBSD: audiodef.h,v 1.3 2019/05/23 12:20:27 isaki Exp $ */ /* * Copyright (C) 2017 Tetsuya Isaki. All rights reserved. @@ -182,13 +182,6 @@ struct audio_file { /* process who wants audio SIGIO. */ pid_t async_audio; - /* - * Non-zero if some thread context is using this file structure - * (including ptrack and rtrack) now. - * Must be protected by sc_lock. - */ - volatile int lock; - SLIST_ENTRY(audio_file) entry; };