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 ?
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:
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
