[PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Daniel Mack]

In endpoint.c, bail out earlier in case the stream is stopped. Also
clear the flag from deactivate_urbs() in case the chip's shutdown
flag is set.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 sound/usb/endpoint.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index e690690..a75bdf4 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
 		     urb->status == -ENODEV ||		/* device removed */
 		     urb->status == -ECONNRESET ||	/* unlinked */
 		     urb->status == -ESHUTDOWN ||	/* device disabled */
-		     ep->chip->shutdown))		/* device disconnected */
+		     ep->chip->shutdown) ||		/* device disconnected */
+		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
 		goto exit_clear;
 
 	if (usb_pipeout(ep->pipe)) {
@@ -522,13 +523,13 @@ static int deactivate_urbs(struct snd_usb_endpoint *ep, int force, int can_sleep
 	unsigned int i;
 	int async;
 
+	clear_bit(EP_FLAG_RUNNING, &ep->flags);
+
 	if (!force && ep->chip->shutdown) /* to be sure... */
 		return -EBADFD;
 
 	async = !can_sleep && ep->chip->async_unlink;
 
-	clear_bit(EP_FLAG_RUNNING, &ep->flags);
-
 	INIT_LIST_HEAD(&ep->ready_playback_urbs);
 	ep->next_packet_read_pos = 0;
 	ep->next_packet_write_pos = 0;
-- 
1.7.10.4

[PATCH 2/2] ALSA: snd-usb: move calls to usb_set_interface

[Daniel Mack]

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@gmail.com>
Reported-and-tested-by: Philipp Dreimann <philipp@dreimann.net>
Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
---
 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

Re: [PATCH 2/2] ALSA: snd-usb: move calls to usb_set_interface

[Takashi Iwai]

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@gmail.com>
> Reported-and-tested-by: Philipp Dreimann <philipp@dreimann.net>
> Reported-by: Joseph Salisbury <joseph.salisbury@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
> 

Re: [PATCH 2/2] ALSA: snd-usb: move calls to usb_set_interface

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On 13.07.2012 07:58, Takashi Iwai wrote:
> 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@gmail.com>
>> Reported-and-tested-by: Philipp Dreimann <philipp@dreimann.net>
>> Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> 
> Looks like the patch contains lots of coding-style issues.
> Could you fix what checkpatch.pl compains and resubmit?

Oops, sorry. Was working on a different machine for this, hence I didn't
notice.

Daniel


[-- Attachment #2: 0001-ALSA-snd-usb-move-calls-to-usb_set_interface.patch --]
[-- Type: text/x-patch, Size: 7668 bytes --]

>From 6b62323f3d7a28fdeece03dd40a2eb66d0f8b9dd Mon Sep 17 00:00:00 2001
From: Daniel Mack <zonque@gmail.com>
Date: Thu, 12 Jul 2012 13:08:40 +0200
Subject: [PATCH v2] ALSA: snd-usb: move calls to usb_set_interface

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@gmail.com>
Reported-and-tested-by: Philipp Dreimann <philipp@dreimann.net>
Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
---
 sound/usb/endpoint.c |   73 +++++---------------------------------------------
 sound/usb/pcm.c      |   57 ++++++++++++++++++++++++---------------
 2 files changed, 41 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..a1298f3 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,33 @@ 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 +474,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 +508,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 +947,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ALSA: snd-usb: move calls to usb_set_interface

[Takashi Iwai]

At Fri, 13 Jul 2012 08:08:26 +0200,
Daniel Mack wrote:
> 
> On 13.07.2012 07:58, Takashi Iwai wrote:
> > 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@gmail.com>
> >> Reported-and-tested-by: Philipp Dreimann <philipp@dreimann.net>
> >> Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> > 
> > Looks like the patch contains lots of coding-style issues.
> > Could you fix what checkpatch.pl compains and resubmit?
> 
> Oops, sorry. Was working on a different machine for this, hence I didn't
> notice.

Applied now.  Thanks.


Takashi

Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Clemens Ladisch]

Daniel Mack wrote:
> In endpoint.c, bail out earlier in case the stream is stopped.
> ...
> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>  		     urb->status == -ENODEV ||		/* device removed */
>  		     urb->status == -ECONNRESET ||	/* unlinked */
>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
> -		     ep->chip->shutdown))		/* device disconnected */
> +		     ep->chip->shutdown) ||		/* device disconnected */
> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>  		goto exit_clear;

Is this really needed?
The URBs will be unlinked at the same time.


Regards,
Clemens

Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Daniel Mack]

Hi Clemens,

On 12.07.2012 16:29, Clemens Ladisch wrote:
> Daniel Mack wrote:
>> In endpoint.c, bail out earlier in case the stream is stopped.
>> ...
>> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>>  		     urb->status == -ENODEV ||		/* device removed */
>>  		     urb->status == -ECONNRESET ||	/* unlinked */
>>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
>> -		     ep->chip->shutdown))		/* device disconnected */
>> +		     ep->chip->shutdown) ||		/* device disconnected */
>> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>>  		goto exit_clear;
> 
> Is this really needed?
> The URBs will be unlinked at the same time.

This just brings the code in sync with what we had before. If URBs are
just unlinked but not killed, they will return with data payload, and in
case of implicit feedback streams, the retire code could issue new
output packets. Might be I overlooked a reason why that could never
happen, but even if I did, the check here doesn't hurt, right?


Daniel

Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Clemens Ladisch]

