From 978520b75f0a1ce82b17e1e8186417250de6d545 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 12 Oct 2012 15:12:55 +0200 Subject: ALSA: usb-audio: Fix races at disconnection Close some races at disconnection of a USB audio device by adding the chip->shutdown_mutex and chip->shutdown check at appropriate places. The spots to put bandaids are: - PCM prepare, hw_params and hw_free - where the usb device is accessed for communication or get speed, in mixer.c and others; the device speed is now cached in subs->speed instead of accessing to chip->dev The accesses in PCM open and close don't need the mutex protection because these are already handled in the core PCM disconnection code. The autosuspend/autoresume codes are still uncovered by this patch because of possible mutex deadlocks. They'll be covered by the upcoming change to rwsem. Also the mixer codes are untouched, too. These will be fixed in another patch, too. Reported-by: Matthieu CASTET Cc: Signed-off-by: Takashi Iwai --- sound/usb/pcm.c | 49 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 17 deletions(-) (limited to 'sound/usb/pcm.c') diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 55e19e1b80ec..55e741c5f231 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -71,6 +71,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream unsigned int hwptr_done; subs = (struct snd_usb_substream *)substream->runtime->private_data; + if (subs->stream->chip->shutdown) + return SNDRV_PCM_POS_XRUN; spin_lock(&subs->lock); hwptr_done = subs->hwptr_done; substream->runtime->delay = snd_usb_pcm_delay(subs, @@ -444,7 +446,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 +456,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, @@ -465,9 +466,6 @@ static int configure_endpoint(struct snd_usb_substream *subs) subs->cur_rate, subs->cur_audiofmt, NULL); - -unlock: - mutex_unlock(&subs->stream->chip->shutdown_mutex); return ret; } @@ -505,7 +503,13 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, return -EINVAL; } - if ((ret = set_format(subs, fmt)) < 0) + mutex_lock(&subs->stream->chip->shutdown_mutex); + if (subs->stream->chip->shutdown) + ret = -ENODEV; + else + ret = set_format(subs, fmt); + mutex_unlock(&subs->stream->chip->shutdown_mutex); + if (ret < 0) return ret; subs->interface = fmt->iface; @@ -528,8 +532,10 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->cur_rate = 0; subs->period_bytes = 0; mutex_lock(&subs->stream->chip->shutdown_mutex); - stop_endpoints(subs, 0, 1, 1); - deactivate_endpoints(subs); + if (!subs->stream->chip->shutdown) { + stop_endpoints(subs, 0, 1, 1); + deactivate_endpoints(subs); + } mutex_unlock(&subs->stream->chip->shutdown_mutex); return snd_pcm_lib_free_vmalloc_buffer(substream); } @@ -552,12 +558,19 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) return -ENXIO; } - if (snd_BUG_ON(!subs->data_endpoint)) - return -EIO; + mutex_lock(&subs->stream->chip->shutdown_mutex); + if (subs->stream->chip->shutdown) { + ret = -ENODEV; + goto unlock; + } + 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 +580,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,9 +605,11 @@ 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); - return 0; + unlock: + mutex_unlock(&subs->stream->chip->shutdown_mutex); + return ret; } static struct snd_pcm_hardware snd_usb_hardware = @@ -647,7 +662,7 @@ static int hw_check_valid_format(struct snd_usb_substream *subs, return 0; } /* check whether the period time is >= the data packet interval */ - if (snd_usb_get_speed(subs->dev) != USB_SPEED_FULL) { + if (subs->speed != USB_SPEED_FULL) { ptime = 125 * (1 << fp->datainterval); if (ptime > pt->max || (ptime == pt->max && pt->openmax)) { hwc_debug(" > check: ptime %u > max %u\n", ptime, pt->max); @@ -925,7 +940,7 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre return err; param_period_time_if_needed = SNDRV_PCM_HW_PARAM_PERIOD_TIME; - if (snd_usb_get_speed(subs->dev) == USB_SPEED_FULL) + if (subs->speed == USB_SPEED_FULL) /* full speed devices have fixed data packet interval */ ptmin = 1000; if (ptmin == 1000) -- cgit v1.2.3 From 34f3c89fda4fba9fe689db22253ca8db2f5e6386 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Mon, 15 Oct 2012 12:16:02 +0200 Subject: ALSA: usb-audio: Use rwsem for disconnect protection Replace mutex with rwsem for codec->shutdown protection so that concurrent accesses are allowed. Also add the protection to snd_usb_autosuspend() and snd_usb_autoresume(), too. Reported-by: Matthieu CASTET Cc: Signed-off-by: Takashi Iwai --- sound/usb/card.c | 12 ++++++++---- sound/usb/mixer.c | 12 ++++++------ sound/usb/pcm.c | 12 ++++++------ sound/usb/usbaudio.h | 2 +- 4 files changed, 21 insertions(+), 17 deletions(-) (limited to 'sound/usb/pcm.c') diff --git a/sound/usb/card.c b/sound/usb/card.c index 561bb74fd364..282f0fc9fed1 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -339,7 +339,7 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx, } mutex_init(&chip->mutex); - mutex_init(&chip->shutdown_mutex); + init_rwsem(&chip->shutdown_rwsem); chip->index = idx; chip->dev = dev; chip->card = card; @@ -560,7 +560,7 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, card = chip->card; mutex_lock(®ister_mutex); - mutex_lock(&chip->shutdown_mutex); + down_write(&chip->shutdown_rwsem); chip->shutdown = 1; chip->num_interfaces--; if (chip->num_interfaces <= 0) { @@ -582,11 +582,11 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, snd_usb_mixer_disconnect(p); } usb_chip[chip->index] = NULL; - mutex_unlock(&chip->shutdown_mutex); + up_write(&chip->shutdown_rwsem); mutex_unlock(®ister_mutex); snd_card_free_when_closed(card); } else { - mutex_unlock(&chip->shutdown_mutex); + up_write(&chip->shutdown_rwsem); mutex_unlock(®ister_mutex); } } @@ -618,16 +618,20 @@ int snd_usb_autoresume(struct snd_usb_audio *chip) { int err = -ENODEV; + down_read(&chip->shutdown_rwsem); if (!chip->shutdown && !chip->probing) err = usb_autopm_get_interface(chip->pm_intf); + up_read(&chip->shutdown_rwsem); return err; } void snd_usb_autosuspend(struct snd_usb_audio *chip) { + down_read(&chip->shutdown_rwsem); if (!chip->shutdown && !chip->probing) usb_autopm_put_interface(chip->pm_intf); + up_read(&chip->shutdown_rwsem); } static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index c2ef11ccd66a..298070e8f2d4 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -292,7 +292,7 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v err = snd_usb_autoresume(cval->mixer->chip); if (err < 0) return -EIO; - mutex_lock(&chip->shutdown_mutex); + down_read(&chip->shutdown_rwsem); while (timeout-- > 0) { if (chip->shutdown) break; @@ -310,7 +310,7 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v err = -EINVAL; out: - mutex_unlock(&chip->shutdown_mutex); + up_read(&chip->shutdown_rwsem); snd_usb_autosuspend(cval->mixer->chip); return err; } @@ -337,7 +337,7 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v if (ret) goto error; - mutex_lock(&chip->shutdown_mutex); + down_read(&chip->shutdown_rwsem); if (chip->shutdown) ret = -ENODEV; else { @@ -346,7 +346,7 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN, validx, idx, buf, size); } - mutex_unlock(&chip->shutdown_mutex); + up_read(&chip->shutdown_rwsem); snd_usb_autosuspend(chip); if (ret < 0) { @@ -453,7 +453,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, err = snd_usb_autoresume(chip); if (err < 0) return -EIO; - mutex_lock(&chip->shutdown_mutex); + down_read(&chip->shutdown_rwsem); while (timeout-- > 0) { if (chip->shutdown) break; @@ -471,7 +471,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, err = -EINVAL; out: - mutex_unlock(&chip->shutdown_mutex); + up_read(&chip->shutdown_rwsem); snd_usb_autosuspend(chip); return err; } diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 55e741c5f231..37428f74dbb6 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -503,12 +503,12 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, return -EINVAL; } - mutex_lock(&subs->stream->chip->shutdown_mutex); + down_read(&subs->stream->chip->shutdown_rwsem); if (subs->stream->chip->shutdown) ret = -ENODEV; else ret = set_format(subs, fmt); - mutex_unlock(&subs->stream->chip->shutdown_mutex); + up_read(&subs->stream->chip->shutdown_rwsem); if (ret < 0) return ret; @@ -531,12 +531,12 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->cur_audiofmt = NULL; subs->cur_rate = 0; subs->period_bytes = 0; - mutex_lock(&subs->stream->chip->shutdown_mutex); + down_read(&subs->stream->chip->shutdown_rwsem); if (!subs->stream->chip->shutdown) { stop_endpoints(subs, 0, 1, 1); deactivate_endpoints(subs); } - mutex_unlock(&subs->stream->chip->shutdown_mutex); + up_read(&subs->stream->chip->shutdown_rwsem); return snd_pcm_lib_free_vmalloc_buffer(substream); } @@ -558,7 +558,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) return -ENXIO; } - mutex_lock(&subs->stream->chip->shutdown_mutex); + down_read(&subs->stream->chip->shutdown_rwsem); if (subs->stream->chip->shutdown) { ret = -ENODEV; goto unlock; @@ -608,7 +608,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) ret = start_endpoints(subs, 1); unlock: - mutex_unlock(&subs->stream->chip->shutdown_mutex); + up_read(&subs->stream->chip->shutdown_rwsem); return ret; } diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h index b8233ebe250f..ef42797f56fb 100644 --- a/sound/usb/usbaudio.h +++ b/sound/usb/usbaudio.h @@ -37,7 +37,7 @@ struct snd_usb_audio { struct usb_interface *pm_intf; u32 usb_id; struct mutex mutex; - struct mutex shutdown_mutex; + struct rw_semaphore shutdown_rwsem; unsigned int shutdown:1; unsigned int probing:1; unsigned int autosuspended:1; -- cgit v1.2.3 From f58161ba1b05a968e5136824b5a16b714b6a5317 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 8 Nov 2012 08:52:45 +0100 Subject: ALSA: usb-audio: Fix crash at re-preparing the PCM stream There are bug reports of a crash with USB-audio devices when PCM prepare is performed immediately after the stream is stopped via trigger callback. It turned out that the problem is that we don't wait until all URBs are killed. This patch adds a new function to synchronize the pending stop operation on an endpoint, and calls in the prepare callback for avoiding the crash above. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=49181 Reported-and-tested-by: Artem S. Tashkinov Cc: [v3.6] Signed-off-by: Takashi Iwai --- sound/usb/endpoint.c | 13 +++++++++++++ sound/usb/endpoint.h | 1 + sound/usb/pcm.c | 3 +++ 3 files changed, 17 insertions(+) (limited to 'sound/usb/pcm.c') diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 7f78c6d782b0..34de6f2faf61 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -35,6 +35,7 @@ #define EP_FLAG_ACTIVATED 0 #define EP_FLAG_RUNNING 1 +#define EP_FLAG_STOPPING 2 /* * snd_usb_endpoint is a model that abstracts everything related to an @@ -502,10 +503,20 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) if (alive) snd_printk(KERN_ERR "timeout: still %d active urbs on EP #%x\n", alive, ep->ep_num); + clear_bit(EP_FLAG_STOPPING, &ep->flags); return 0; } +/* sync the pending stop operation; + * this function itself doesn't trigger the stop operation + */ +void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep) +{ + if (ep && test_bit(EP_FLAG_STOPPING, &ep->flags)) + wait_clear_urbs(ep); +} + /* * unlink active urbs. */ @@ -918,6 +929,8 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, if (wait) wait_clear_urbs(ep); + else + set_bit(EP_FLAG_STOPPING, &ep->flags); } } diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 6376ccf10fd4..3d4c9705041f 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -19,6 +19,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, int force, int can_sleep, int wait); +void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); void snd_usb_endpoint_free(struct list_head *head); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 37428f74dbb6..5c12a3fe8c3e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -568,6 +568,9 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) goto unlock; } + snd_usb_endpoint_sync_pending_stop(subs->sync_endpoint); + snd_usb_endpoint_sync_pending_stop(subs->data_endpoint); + ret = set_format(subs, subs->cur_audiofmt); if (ret < 0) goto unlock; -- cgit v1.2.3 From 947d299686aa9cc8aecf749d54e8475c6e498956 Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Thu, 22 Nov 2012 20:27:59 +0100 Subject: ALSA: snd-usb: properly initialize the sync endpoint Jeffrey Barish reported an obvious bug in the pcm part of the usb-audio driver which causes the code to not initialize the sync endpoint from configure_endpoint(). Reported-by: Jeffrey Barish Signed-off-by: Daniel Mack Cc: stable@kernel.org [3.5+] Signed-off-by: Takashi Iwai --- sound/usb/pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sound/usb/pcm.c') diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 5c12a3fe8c3e..ef6fa24fc473 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -459,7 +459,7 @@ static int configure_endpoint(struct snd_usb_substream *subs) return ret; if (subs->sync_endpoint) - ret = snd_usb_endpoint_set_params(subs->data_endpoint, + ret = snd_usb_endpoint_set_params(subs->sync_endpoint, subs->pcm_format, subs->channels, subs->period_bytes, -- cgit v1.2.3