Re: usb audio race at disconnect time

[Matthieu CASTET <matthieu.castet-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html