Daniel Mack wrote:
> On 12.07.2012 16:29, Clemens Ladisch wrote:
>> Daniel Mack wrote:
>>> In endpoint.c, bail out earlier in case the stream is stopped.
>>> ...
>>> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>>>  		     urb->status == -ENODEV ||		/* device removed */
>>>  		     urb->status == -ECONNRESET ||	/* unlinked */
>>>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
>>> -		     ep->chip->shutdown))		/* device disconnected */
>>> +		     ep->chip->shutdown) ||		/* device disconnected */
>>> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>>>  		goto exit_clear;
>>
>> Is this really needed?
>> The URBs will be unlinked at the same time.
>
> This just brings the code in sync with what we had before. If URBs are
> just unlinked but not killed, they will return with data payload, and in
> case of implicit feedback streams, the retire code could issue new
> output packets.

And is that bad?

The EP_FLAG_RUNNING bit is cleared immediately before the URBs are
unlinked, and some URB could have been completed regularly immediately
before that.  So if there is any case where the additional test_bit()
call prevents submitting a new playback packet, there is another similar
case where you've just got such a playback packet anyway.

In other words: if that test_bit() call were necessary to protect
against something bad, it wouldn't be sufficient.

The driver ensures that (implicit feedback) playback URBs are unlinked
only after the corresponding capture URBs have been completely killed,
doesn't it?


Regards,
Clemens

Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Daniel Mack]

On 12.07.2012 17:20, Clemens Ladisch wrote:
> Daniel Mack wrote:
>> On 12.07.2012 16:29, Clemens Ladisch wrote:
>>> Daniel Mack wrote:
>>>> In endpoint.c, bail out earlier in case the stream is stopped.
>>>> ...
>>>> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>>>>  		     urb->status == -ENODEV ||		/* device removed */
>>>>  		     urb->status == -ECONNRESET ||	/* unlinked */
>>>>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
>>>> -		     ep->chip->shutdown))		/* device disconnected */
>>>> +		     ep->chip->shutdown) ||		/* device disconnected */
>>>> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>>>>  		goto exit_clear;
>>>
>>> Is this really needed?
>>> The URBs will be unlinked at the same time.
>>
>> This just brings the code in sync with what we had before. If URBs are
>> just unlinked but not killed, they will return with data payload, and in
>> case of implicit feedback streams, the retire code could issue new
>> output packets.
> 
> And is that bad?
> 
> The EP_FLAG_RUNNING bit is cleared immediately before the URBs are
> unlinked, and some URB could have been completed regularly immediately
> before that.  So if there is any case where the additional test_bit()
> call prevents submitting a new playback packet, there is another similar
> case where you've just got such a playback packet anyway.
> 
> In other words: if that test_bit() call were necessary to protect
> against something bad, it wouldn't be sufficient.
> 
> The driver ensures that (implicit feedback) playback URBs are unlinked
> only after the corresponding capture URBs have been completely killed,
> doesn't it?

You're right. Especially because sending out packets in implicit
feedback mode happens from queue_pending_output_urbs() which has an
extra chip.

So we can drop the whole patch then. Thanks for the review!


Daniel

Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Daniel Mack]

On 12.07.2012 21:19, Joseph Salisbury wrote:
> On 07/12/2012 11:27 AM, Daniel Mack wrote:
>> On 12.07.2012 17:20, Clemens Ladisch wrote:
>>> Daniel Mack wrote:
>>>> On 12.07.2012 16:29, Clemens Ladisch wrote:
>>>>> Daniel Mack wrote:
>>>>>> In endpoint.c, bail out earlier in case the stream is stopped.
>>>>>> ...
>>>>>> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>>>>>>  		     urb->status == -ENODEV ||		/* device removed */
>>>>>>  		     urb->status == -ECONNRESET ||	/* unlinked */
>>>>>>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
>>>>>> -		     ep->chip->shutdown))		/* device disconnected */
>>>>>> +		     ep->chip->shutdown) ||		/* device disconnected */
>>>>>> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>>>>>>  		goto exit_clear;
>>>>>
>>>>> Is this really needed?
>>>>> The URBs will be unlinked at the same time.
>>>>
>>>> This just brings the code in sync with what we had before. If URBs are
>>>> just unlinked but not killed, they will return with data payload, and in
>>>> case of implicit feedback streams, the retire code could issue new
>>>> output packets.
>>>
>>> And is that bad?
>>>
>>> The EP_FLAG_RUNNING bit is cleared immediately before the URBs are
>>> unlinked, and some URB could have been completed regularly immediately
>>> before that.  So if there is any case where the additional test_bit()
>>> call prevents submitting a new playback packet, there is another similar
>>> case where you've just got such a playback packet anyway.
>>>
>>> In other words: if that test_bit() call were necessary to protect
>>> against something bad, it wouldn't be sufficient.
>>>
>>> The driver ensures that (implicit feedback) playback URBs are unlinked
>>> only after the corresponding capture URBs have been completely killed,
>>> doesn't it?
>>
>> You're right. Especially because sending out packets in implicit
>> feedback mode happens from queue_pending_output_urbs() which has an
>> extra chip.
>>
>> So we can drop the whole patch then. Thanks for the review!
>>
>>
>> Daniel
> 
> Hi Daniel,
> 
> I tested without your first patch.  I'm not sure why, but dropping the
> first patch(ALSA: snd-usb: tighten EP_FLAG_RUNNING checks) causes a long
> delay, about 30 seconds, during bootup of an ubuntu system.
> 
> During the delay, I am able to enter my password, but the system seems
> to be doing something in the background, slowing the login process.
> 
> I confirmed that adding the patch back causes the delay to go away.

That really sounds odd and totally unrelated. Is there any message in
dmesg that looks suspicious? Otherwise, I would ask you to test again.
Could be it was caused by something that runs on every n'th boot or so.


Daniel