Commit 978520b7 authored by Takashi Iwai's avatar Takashi Iwai

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: default avatarMatthieu CASTET <matthieu.castet@parrot.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 9b0573c0
...@@ -126,6 +126,7 @@ struct snd_usb_substream { ...@@ -126,6 +126,7 @@ struct snd_usb_substream {
struct snd_usb_endpoint *sync_endpoint; struct snd_usb_endpoint *sync_endpoint;
unsigned long flags; unsigned long flags;
bool need_setup_ep; /* (re)configure EP at prepare? */ bool need_setup_ep; /* (re)configure EP at prepare? */
unsigned int speed; /* USB_SPEED_XXX */
u64 formats; /* format bitmasks (all or'ed) */ u64 formats; /* format bitmasks (all or'ed) */
unsigned int num_formats; /* number of supported audio formats (list) */ unsigned int num_formats; /* number of supported audio formats (list) */
......
...@@ -287,25 +287,32 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v ...@@ -287,25 +287,32 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v
unsigned char buf[2]; unsigned char buf[2];
int val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; int val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
int timeout = 10; int timeout = 10;
int err; int idx = 0, err;
err = snd_usb_autoresume(cval->mixer->chip); err = snd_usb_autoresume(cval->mixer->chip);
if (err < 0) if (err < 0)
return -EIO; return -EIO;
mutex_lock(&chip->shutdown_mutex);
while (timeout-- > 0) { while (timeout-- > 0) {
if (chip->shutdown)
break;
idx = snd_usb_ctrl_intf(chip) | (cval->id << 8);
if (snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), request, if (snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), request,
USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), validx, idx, buf, val_len) >= val_len) {
buf, val_len) >= val_len) {
*value_ret = convert_signed_value(cval, snd_usb_combine_bytes(buf, val_len)); *value_ret = convert_signed_value(cval, snd_usb_combine_bytes(buf, val_len));
snd_usb_autosuspend(cval->mixer->chip); err = 0;
return 0; goto out;
} }
} }
snd_usb_autosuspend(cval->mixer->chip);
snd_printdd(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n", snd_printdd(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
request, validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), cval->val_type); request, validx, idx, cval->val_type);
return -EINVAL; err = -EINVAL;
out:
mutex_unlock(&chip->shutdown_mutex);
snd_usb_autosuspend(cval->mixer->chip);
return err;
} }
static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret) static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret)
...@@ -313,7 +320,7 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v ...@@ -313,7 +320,7 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v
struct snd_usb_audio *chip = cval->mixer->chip; struct snd_usb_audio *chip = cval->mixer->chip;
unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */ unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */
unsigned char *val; unsigned char *val;
int ret, size; int idx = 0, ret, size;
__u8 bRequest; __u8 bRequest;
if (request == UAC_GET_CUR) { if (request == UAC_GET_CUR) {
...@@ -330,16 +337,22 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v ...@@ -330,16 +337,22 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v
if (ret) if (ret)
goto error; goto error;
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), bRequest, mutex_lock(&chip->shutdown_mutex);
if (chip->shutdown)
ret = -ENODEV;
else {
idx = snd_usb_ctrl_intf(chip) | (cval->id << 8);
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), bRequest,
USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), validx, idx, buf, size);
buf, size); }
mutex_unlock(&chip->shutdown_mutex);
snd_usb_autosuspend(chip); snd_usb_autosuspend(chip);
if (ret < 0) { if (ret < 0) {
error: error:
snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n", snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
request, validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), cval->val_type); request, validx, idx, cval->val_type);
return ret; return ret;
} }
...@@ -417,7 +430,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, ...@@ -417,7 +430,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
{ {
struct snd_usb_audio *chip = cval->mixer->chip; struct snd_usb_audio *chip = cval->mixer->chip;
unsigned char buf[2]; unsigned char buf[2];
int val_len, err, timeout = 10; int idx = 0, val_len, err, timeout = 10;
if (cval->mixer->protocol == UAC_VERSION_1) { if (cval->mixer->protocol == UAC_VERSION_1) {
val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
...@@ -440,19 +453,27 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, ...@@ -440,19 +453,27 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
err = snd_usb_autoresume(chip); err = snd_usb_autoresume(chip);
if (err < 0) if (err < 0)
return -EIO; return -EIO;
while (timeout-- > 0) mutex_lock(&chip->shutdown_mutex);
while (timeout-- > 0) {
if (chip->shutdown)
break;
idx = snd_usb_ctrl_intf(chip) | (cval->id << 8);
if (snd_usb_ctl_msg(chip->dev, if (snd_usb_ctl_msg(chip->dev,
usb_sndctrlpipe(chip->dev, 0), request, usb_sndctrlpipe(chip->dev, 0), request,
USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT,
validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), validx, idx, buf, val_len) >= 0) {
buf, val_len) >= 0) { err = 0;
snd_usb_autosuspend(chip); goto out;
return 0;
} }
snd_usb_autosuspend(chip); }
snd_printdd(KERN_ERR "cannot set ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d, data = %#x/%#x\n", snd_printdd(KERN_ERR "cannot set ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d, data = %#x/%#x\n",
request, validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), cval->val_type, buf[0], buf[1]); request, validx, idx, cval->val_type, buf[0], buf[1]);
return -EINVAL; err = -EINVAL;
out:
mutex_unlock(&chip->shutdown_mutex);
snd_usb_autosuspend(chip);
return err;
} }
static int set_cur_ctl_value(struct usb_mixer_elem_info *cval, int validx, int value) static int set_cur_ctl_value(struct usb_mixer_elem_info *cval, int validx, int value)
......
...@@ -71,6 +71,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream ...@@ -71,6 +71,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
unsigned int hwptr_done; unsigned int hwptr_done;
subs = (struct snd_usb_substream *)substream->runtime->private_data; subs = (struct snd_usb_substream *)substream->runtime->private_data;
if (subs->stream->chip->shutdown)
return SNDRV_PCM_POS_XRUN;
spin_lock(&subs->lock); spin_lock(&subs->lock);
hwptr_done = subs->hwptr_done; hwptr_done = subs->hwptr_done;
substream->runtime->delay = snd_usb_pcm_delay(subs, substream->runtime->delay = snd_usb_pcm_delay(subs,
...@@ -444,7 +446,6 @@ static int configure_endpoint(struct snd_usb_substream *subs) ...@@ -444,7 +446,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
{ {
int ret; int ret;
mutex_lock(&subs->stream->chip->shutdown_mutex);
/* format changed */ /* format changed */
stop_endpoints(subs, 0, 0, 0); stop_endpoints(subs, 0, 0, 0);
ret = snd_usb_endpoint_set_params(subs->data_endpoint, ret = snd_usb_endpoint_set_params(subs->data_endpoint,
...@@ -455,7 +456,7 @@ static int configure_endpoint(struct snd_usb_substream *subs) ...@@ -455,7 +456,7 @@ static int configure_endpoint(struct snd_usb_substream *subs)
subs->cur_audiofmt, subs->cur_audiofmt,
subs->sync_endpoint); subs->sync_endpoint);
if (ret < 0) if (ret < 0)
goto unlock; return ret;
if (subs->sync_endpoint) if (subs->sync_endpoint)
ret = snd_usb_endpoint_set_params(subs->data_endpoint, ret = snd_usb_endpoint_set_params(subs->data_endpoint,
...@@ -465,9 +466,6 @@ static int configure_endpoint(struct snd_usb_substream *subs) ...@@ -465,9 +466,6 @@ static int configure_endpoint(struct snd_usb_substream *subs)
subs->cur_rate, subs->cur_rate,
subs->cur_audiofmt, subs->cur_audiofmt,
NULL); NULL);
unlock:
mutex_unlock(&subs->stream->chip->shutdown_mutex);
return ret; return ret;
} }
...@@ -505,7 +503,13 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, ...@@ -505,7 +503,13 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
return -EINVAL; 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; return ret;
subs->interface = fmt->iface; subs->interface = fmt->iface;
...@@ -528,8 +532,10 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) ...@@ -528,8 +532,10 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
subs->cur_rate = 0; subs->cur_rate = 0;
subs->period_bytes = 0; subs->period_bytes = 0;
mutex_lock(&subs->stream->chip->shutdown_mutex); mutex_lock(&subs->stream->chip->shutdown_mutex);
stop_endpoints(subs, 0, 1, 1); if (!subs->stream->chip->shutdown) {
deactivate_endpoints(subs); stop_endpoints(subs, 0, 1, 1);
deactivate_endpoints(subs);
}
mutex_unlock(&subs->stream->chip->shutdown_mutex); mutex_unlock(&subs->stream->chip->shutdown_mutex);
return snd_pcm_lib_free_vmalloc_buffer(substream); return snd_pcm_lib_free_vmalloc_buffer(substream);
} }
...@@ -552,12 +558,19 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) ...@@ -552,12 +558,19 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
return -ENXIO; return -ENXIO;
} }
if (snd_BUG_ON(!subs->data_endpoint)) mutex_lock(&subs->stream->chip->shutdown_mutex);
return -EIO; 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); ret = set_format(subs, subs->cur_audiofmt);
if (ret < 0) if (ret < 0)
return ret; goto unlock;
iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface); iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
alts = &iface->altsetting[subs->cur_audiofmt->altset_idx]; alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
...@@ -567,12 +580,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) ...@@ -567,12 +580,12 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
subs->cur_audiofmt, subs->cur_audiofmt,
subs->cur_rate); subs->cur_rate);
if (ret < 0) if (ret < 0)
return ret; goto unlock;
if (subs->need_setup_ep) { if (subs->need_setup_ep) {
ret = configure_endpoint(subs); ret = configure_endpoint(subs);
if (ret < 0) if (ret < 0)
return ret; goto unlock;
subs->need_setup_ep = false; subs->need_setup_ep = false;
} }
...@@ -592,9 +605,11 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) ...@@ -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 /* for playback, submit the URBs now; otherwise, the first hwptr_done
* updates for all URBs would happen at the same time when starting */ * updates for all URBs would happen at the same time when starting */
if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) 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 = static struct snd_pcm_hardware snd_usb_hardware =
...@@ -647,7 +662,7 @@ static int hw_check_valid_format(struct snd_usb_substream *subs, ...@@ -647,7 +662,7 @@ static int hw_check_valid_format(struct snd_usb_substream *subs,
return 0; return 0;
} }
/* check whether the period time is >= the data packet interval */ /* 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); ptime = 125 * (1 << fp->datainterval);
if (ptime > pt->max || (ptime == pt->max && pt->openmax)) { if (ptime > pt->max || (ptime == pt->max && pt->openmax)) {
hwc_debug(" > check: ptime %u > max %u\n", ptime, pt->max); 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 ...@@ -925,7 +940,7 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre
return err; return err;
param_period_time_if_needed = SNDRV_PCM_HW_PARAM_PERIOD_TIME; 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 */ /* full speed devices have fixed data packet interval */
ptmin = 1000; ptmin = 1000;
if (ptmin == 1000) if (ptmin == 1000)
......
...@@ -108,7 +108,7 @@ static void proc_dump_substream_formats(struct snd_usb_substream *subs, struct s ...@@ -108,7 +108,7 @@ static void proc_dump_substream_formats(struct snd_usb_substream *subs, struct s
} }
snd_iprintf(buffer, "\n"); snd_iprintf(buffer, "\n");
} }
if (snd_usb_get_speed(subs->dev) != USB_SPEED_FULL) if (subs->speed != USB_SPEED_FULL)
snd_iprintf(buffer, " Data packet interval: %d us\n", snd_iprintf(buffer, " Data packet interval: %d us\n",
125 * (1 << fp->datainterval)); 125 * (1 << fp->datainterval));
// snd_iprintf(buffer, " Max Packet Size = %d\n", fp->maxpacksize); // snd_iprintf(buffer, " Max Packet Size = %d\n", fp->maxpacksize);
...@@ -124,7 +124,7 @@ static void proc_dump_ep_status(struct snd_usb_substream *subs, ...@@ -124,7 +124,7 @@ static void proc_dump_ep_status(struct snd_usb_substream *subs,
return; return;
snd_iprintf(buffer, " Packet Size = %d\n", ep->curpacksize); snd_iprintf(buffer, " Packet Size = %d\n", ep->curpacksize);
snd_iprintf(buffer, " Momentary freq = %u Hz (%#x.%04x)\n", snd_iprintf(buffer, " Momentary freq = %u Hz (%#x.%04x)\n",
snd_usb_get_speed(subs->dev) == USB_SPEED_FULL subs->speed == USB_SPEED_FULL
? get_full_speed_hz(ep->freqm) ? get_full_speed_hz(ep->freqm)
: get_high_speed_hz(ep->freqm), : get_high_speed_hz(ep->freqm),
ep->freqm >> 16, ep->freqm & 0xffff); ep->freqm >> 16, ep->freqm & 0xffff);
......
...@@ -90,6 +90,7 @@ static void snd_usb_init_substream(struct snd_usb_stream *as, ...@@ -90,6 +90,7 @@ static void snd_usb_init_substream(struct snd_usb_stream *as,
subs->direction = stream; subs->direction = stream;
subs->dev = as->chip->dev; subs->dev = as->chip->dev;
subs->txfr_quirk = as->chip->txfr_quirk; subs->txfr_quirk = as->chip->txfr_quirk;
subs->speed = snd_usb_get_speed(subs->dev);
snd_usb_set_pcm_ops(as->pcm, stream); snd_usb_set_pcm_ops(as->pcm, stream);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment