[alsa-devel] usb audio race at disconnect time
Takashi Iwai
tiwai at suse.de
Sat Oct 13 10:31:22 CEST 2012
At Fri, 12 Oct 2012 17:42:19 +0200,
Matthieu CASTET wrote:
>
> Hi,
>
>
> Takashi Iwai a écrit :
> > [Added Daniel and Clemens in the loop]
> >
> >
> > I don't think this is needed.
> >
> > So... the below is a quick hack I did without testing at all.
> > Hopefully this can give some advance.
> Thanks for the quick patch.
>
> The patch didn't apply cleany of linus tree, of which tree is based your patch ?
Did you try the second one?
The second one covers races in more places.
The only uncovered place is the autosuspend stuff in mixer.c (for PCM,
it's used only in open/close, so it's no problem). I'll fix it later.
thanks,
Takashi
>
> It solve the race in snd_usb_hw_params.
>
> But I wonder if snd_usb_substream_playback_trigger,
> snd_usb_substream_capture_trigger, snd_usb_pcm_pointer are safe : they don't
> take shutdown_mutex.
>
> Also it is hard to check if everything is safe. Sometimes the chip->shutdown is
> far from the shutdown_mutex. For example snd_usb_pcm_open take shutdown_mutex,
> but the check is done in snd_usb_autoresume.
>
> I will do more testing.
>
> Thanks,
>
> Matthieu
> >
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/usb/card.c b/sound/usb/card.c
> > index 561bb74..115484e 100644
> > --- a/sound/usb/card.c
> > +++ b/sound/usb/card.c
> > @@ -131,9 +131,13 @@ static void snd_usb_stream_disconnect(struct list_head *head)
> > subs = &as->substream[idx];
> > if (!subs->num_formats)
> > continue;
> > + if (subs->pcm_substream)
> > + snd_pcm_stream_lock_irq(subs->pcm_substream);
> > subs->interface = -1;
> > subs->data_endpoint = NULL;
> > subs->sync_endpoint = NULL;
> > + if (subs->pcm_substream)
> > + snd_pcm_stream_unlock_irq(subs->pcm_substream);
> > }
> > }
> >
> > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> > index 55e19e1..01e82ac 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -444,7 +444,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
> > {
> > int ret;
> >
> > - mutex_lock(&subs->stream->chip->shutdown_mutex);
> > /* format changed */
> > stop_endpoints(subs, 0, 0, 0);
> > ret = snd_usb_endpoint_set_params(subs->data_endpoint,
> > @@ -455,7 +454,7 @@ static int configure_endpoint(struct snd_usb_substream *subs)
> > subs->cur_audiofmt,
> > subs->sync_endpoint);
> > if (ret < 0)
> > - goto unlock;
> > + return ret;
> >
> > if (subs->sync_endpoint)
> > ret = snd_usb_endpoint_set_params(subs->data_endpoint,
> > @@ -466,8 +465,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
> > subs->cur_audiofmt,
> > NULL);
> >
> > -unlock:
> > - mutex_unlock(&subs->stream->chip->shutdown_mutex);
> > return ret;
> > }
> >
> > @@ -488,10 +485,15 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
> > struct audioformat *fmt;
> > int ret;
> >
> > + mutex_lock(&subs->stream->chip->shutdown_mutex);
> > + if (subs->stream->chip->shutdown) {
> > + ret = -ENODEV;
> > + goto unlock;
> > + }
> > ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
> > params_buffer_bytes(hw_params));
> > if (ret < 0)
> > - return ret;
> > + goto unlock;
> >
> > subs->pcm_format = params_format(hw_params);
> > subs->period_bytes = params_period_bytes(hw_params);
> > @@ -502,17 +504,20 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
> > if (!fmt) {
> > snd_printd(KERN_DEBUG "cannot set format: format = %#x, rate = %d, channels = %d\n",
> > subs->pcm_format, subs->cur_rate, subs->channels);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto unlock;
> > }
> >
> > if ((ret = set_format(subs, fmt)) < 0)
> > - return ret;
> > + goto unlock;
> >
> > subs->interface = fmt->iface;
> > subs->altset_idx = fmt->altset_idx;
> > subs->need_setup_ep = true;
> >
> > - return 0;
> > + unlock:
> > + mutex_unlock(&subs->stream->chip->shutdown_mutex);
> > + return ret;
> > }
> >
> > /*
> > @@ -547,17 +552,26 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> > struct usb_interface *iface;
> > int ret;
> >
> > + mutex_lock(&subs->stream->chip->shutdown_mutex);
> > + if (subs->stream->chip->shutdown) {
> > + ret = -ENODEV;
> > + goto unlock;
> > + }
> > +
> > if (! subs->cur_audiofmt) {
> > snd_printk(KERN_ERR "usbaudio: no format is specified!\n");
> > - return -ENXIO;
> > + ret = -ENXIO;
> > + goto unlock;
> > }
> >
> > - if (snd_BUG_ON(!subs->data_endpoint))
> > - return -EIO;
> > + if (snd_BUG_ON(!subs->data_endpoint)) {
> > + ret = -EIO;
> > + goto unlock;
> > + }
> >
> > ret = set_format(subs, subs->cur_audiofmt);
> > if (ret < 0)
> > - return ret;
> > + goto unlock;
> >
> > iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
> > alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
> > @@ -567,12 +581,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> > subs->cur_audiofmt,
> > subs->cur_rate);
> > if (ret < 0)
> > - return ret;
> > + goto unlock;
> >
> > if (subs->need_setup_ep) {
> > ret = configure_endpoint(subs);
> > if (ret < 0)
> > - return ret;
> > + goto unlock;
> > subs->need_setup_ep = false;
> > }
> >
> > @@ -592,8 +606,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> > /* for playback, submit the URBs now; otherwise, the first hwptr_done
> > * updates for all URBs would happen at the same time when starting */
> > if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
> > - return start_endpoints(subs, 1);
> > + ret = start_endpoints(subs, 1);
> >
> > + unlock:
> > + mutex_unlock(&subs->stream->chip->shutdown_mutex);
> > return 0;
> > }
> >
> > @@ -980,14 +996,18 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
> > struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
> > struct snd_pcm_runtime *runtime = substream->runtime;
> > struct snd_usb_substream *subs = &as->substream[direction];
> > + int ret;
> >
> > + mutex_lock(&as->chip->shutdown_mutex);
> > subs->interface = -1;
> > subs->altset_idx = 0;
> > runtime->hw = snd_usb_hardware;
> > runtime->private_data = subs;
> > subs->pcm_substream = substream;
> > /* runtime PM is also done there */
> > - return setup_hw_info(runtime, subs);
> > + ret = setup_hw_info(runtime, subs);
> > + mutex_unlock(&as->chip->shutdown_mutex);
> > + return ret;
> > }
> >
> > static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
> > @@ -995,6 +1015,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
> > struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
> > struct snd_usb_substream *subs = &as->substream[direction];
> >
> > + mutex_lock(&as->chip->shutdown_mutex);
> > stop_endpoints(subs, 0, 0, 0);
> >
> > if (!as->chip->shutdown && subs->interface >= 0) {
> > @@ -1004,6 +1025,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
> >
> > subs->pcm_substream = NULL;
> > snd_usb_autosuspend(subs->stream->chip);
> > + mutex_unlock(&as->chip->shutdown_mutex);
> >
> > return 0;
> > }
> > @@ -1222,6 +1244,9 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
> > {
> > struct snd_usb_substream *subs = substream->runtime->private_data;
> >
> > + if (subs->stream->chip->shutdown)
> > + return -ENODEV;
> > +
> > switch (cmd) {
> > case SNDRV_PCM_TRIGGER_START:
> > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> >
>
More information about the Alsa-devel
mailing list