PERFORCE change 144991 for review

[Alexander Leidinger]

Quoting Hans Petter Selasky <hselasky at FreeBSD.org> (from Thu, 10 Jul  
2008 07:57:37 GMT):

> http://perforce.freebsd.org/chv.cgi?CH=144991
>
> Change 144991 by hselasky at 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 = 72077137