At Thu, 01 Nov 2012 09:53:36 -0700, <[email protected]> wrote: > > > The patch below does not apply to the 3.6-stable tree. > If someone wants it applied there, or to any other stable or longterm > tree, then please email the backport, including the original git commit > id to <[email protected]>.
OK, I'm going to rebase the patches for stable kernels and repost in the next week (as we have a long weekend here in Germany now). thanks, Takashi > > thanks, > > greg k-h > > ------------------ original commit in Linus's tree ------------------ > > >From 978520b75f0a1ce82b17e1e8186417250de6d545 Mon Sep 17 00:00:00 2001 > From: Takashi Iwai <[email protected]> > Date: Fri, 12 Oct 2012 15:12:55 +0200 > Subject: [PATCH] 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 <[email protected]> > Cc: <[email protected]> > Signed-off-by: Takashi Iwai <[email protected]> > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index afa4f9e..814cb35 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -126,6 +126,7 @@ struct snd_usb_substream { > struct snd_usb_endpoint *sync_endpoint; > unsigned long flags; > bool need_setup_ep; /* (re)configure EP at prepare? */ > + unsigned int speed; /* USB_SPEED_XXX */ > > u64 formats; /* format bitmasks (all or'ed) */ > unsigned int num_formats; /* number of supported audio > formats (list) */ > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c > index fe56c9d..c2ef11c 100644 > --- a/sound/usb/mixer.c > +++ b/sound/usb/mixer.c > @@ -287,25 +287,32 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info > *cval, int request, int v > unsigned char buf[2]; > int val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1; > int timeout = 10; > - int err; > + int idx = 0, err; > > err = snd_usb_autoresume(cval->mixer->chip); > if (err < 0) > return -EIO; > + 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, usb_rcvctrlpipe(chip->dev, 0), > request, > USB_RECIP_INTERFACE | USB_TYPE_CLASS | > USB_DIR_IN, > - validx, snd_usb_ctrl_intf(chip) | (cval->id > << 8), > - buf, val_len) >= val_len) { > + validx, idx, buf, val_len) >= val_len) { > *value_ret = convert_signed_value(cval, > snd_usb_combine_bytes(buf, val_len)); > - snd_usb_autosuspend(cval->mixer->chip); > - return 0; > + err = 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", > - request, validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), > cval->val_type); > - return -EINVAL; > + request, validx, idx, cval->val_type); > + 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) > @@ -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; > unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */ > unsigned char *val; > - int ret, size; > + int idx = 0, ret, size; > __u8 bRequest; > > 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 > if (ret) > 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, > - validx, snd_usb_ctrl_intf(chip) | (cval->id << 8), > - buf, size); > + validx, idx, buf, size); > + } > + mutex_unlock(&chip->shutdown_mutex); > snd_usb_autosuspend(chip); > > if (ret < 0) { > error: > 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; > } > > @@ -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; > 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) { > 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, > err = snd_usb_autoresume(chip); > if (err < 0) > 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, > usb_sndctrlpipe(chip->dev, 0), request, > USB_RECIP_INTERFACE | USB_TYPE_CLASS | > USB_DIR_OUT, > - validx, snd_usb_ctrl_intf(chip) | (cval->id > << 8), > - buf, val_len) >= 0) { > - snd_usb_autosuspend(chip); > - return 0; > + validx, idx, buf, val_len) >= 0) { > + err = 0; > + goto out; > } > - snd_usb_autosuspend(chip); > + } > 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]); > - return -EINVAL; > + request, validx, idx, cval->val_type, buf[0], buf[1]); > + 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) > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 55e19e1..55e741c 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) > diff --git a/sound/usb/proc.c b/sound/usb/proc.c > index ebc1a5b..d218f76 100644 > --- a/sound/usb/proc.c > +++ b/sound/usb/proc.c > @@ -108,7 +108,7 @@ static void proc_dump_substream_formats(struct > snd_usb_substream *subs, struct s > } > 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", > 125 * (1 << fp->datainterval)); > // 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, > return; > snd_iprintf(buffer, " Packet Size = %d\n", ep->curpacksize); > 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_high_speed_hz(ep->freqm), > ep->freqm >> 16, ep->freqm & 0xffff); > diff --git a/sound/usb/stream.c b/sound/usb/stream.c > index 083ed81..1de0c8c 100644 > --- a/sound/usb/stream.c > +++ b/sound/usb/stream.c > @@ -90,6 +90,7 @@ static void snd_usb_init_substream(struct snd_usb_stream > *as, > subs->direction = stream; > subs->dev = as->chip->dev; > subs->txfr_quirk = as->chip->txfr_quirk; > + subs->speed = snd_usb_get_speed(subs->dev); > > snd_usb_set_pcm_ops(as->pcm, stream); > > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
