[alsa-devel] [PATCH 2/2] ALSA: snd-usb: move calls to usb_set_interface
Takashi Iwai
tiwai at suse.de
Fri Jul 13 07:58:18 CEST 2012
At Thu, 12 Jul 2012 15:19:47 +0200,
Daniel Mack wrote:
>
> The rework of the snd-usb endpoint logic moved the calls to
> snd_usb_set_interface() into the snd_usb_endpoint implemenation. This
> changed the order in which these calls are issued to the device, and
> thereby caused regressions for some webcams.
>
> Fix this by moving the calls back to pcm.c for now to make it work again
> and use snd_usb_endpoint_activate() to really tear down all remaining
> URBs in the flight, consequently fixing another regression caused by USB
> packets on the wire after altsetting 0 has been selected.
>
> Signed-off-by: Daniel Mack <zonque at gmail.com>
> Reported-and-tested-by: Philipp Dreimann <philipp at dreimann.net>
> Reported-by: Joseph Salisbury <joseph.salisbury at canonical.com>
Looks like the patch contains lots of coding-style issues.
Could you fix what checkpatch.pl compains and resubmit?
thanks,
Takashi
> ---
> sound/usb/endpoint.c | 73 +++++---------------------------------------------
> sound/usb/pcm.c | 55 ++++++++++++++++++++++---------------
> 2 files changed, 39 insertions(+), 89 deletions(-)
>
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index a75bdf4..4bc5778 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -415,7 +415,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
> {
> struct list_head *p;
> struct snd_usb_endpoint *ep;
> - int ret, is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
> + int is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
>
> mutex_lock(&chip->mutex);
>
> @@ -435,16 +435,6 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
> type == SND_USB_ENDPOINT_TYPE_DATA ? "data" : "sync",
> ep_num);
>
> - /* select the alt setting once so the endpoints become valid */
> - ret = usb_set_interface(chip->dev, alts->desc.bInterfaceNumber,
> - alts->desc.bAlternateSetting);
> - if (ret < 0) {
> - snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n",
> - __func__, ret);
> - ep = NULL;
> - goto __exit_unlock;
> - }
> -
> ep = kzalloc(sizeof(*ep), GFP_KERNEL);
> if (!ep)
> goto __exit_unlock;
> @@ -832,9 +822,6 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
> if (++ep->use_count != 1)
> return 0;
>
> - if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags)))
> - return -EINVAL;
> -
> /* just to be sure */
> deactivate_urbs(ep, 0, 1);
> wait_clear_urbs(ep);
> @@ -912,9 +899,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep,
> if (snd_BUG_ON(ep->use_count == 0))
> return;
>
> - if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags)))
> - return;
> -
> if (--ep->use_count == 0) {
> deactivate_urbs(ep, force, can_sleep);
> ep->data_subs = NULL;
> @@ -928,42 +912,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep,
> }
>
> /**
> - * snd_usb_endpoint_activate: activate an snd_usb_endpoint
> - *
> - * @ep: the endpoint to activate
> - *
> - * If the endpoint is not currently in use, this functions will select the
> - * correct alternate interface setting for the interface of this endpoint.
> - *
> - * In case of any active users, this functions does nothing.
> - *
> - * Returns an error if usb_set_interface() failed, 0 in all other
> - * cases.
> - */
> -int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep)
> -{
> - if (ep->use_count != 0)
> - return 0;
> -
> - if (!ep->chip->shutdown &&
> - !test_and_set_bit(EP_FLAG_ACTIVATED, &ep->flags)) {
> - int ret;
> -
> - ret = usb_set_interface(ep->chip->dev, ep->iface, ep->alt_idx);
> - if (ret < 0) {
> - snd_printk(KERN_ERR "%s() usb_set_interface() failed, ret = %d\n",
> - __func__, ret);
> - clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
> - return ret;
> - }
> -
> - return 0;
> - }
> -
> - return -EBUSY;
> -}
> -
> -/**
> * snd_usb_endpoint_deactivate: deactivate an snd_usb_endpoint
> *
> * @ep: the endpoint to deactivate
> @@ -981,24 +929,15 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
> if (!ep)
> return -EINVAL;
>
> + deactivate_urbs(ep, 1, 1);
> + wait_clear_urbs(ep);
> +
> if (ep->use_count != 0)
> return 0;
>
> - if (!ep->chip->shutdown &&
> - test_and_clear_bit(EP_FLAG_ACTIVATED, &ep->flags)) {
> - int ret;
> -
> - ret = usb_set_interface(ep->chip->dev, ep->iface, 0);
> - if (ret < 0) {
> - snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n",
> - __func__, ret);
> - return ret;
> - }
> + clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
>
> - return 0;
> - }
> -
> - return -EBUSY;
> + return 0;
> }
>
> /**
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index f0ede13..26778a9 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -261,19 +261,6 @@ static void stop_endpoints(struct snd_usb_substream *subs,
> force, can_sleep, wait);
> }
>
> -static int activate_endpoints(struct snd_usb_substream *subs)
> -{
> - if (subs->sync_endpoint) {
> - int ret;
> -
> - ret = snd_usb_endpoint_activate(subs->sync_endpoint);
> - if (ret < 0)
> - return ret;
> - }
> -
> - return snd_usb_endpoint_activate(subs->data_endpoint);
> -}
> -
> static int deactivate_endpoints(struct snd_usb_substream *subs)
> {
> int reta, retb;
> @@ -314,6 +301,31 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
> if (fmt == subs->cur_audiofmt)
> return 0;
>
> + /* close the old interface */
> + if (subs->interface >= 0 && subs->interface != fmt->iface) {
> + err = usb_set_interface(subs->dev, subs->interface, 0);
> + if (err < 0) {
> + snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n",
> + dev->devnum, fmt->iface, fmt->altsetting, err);
> + return -EIO;
> + }
> + subs->interface = -1;
> + subs->altset_idx = 0;
> + }
> +
> + /* set interface */
> + if (subs->interface != fmt->iface || subs->altset_idx != fmt->altset_idx) {
> + err = usb_set_interface(dev, fmt->iface, fmt->altsetting);
> + if (err < 0) {
> + snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
> + dev->devnum, fmt->iface, fmt->altsetting, err);
> + return -EIO;
> + }
> + snd_printdd(KERN_INFO "setting usb interface %d:%d\n", fmt->iface, fmt->altsetting);
> + subs->interface = fmt->iface;
> + subs->altset_idx = fmt->altset_idx;
> + }
> +
> subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
> alts, fmt->endpoint, subs->direction,
> SND_USB_ENDPOINT_TYPE_DATA);
> @@ -460,12 +472,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
> mutex_lock(&subs->stream->chip->shutdown_mutex);
> /* format changed */
> stop_endpoints(subs, 0, 0, 0);
> - deactivate_endpoints(subs);
> -
> - ret = activate_endpoints(subs);
> - if (ret < 0)
> - goto unlock;
> -
> ret = snd_usb_endpoint_set_params(subs->data_endpoint, hw_params, fmt,
> subs->sync_endpoint);
> if (ret < 0)
> @@ -500,6 +506,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
> subs->period_bytes = 0;
> mutex_lock(&subs->stream->chip->shutdown_mutex);
> stop_endpoints(subs, 0, 1, 1);
> + deactivate_endpoints(subs);
> mutex_unlock(&subs->stream->chip->shutdown_mutex);
> return snd_pcm_lib_free_vmalloc_buffer(substream);
> }
> @@ -938,16 +945,20 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
>
> static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
> {
> - int ret;
> struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
> struct snd_usb_substream *subs = &as->substream[direction];
>
> stop_endpoints(subs, 0, 0, 0);
> - ret = deactivate_endpoints(subs);
> +
> + if (!as->chip->shutdown && subs->interface >= 0) {
> + usb_set_interface(subs->dev, subs->interface, 0);
> + subs->interface = -1;
> + }
> +
> subs->pcm_substream = NULL;
> snd_usb_autosuspend(subs->stream->chip);
>
> - return ret;
> + return 0;
> }
>
> /* Since a URB can handle only a single linear buffer, we must use double
> --
> 1.7.10.4
>
More information about the Alsa-devel
mailing list