Date: Fri, 11 Jul 2008 08:05:14 +0200 From: Alexander Leidinger <Alexander@Leidinger.net> To: Hans Petter Selasky <hselasky@FreeBSD.org> Cc: Perforce Change Reviews <perforce@FreeBSD.org>, ariff@FreeBSD.org Subject: Re: PERFORCE change 144991 for review Message-ID: <20080711080514.92125k684wobbhgk@webmail.leidinger.net> In-Reply-To: <200807100757.m6A7vbex060400@repoman.freebsd.org>
index | next in thread | previous in thread | raw e-mail
Quoting Hans Petter Selasky <hselasky@FreeBSD.org> (from Thu, 10 Jul 2008 07:57:37 GMT): > http://perforce.freebsd.org/chv.cgi?CH=144991 > > Change 144991 by hselasky@hselasky_laptop001 on 2008/07/10 07:56:37 > > > These patches into the USB sound system are needed to make > the Giant-free USB audio driver work seamlessly. If the mixer uninit can sleep, there's the possibility for a race, isn't it? How do you make sure there's no race after not locking the mixer? What about a different approach: - keep the generic sound code as it is (locking of the mixer) - add some uninit flag to the mixer private data - check the flag + if it is not set on uninit, set it + if it is set on uninit (and maybe other mixer operations), abort - unlock the mutex in the uninit - do what can sleep - lock the mutex - return Bye, Alexander. > Affected files ... > > .. //depot/projects/usb/src/sys/dev/sound/pcm/mixer.c#10 edit > .. //depot/projects/usb/src/sys/dev/sound/pcm/mixer.h#9 edit > .. //depot/projects/usb/src/sys/dev/usb2/sound/uaudio2.c#6 edit > .. //depot/projects/usb/src/sys/dev/usb2/sound/uaudio2_pcm.c#3 edit > > Differences ... > > ==== //depot/projects/usb/src/sys/dev/sound/pcm/mixer.c#10 (text+ko) ==== > > @@ -589,7 +589,7 @@ > KASSERT(m->type == MIXER_TYPE_SECONDARY, > ("%s(): illegal mixer type=%d", __func__, m->type)); > > - snd_mtxlock(m->lock); > + /* mixer uninit can sleep --hps */ > > MIXER_UNINIT(m); > > @@ -712,6 +712,10 @@ > > mixer_setrecsrc(m, SOUND_MASK_MIC); > > + snd_mtxunlock(m->lock); > + > + /* mixer uninit can sleep --hps */ > + > MIXER_UNINIT(m); > > snd_mtxfree(m->lock); > @@ -1280,3 +1284,16 @@ > > return (EINVAL); > } > + > +/* > + * Allow the sound driver to use the mixer lock to protect its mixer > + * data: > + */ > +struct mtx * > +mixer_get_lock(struct snd_mixer *m) > +{ > + if (m->lock == NULL) { > + return (&Giant); > + } > + return (m->lock); > +} > > ==== //depot/projects/usb/src/sys/dev/sound/pcm/mixer.h#9 (text+ko) ==== > > @@ -56,6 +56,7 @@ > u_int32_t mix_getparent(struct snd_mixer *m, u_int32_t dev); > u_int32_t mix_getchild(struct snd_mixer *m, u_int32_t dev); > void *mix_getdevinfo(struct snd_mixer *m); > +struct mtx *mixer_get_lock(struct snd_mixer *m); > > extern int mixer_count; > > > ==== //depot/projects/usb/src/sys/dev/usb2/sound/uaudio2.c#6 (text+ko) ==== > > @@ -220,7 +220,6 @@ > > struct usb2_device *sc_udev; > struct usb2_xfer *sc_mixer_xfer[1]; > - struct mtx *sc_mixer_lock; > struct uaudio_mixer_node *sc_mixer_root; > struct uaudio_mixer_node *sc_mixer_curr; > > @@ -3091,12 +3090,11 @@ > { > DPRINTF(0, "\n"); > > - sc->sc_mixer_lock = mixer_get_lock(m); > - > if (usb2_transfer_setup(sc->sc_udev, &(sc->sc_mixer_iface_index), > sc->sc_mixer_xfer, uaudio_mixer_config, 1, sc, > - sc->sc_mixer_lock)) { > - DPRINTF(0, "could not allocate USB transfer for audio mixer!\n"); > + mixer_get_lock(m))) { > + DPRINTF(-1, "could not allocate USB " > + "transfer for audio mixer!\n"); > return (ENOMEM); > } > if (!(sc->sc_mix_info & SOUND_MASK_VOLUME)) { > > ==== //depot/projects/usb/src/sys/dev/usb2/sound/uaudio2_pcm.c#3 > (text+ko) ==== > > @@ -127,10 +127,18 @@ > ua_mixer_set(struct snd_mixer *m, unsigned type, unsigned left, > unsigned right) > { > struct mtx *mtx = mixer_get_lock(m); > + uint8_t do_unlock; > > - snd_mtxlock(mtx); /* XXX */ > + if (mtx_owned(mtx)) { > + do_unlock = 0; > + } else { > + do_unlock = 1; > + mtx_lock(mtx); > + } > uaudio_mixer_set(mix_getdevinfo(m), type, left, right); > - snd_mtxunlock(mtx); /* XXX */ > + if (do_unlock) { > + mtx_unlock(mtx); > + } > return (left | (right << 8)); > } > > @@ -139,10 +147,18 @@ > { > struct mtx *mtx = mixer_get_lock(m); > int retval; > + uint8_t do_unlock; > > - snd_mtxlock(mtx); /* XXX */ > + if (mtx_owned(mtx)) { > + do_unlock = 0; > + } else { > + do_unlock = 1; > + mtx_lock(mtx); > + } > retval = uaudio_mixer_setrecsrc(mix_getdevinfo(m), src); > - snd_mtxunlock(mtx); /* XXX */ > + if (do_unlock) { > + mtx_unlock(mtx); > + } > return (retval); > } > > > -- CREDITOR: A man who has a better memory than a debtor. http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID = B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID = 72077137home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080711080514.92125k684wobbhgk>