[PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage

[PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage

[Eldad Zack]

Hi,

This patch series attempts to fix a long standing problem with the concurrent
usage of playback and capture on implicit feedback devices.

Applies against current for-next (HEAD 68538bf2bce557c3b5fe8c59b034d45352500db1 ).

v2:
 http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html
v1:
 http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html

 * Changed "altset_idx" to "altsetting" thanks to Clemens' clarification.

 #3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace calling
     snd_pcm_open() and then snd_pcm_close() immediately.
 #8: subs->altset_idx was left intact, since it corresponds to the array index.
 #10: this patch now removed subs->altset_idx, but also changes the check to be
      performed against fmt->altsetting.
 #11: New. This changes the name of snd_usb_endpoint member alt_idx to altsetting
      for the same reason Clemens described.

Clemens and Daniel - thank you very much for your help!

Cheers,
Eldad

Eldad Zack (11):
  ALSA: usb-audio: remove unused parameter from sync_ep_set_params
  ALSA: usb-audio: remove deactivate_endpoints()
  ALSA: usb-audio: prevent NULL dereference on stop trigger
  ALSA: usb-audio: don't deactivate URBs on in-use EP
  ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate()
  ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error
  ALSA: usb-audio: correct ep use_count semantics (add set_param flag)
  ALSA: usb-audio: conditional interface altsetting
  ALSA: usb-audio: conditional concurrent usage of endpoint
  ALSA: usb-audio: remove altset_idx from snd_usb_substream
  ALSA: usb-audio: rename alt_idx to altsetting

 sound/usb/card.h     |   4 +-
 sound/usb/endpoint.c |  82 ++++++++++++++++++-------
 sound/usb/endpoint.h |  11 +++-
 sound/usb/pcm.c      | 168 ++++++++++++++++++++++++++++++++++++++-------------
 sound/usb/pcm.h      |   2 +
 sound/usb/proc.c     |   4 +-
 6 files changed, 201 insertions(+), 70 deletions(-)

-- 
1.8.1.5

[PATCH v3 01/11] ALSA: usb-audio: remove unused parameter from sync_ep_set_params

[Eldad Zack]

Since the format is not actually used in sync_ep_set_params(),
there is no need to pass it down.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 93e970f..86b23af 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -703,8 +703,7 @@ out_of_memory:
 /*
  * configure a sync endpoint
  */
-static int sync_ep_set_params(struct snd_usb_endpoint *ep,
-			      struct audioformat *fmt)
+static int sync_ep_set_params(struct snd_usb_endpoint *ep)
 {
 	int i;
 
@@ -796,7 +795,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 					 period_bytes, fmt, sync_ep);
 		break;
 	case  SND_USB_ENDPOINT_TYPE_SYNC:
-		err = sync_ep_set_params(ep, fmt);
+		err = sync_ep_set_params(ep);
 		break;
 	default:
 		err = -EINVAL;
-- 
1.8.1.5

[PATCH v3 02/11] ALSA: usb-audio: remove deactivate_endpoints()

[Eldad Zack]

The only call site for deactivate_endpoints() at snd_usb_hw_free().
The return value is not checked there, as it is irrelevant if it
fails on hw_free.
This patch moves the deactivation of the endpoints directly into
snd_usb_hw_free().

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b375d58..9ec401a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -282,22 +282,6 @@ static void stop_endpoints(struct snd_usb_substream *subs, bool wait)
 	}
 }
 
-static int deactivate_endpoints(struct snd_usb_substream *subs)
-{
-	int reta, retb;
-
-	reta = snd_usb_endpoint_deactivate(subs->sync_endpoint);
-	retb = snd_usb_endpoint_deactivate(subs->data_endpoint);
-
-	if (reta < 0)
-		return reta;
-
-	if (retb < 0)
-		return retb;
-
-	return 0;
-}
-
 static int search_roland_implicit_fb(struct usb_device *dev, int ifnum,
 				     unsigned int altsetting,
 				     struct usb_host_interface **alts,
@@ -730,7 +714,8 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
 	down_read(&subs->stream->chip->shutdown_rwsem);
 	if (!subs->stream->chip->shutdown) {
 		stop_endpoints(subs, true);
-		deactivate_endpoints(subs);
+		snd_usb_endpoint_deactivate(subs->sync_endpoint);
+		snd_usb_endpoint_deactivate(subs->data_endpoint);
 	}
 	up_read(&subs->stream->chip->shutdown_rwsem);
 	return snd_pcm_lib_free_vmalloc_buffer(substream);
-- 
1.8.1.5

[PATCH v3 03/11] ALSA: usb-audio: prevent NULL dereference on stop trigger

[Eldad Zack]

If an endpoint uses another endpoint for synchronization, and the
other endpoint is stopped, an oops will occur on NULL dereference.
Clearing the prepare/retire callbacks solves this issue.

v2: Thanks to Daniel Mack, fixed (an irnoic) NULL dereference when
the pcm substream is opened and closed immediately.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 9ec401a..4227d34 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1199,6 +1199,11 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
 		subs->interface = -1;
 	}
 
+	if (subs->data_endpoint) {
+		subs->data_endpoint->prepare_data_urb = NULL;
+		subs->data_endpoint->retire_data_urb = NULL;
+	}
+
 	subs->pcm_substream = NULL;
 	snd_usb_autosuspend(subs->stream->chip);
 
@@ -1525,6 +1530,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 		subs->running = 1;
 		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
+		subs->data_endpoint->prepare_data_urb = NULL;
+		subs->data_endpoint->retire_data_urb = NULL;
 		stop_endpoints(subs, false);
 		subs->running = 0;
 		return 0;
@@ -1555,6 +1562,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream
 		subs->running = 1;
 		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
+		subs->data_endpoint->retire_data_urb = NULL;
 		stop_endpoints(subs, false);
 		subs->running = 0;
 		return 0;
-- 
1.8.1.5

[PATCH v3 04/11] ALSA: usb-audio: don't deactivate URBs on in-use EP

[Eldad Zack]

If an endpoint in use, its associated URBs should not be
deactivated.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 86b23af..420df01 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -943,12 +943,12 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
 	if (!ep)
 		return -EINVAL;
 
-	deactivate_urbs(ep, true);
-	wait_clear_urbs(ep);
-
 	if (ep->use_count != 0)
 		return 0;
 
+	deactivate_urbs(ep, true);
+	wait_clear_urbs(ep);
+
 	clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
 
 	return 0;
-- 
1.8.1.5

[PATCH v3 05/11] ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate()

[Eldad Zack]

The return value of snd_usb_endpoint_deactivate() is not used,
make the function have no return value.
Update the documentation to reflect what the function is actually
doing.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c | 15 +++++----------
 sound/usb/endpoint.h |  2 +-
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 420df01..c6fcb7b 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -930,28 +930,23 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep)
  *
  * @ep: the endpoint to deactivate
  *
- * If the endpoint is not currently in use, this functions will select the
- * alternate interface setting 0 for the interface of this endpoint.
+ * If the endpoint is not currently in use, this functions will
+ * deactivate its associated URBs.
  *
  * 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_deactivate(struct snd_usb_endpoint *ep)
+void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
 {
 	if (!ep)
-		return -EINVAL;
+		return;
 
 	if (ep->use_count != 0)
-		return 0;
+		return;
 
 	deactivate_urbs(ep, true);
 	wait_clear_urbs(ep);
 
 	clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
-
-	return 0;
 }
 
 /**
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 2287adf..c928e0c 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -20,7 +20,7 @@ int  snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep);
 void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep);
 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_deactivate(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_free(struct list_head *head);
 
 int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
-- 
1.8.1.5

[PATCH v3 06/11] ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error

[Eldad Zack]

If setting the interface fails, the SUBSTREAM_FLAG_SYNC_EP_STARTED
should be cleared.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 4227d34..ef9664f 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -246,6 +246,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
 						subs->sync_endpoint->iface,
 						subs->sync_endpoint->alt_idx);
 			if (err < 0) {
+				clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
 				snd_printk(KERN_ERR
 					   "%d:%d:%d: cannot set interface (%d)\n",
 					   subs->dev->devnum,
-- 
1.8.1.5

[PATCH v3 07/11] ALSA: usb-audio: correct ep use_count semantics (add set_param flag)

[Eldad Zack]

Currently, use_count is used in snd_usb_endpoint_set_params to
ensure the parameters don't get changed for an in-use endpoint.

However, there is a subtle condition where this check fails -
if hw_params is called on both substreams before calling prepare (for
playback) or start trigger (for capture): the endpoint is not yet
started, i.e., snd_usb_endpoint_start() does not yet increment use_count.

This adds a flag to check if the parameters are set, but does not
omit checking the use_count.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

---
v2: Cleaned up the patch, now it does only one thing (add the flag and
the check).
---
 sound/usb/card.h     | 1 +
 sound/usb/endpoint.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 5ecacaa..4061ee1 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -51,6 +51,7 @@ struct snd_usb_endpoint {
 	struct snd_usb_audio *chip;
 
 	int use_count;
+	bool param_set;
 	int ep_num;		/* the referenced endpoint number */
 	int type;		/* SND_USB_ENDPOINT_TYPE_* */
 	unsigned long flags;
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c6fcb7b..6ff36f5 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -472,6 +472,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 		ep->syncmaxsize = le16_to_cpu(get_endpoint(alts, 1)->wMaxPacketSize);
 	}
 
+	ep->param_set = false;
+
 	list_add_tail(&ep->list, &chip->ep_list);
 
 __exit_unlock:
@@ -765,11 +767,12 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 {
 	int err;
 
-	if (ep->use_count != 0) {
+	if (ep->param_set || ep->use_count != 0) {
 		snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n",
 			   ep->ep_num);
 		return -EBUSY;
 	}
+	ep->param_set = true;
 
 	/* release old buffers, if any */
 	release_urbs(ep, 0);
@@ -921,6 +924,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep)
 		ep->sync_slave = NULL;
 		ep->retire_data_urb = NULL;
 		ep->prepare_data_urb = NULL;
+		ep->param_set = false;
 		set_bit(EP_FLAG_STOPPING, &ep->flags);
 	}
 }
-- 
1.8.1.5

[PATCH v3 08/11] ALSA: usb-audio: conditional interface altsetting

[Eldad Zack]

On some devices, if the endpoint for the other direction is in use,
setting one interface will shutdown the other (in use) endpoint.
This patch moves all of the alternate setting operations for pcm
ops to one function which checks if we can do so.

If current alternate is 0, it is safe to set.

Thanks to Clemens Ladisch for pointing out that cur_altsetting
in usb_interface can be used to determine the current altsetting
index as well as the correct naming convention for the altsetting
number.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

---
v2:
* Removed get_interface usage.
* Fixed NULL dereference for explicit feedback - now other_subs will
  be set only after testing that subs is not NULL.
v3:
* Changed "altset_idx" to "altsetting".
---
 sound/usb/pcm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index ef9664f..a1332d0 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -216,6 +216,71 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 	}
 }
 
+static int get_cur_altsetting(struct usb_device *dev, int ifnum)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *altsd;
+
+	if (ifnum == -1)
+		return 0;
+
+	iface = usb_ifnum_to_if(dev, ifnum);
+	if (!iface)
+		return -EINVAL;
+
+	if (!iface->cur_altsetting)
+		return -EINVAL;
+	altsd = &(iface->cur_altsetting->desc);
+
+	return altsd->bAlternateSetting;
+}
+
+static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
+			      int altsetting)
+{
+	struct snd_usb_substream *other_subs;
+	int err;
+
+	snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n",
+		    __func__, ifnum, altsetting);
+
+	if (subs == NULL)
+		return usb_set_interface(subs->dev, ifnum, altsetting);
+	other_subs = &(subs->stream->substream[subs->direction ^ 1]);
+
+	if (other_subs->data_endpoint &&
+	    other_subs->data_endpoint->use_count != 0) {
+		int cur_altsetting = get_cur_altsetting(subs->dev, ifnum);
+
+		if (cur_altsetting == altsetting)
+			return 0;
+
+		if (cur_altsetting > 0 && altsetting == 0)
+			return 0;
+	}
+
+	err = usb_set_interface(subs->dev, ifnum, altsetting);
+	snd_printdd(KERN_DEBUG "%s: Interface %d alts set to %d (err %d)\n",
+		    __func__, ifnum, altsetting, err);
+	if (err < 0)
+		return err;
+
+	subs->interface = altsetting == 0 ? -1 : ifnum;
+
+	return 0;
+}
+
+int snd_usb_set_interface(struct snd_usb_substream *subs, int ifnum,
+			  int altset_idx)
+{
+	return subs_set_interface(subs, ifnum, altset_idx);
+}
+
+int snd_usb_close_interface(struct snd_usb_substream *subs, int ifnum)
+{
+	return subs_set_interface(subs, ifnum, 0);
+}
+
 static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
 {
 	int err;
@@ -242,9 +307,9 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
 
 		if (subs->data_endpoint->iface != subs->sync_endpoint->iface ||
 		    subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) {
-			err = usb_set_interface(subs->dev,
-						subs->sync_endpoint->iface,
-						subs->sync_endpoint->alt_idx);
+			err = snd_usb_set_interface(subs,
+						    subs->sync_endpoint->iface,
+						    subs->sync_endpoint->alt_idx);
 			if (err < 0) {
 				clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
 				snd_printk(KERN_ERR
@@ -467,20 +532,19 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 
 	/* close the old interface */
 	if (subs->interface >= 0 && subs->interface != fmt->iface) {
-		err = usb_set_interface(subs->dev, subs->interface, 0);
+		err = snd_usb_close_interface(subs, subs->interface);
 		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);
+		err = snd_usb_set_interface(subs, 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);
@@ -488,7 +552,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 		}
 		snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
 				fmt->iface, fmt->altsetting);
-		subs->interface = fmt->iface;
 		subs->altset_idx = fmt->altset_idx;
 
 		snd_usb_set_interface_quirk(dev);
@@ -1196,7 +1259,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
 	stop_endpoints(subs, true);
 
 	if (!as->chip->shutdown && subs->interface >= 0) {
-		usb_set_interface(subs->dev, subs->interface, 0);
+		snd_usb_close_interface(subs, subs->interface);
 		subs->interface = -1;
 	}
 
-- 
1.8.1.5

[PATCH v3 09/11] ALSA: usb-audio: conditional concurrent usage of endpoint

[Eldad Zack]

If the current endpoint is in use, check if the hardware parameters
match. If they do, allow the usage.
This also requires setting the parameters in case an data endpoint
is used as a synchornization source, and it is first set by its
sink endpoint.

Add the same check to hw_params, to prevent overwriting the
current params.
Clear the hardware parameteres on hw_free only if the other endpoint
is not in use.

No change for sync endpoints (explicit feedback).

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
 sound/usb/endpoint.h |  9 ++++++++-
 sound/usb/pcm.c      | 51 +++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 6ff36f5..7e04ebb 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -742,6 +742,34 @@ out_of_memory:
 	return -ENOMEM;
 }
 
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs,
+				     snd_pcm_format_t pcm_format,
+				     unsigned int channels,
+				     unsigned int period_bytes,
+				     unsigned int rate)
+{
+	/* no associated substream */
+	if (!subs)
+		return true;
+
+	/* data_endpoint not yet initialized */
+	if (!subs->data_endpoint)
+		return true;
+
+	if (subs->data_endpoint->use_count == 0)
+		return true;
+
+	if (subs->pcm_format != pcm_format ||
+	    subs->channels != channels ||
+	    subs->period_bytes != period_bytes ||
+	    subs->cur_rate != rate)
+		return false;
+
+	snd_printdd(KERN_INFO "ep #%x in use, parameters match\n",
+		    subs->data_endpoint->ep_num);
+	return true;
+}
+
 /**
  * snd_usb_endpoint_set_params: configure an snd_usb_endpoint
  *
@@ -752,6 +780,7 @@ out_of_memory:
  * @rate: the frame rate.
  * @fmt: the USB audio format information
  * @sync_ep: the sync endpoint to use, if any
+ * @subs: the substream that uses this endpoint as data endpoint
  *
  * Determine the number of URBs to be used on this endpoint.
  * An endpoint must be configured before it can be started.
@@ -763,14 +792,23 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 				unsigned int period_bytes,
 				unsigned int rate,
 				struct audioformat *fmt,
-				struct snd_usb_endpoint *sync_ep)
+				struct snd_usb_endpoint *sync_ep,
+				struct snd_usb_substream *subs)
 {
 	int err;
 
 	if (ep->param_set || ep->use_count != 0) {
-		snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n",
-			   ep->ep_num);
-		return -EBUSY;
+		if (!subs ||
+		    !snd_usb_endpoint_may_set_params(subs, pcm_format,
+						     channels, period_bytes,
+						     rate)) {
+			snd_printk(KERN_WARNING
+				   "Unable to change format on ep #%x: already in use\n",
+				   ep->ep_num);
+			return -EBUSY;
+		}
+		/* Endpoint already set with the same parameters */
+		return 0;
 	}
 	ep->param_set = true;
 
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index c928e0c..64f2b08 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -14,7 +14,8 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 				unsigned int period_bytes,
 				unsigned int rate,
 				struct audioformat *fmt,
-				struct snd_usb_endpoint *sync_ep);
+				struct snd_usb_endpoint *sync_ep,
+				struct snd_usb_substream *subs);
 
 int  snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep);
 void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep);
@@ -30,4 +31,10 @@ void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
 			     struct snd_usb_endpoint *sender,
 			     const struct urb *urb);
 
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs,
+				     snd_pcm_format_t pcm_format,
+				     unsigned int channels,
+				     unsigned int period_bytes,
+				     unsigned int rate);
+
 #endif /* __USBAUDIO_ENDPOINT_H */
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index a1332d0..d58fbcf 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -645,6 +645,7 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
 						   subs->period_bytes,
 						   subs->cur_rate,
 						   subs->cur_audiofmt,
+						   NULL,
 						   NULL);
 
 	/* Try to find the best matching audioformat. */
@@ -681,9 +682,18 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs)
 					  sync_period_bytes,
 					  subs->cur_rate,
 					  sync_fp,
-					  NULL);
+					  NULL,
+					  sync_subs);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	sync_subs->pcm_format = subs->pcm_format;
+	sync_subs->period_bytes = sync_period_bytes;
+	sync_subs->channels = sync_fp->channels;
+	sync_subs->cur_rate = subs->cur_rate;
+	sync_subs->data_endpoint = subs->sync_endpoint;
+
+	return 0;
 }
 
 /*
@@ -703,7 +713,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 					  subs->period_bytes,
 					  subs->cur_rate,
 					  subs->cur_audiofmt,
-					  subs->sync_endpoint);
+					  subs->sync_endpoint,
+					  subs);
 	if (ret < 0)
 		return ret;
 
@@ -729,16 +740,32 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	struct snd_usb_substream *subs = substream->runtime->private_data;
 	struct audioformat *fmt;
 	int ret;
+	snd_pcm_format_t pcm_format;
+	unsigned int channels;
+	unsigned int period_bytes;
+	unsigned int rate;
 
 	ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
 					       params_buffer_bytes(hw_params));
 	if (ret < 0)
 		return ret;
 
-	subs->pcm_format = params_format(hw_params);
-	subs->period_bytes = params_period_bytes(hw_params);
-	subs->channels = params_channels(hw_params);
-	subs->cur_rate = params_rate(hw_params);
+	pcm_format = params_format(hw_params);
+	period_bytes = params_period_bytes(hw_params);
+	channels = params_channels(hw_params);
+	rate = params_rate(hw_params);
+
+	if (!snd_usb_endpoint_may_set_params(subs, pcm_format, channels,
+					     period_bytes, rate)) {
+		snd_printk(KERN_WARNING "Unable to set hw_params on substream (dir: %d), data EP in use\n",
+			   subs->direction);
+		return -EBUSY;
+	}
+
+	subs->pcm_format = pcm_format;
+	subs->period_bytes = period_bytes;
+	subs->channels = channels;
+	subs->cur_rate = rate;
 
 	fmt = find_format(subs);
 	if (!fmt) {
@@ -772,9 +799,13 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
 {
 	struct snd_usb_substream *subs = substream->runtime->private_data;
 
-	subs->cur_audiofmt = NULL;
-	subs->cur_rate = 0;
-	subs->period_bytes = 0;
+	if (!subs->data_endpoint || subs->data_endpoint->use_count == 0) {
+		subs->cur_audiofmt = NULL;
+		subs->cur_rate = 0;
+		subs->period_bytes = 0;
+		subs->channels = 0;
+	}
+
 	down_read(&subs->stream->chip->shutdown_rwsem);
 	if (!subs->stream->chip->shutdown) {
 		stop_endpoints(subs, true);
-- 
1.8.1.5

[PATCH v3 10/11] ALSA: usb-audio: remove altset_idx from snd_usb_substream

[Eldad Zack]

We only need to check the current altsetting in one place, so there's no
benefit from tracking it.
This patch replaces that one check with get_cur_altset_idx() added in
a previous patch and in the proc substream dump routine.

Thanks to Clemens Ladisch for pointing out the difference between
altset_idx and altsetting - the check is changed accordingly to match
against fmt->altsetting.

Note that the proc dump now returns the altsetting number and not the
array index.

This allows removing altset_idx from snd_usb_substream.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

---
v2: corrected check.
---
 sound/usb/card.h | 1 -
 sound/usb/pcm.c  | 8 ++------
 sound/usb/pcm.h  | 2 ++
 sound/usb/proc.c | 4 +++-
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 4061ee1..0fd3f8b 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -117,7 +117,6 @@ struct snd_usb_substream {
 	unsigned int channels_max;	/* max channels in the all audiofmts */
 	unsigned int cur_rate;		/* current rate (for hw_params callback) */
 	unsigned int period_bytes;	/* current period bytes (for hw_params callback) */
-	unsigned int altset_idx;     /* USB data format: index of alternate setting */
 	unsigned int txfr_quirk:1;	/* allow sub-frame alignment */
 	unsigned int fmt_type;		/* USB audio format type (1-3) */
 	unsigned int pkt_offset_adj;	/* Bytes to drop from beginning of packets (for non-compliant devices) */
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index d58fbcf..5018b30 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -216,7 +216,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 	}
 }
 
-static int get_cur_altsetting(struct usb_device *dev, int ifnum)
+int get_cur_altsetting(struct usb_device *dev, int ifnum)
 {
 	struct usb_interface *iface;
 	struct usb_interface_descriptor *altsd;
@@ -538,12 +538,11 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 				dev->devnum, fmt->iface, fmt->altsetting, err);
 			return -EIO;
 		}
-		subs->altset_idx = 0;
 	}
 
 	/* set interface */
 	if (subs->interface != fmt->iface ||
-	    subs->altset_idx != fmt->altset_idx) {
+	    get_cur_altsetting(subs->dev, subs->interface) != fmt->altsetting) {
 		err = snd_usb_set_interface(subs, fmt->iface, fmt->altsetting);
 		if (err < 0) {
 			snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
@@ -552,7 +551,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 		}
 		snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
 				fmt->iface, fmt->altsetting);
-		subs->altset_idx = fmt->altset_idx;
 
 		snd_usb_set_interface_quirk(dev);
 	}
@@ -784,7 +782,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 
 	subs->interface = fmt->iface;
-	subs->altset_idx = fmt->altset_idx;
 	subs->need_setup_ep = true;
 
 	return 0;
@@ -1268,7 +1265,6 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
 	struct snd_usb_substream *subs = &as->substream[direction];
 
 	subs->interface = -1;
-	subs->altset_idx = 0;
 	runtime->hw = snd_usb_hardware;
 	runtime->private_data = subs;
 	subs->pcm_substream = substream;
diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h
index df7a003..0c9cb8c 100644
--- a/sound/usb/pcm.h
+++ b/sound/usb/pcm.h
@@ -10,5 +10,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 		       struct usb_host_interface *alts,
 		       struct audioformat *fmt);
 
+int get_cur_altsetting(struct usb_device *dev, int ifnum);
+
 
 #endif /* __USBAUDIO_PCM_H */
diff --git a/sound/usb/proc.c b/sound/usb/proc.c
index 5f761ab..ab6aafe 100644
--- a/sound/usb/proc.c
+++ b/sound/usb/proc.c
@@ -27,6 +27,7 @@
 #include "card.h"
 #include "endpoint.h"
 #include "proc.h"
+#include "pcm.h"
 
 /* convert our full speed USB rate into sampling rate in Hz */
 static inline unsigned get_full_speed_hz(unsigned int usb_rate)
@@ -140,7 +141,8 @@ static void proc_dump_substream_status(struct snd_usb_substream *subs, struct sn
 	if (subs->running) {
 		snd_iprintf(buffer, "  Status: Running\n");
 		snd_iprintf(buffer, "    Interface = %d\n", subs->interface);
-		snd_iprintf(buffer, "    Altset = %d\n", subs->altset_idx);
+		snd_iprintf(buffer, "    Altset = %d\n",
+			    get_cur_altsetting(subs->dev, subs->interface));
 		proc_dump_ep_status(subs, subs->data_endpoint, subs->sync_endpoint, buffer);
 	} else {
 		snd_iprintf(buffer, "  Status: Stop\n");
-- 
1.8.1.5

[PATCH v3 11/11] ALSA: usb-audio: rename alt_idx to altsetting

[Eldad Zack]

As Clemens Ladisch kindly explained:
 "Please note that there are two methods to identify alternate settings:
  the number, which is the value in bAlternateSetting, and the index,
  which is the index in the descriptor array.  There might be some wording
  in the USB spec that these two values must be the same, but in reality,
  [insert standard rant about firmware writers], bAlternateSetting
  must be treated as a random ID value."

This patch changes the name to express the correct usage semantics.
No functional change.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/card.h     | 2 +-
 sound/usb/endpoint.c | 6 +++---
 sound/usb/pcm.c      | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 0fd3f8b..ce204d8 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -96,7 +96,7 @@ struct snd_usb_endpoint {
 	unsigned int syncinterval;	/* P for adaptive mode, 0 otherwise */
 	unsigned char silence_value;
 	unsigned int stride;
-	int iface, alt_idx;
+	int iface, altsetting;
 	int skip_packets;		/* quirks for devices to ignore the first n packets
 					   in a stream */
 
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 7e04ebb..86aece0 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -426,9 +426,9 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 	list_for_each_entry(ep, &chip->ep_list, list) {
 		if (ep->ep_num == ep_num &&
 		    ep->iface == alts->desc.bInterfaceNumber &&
-		    ep->alt_idx == alts->desc.bAlternateSetting) {
+		    ep->altsetting == alts->desc.bAlternateSetting) {
 			snd_printdd(KERN_DEBUG "Re-using EP %x in iface %d,%d @%p\n",
-					ep_num, ep->iface, ep->alt_idx, ep);
+					ep_num, ep->iface, ep->altsetting, ep);
 			goto __exit_unlock;
 		}
 	}
@@ -447,7 +447,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 	ep->type = type;
 	ep->ep_num = ep_num;
 	ep->iface = alts->desc.bInterfaceNumber;
-	ep->alt_idx = alts->desc.bAlternateSetting;
+	ep->altsetting = alts->desc.bAlternateSetting;
 	INIT_LIST_HEAD(&ep->ready_playback_urbs);
 	ep_num &= USB_ENDPOINT_NUMBER_MASK;
 
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 5018b30..283e6c0 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -306,17 +306,17 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
 		struct snd_usb_endpoint *ep = subs->sync_endpoint;
 
 		if (subs->data_endpoint->iface != subs->sync_endpoint->iface ||
-		    subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) {
+		    subs->data_endpoint->altsetting != subs->sync_endpoint->altsetting) {
 			err = snd_usb_set_interface(subs,
 						    subs->sync_endpoint->iface,
-						    subs->sync_endpoint->alt_idx);
+						    subs->sync_endpoint->altsetting);
 			if (err < 0) {
 				clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
 				snd_printk(KERN_ERR
 					   "%d:%d:%d: cannot set interface (%d)\n",
 					   subs->dev->devnum,
 					   subs->sync_endpoint->iface,
-					   subs->sync_endpoint->alt_idx, err);
+					   subs->sync_endpoint->altsetting, err);
 				return -EIO;
 			}
 		}
-- 
1.8.1.5

Re: [PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage

[Eldad Zack]

On Sun, 25 Aug 2013, Eldad Zack wrote:

> Hi,
> 
> This patch series attempts to fix a long standing problem with the concurrent
> usage of playback and capture on implicit feedback devices.

My crude regression tester caught a problem causing the streams to 
fail with this order (though it doesn't trip any WARNs or oops):

++ Test stream permutation 0000013f
  * [1/14] capture => pcm_open
  * [2/14] capture => pcm_hw_params
  * [3/14] capture => pcm_prepare
  * [4/14] capture => pcm_start
  * [5/14] capture => waitsleep
  * [6/14] capture => pcm_stop
  ++ Frames [capture]: 39360
  * [7/14] playback => pcm_open
  * [8/14] playback => pcm_hw_params
  * [9/14] capture => pcm_close
  * [10/14] playback => pcm_prepare
  * [11/14] playback => pcm_start
  * [12/14] playback => waitsleep
  * [13/14] playback => pcm_stop
  ++ Frames [playback]: 101760
  * [14/14] playback => pcm_close
++ Test stream permutation 0000015f
  * [1/14] capture => pcm_open
  * [2/14] capture => pcm_hw_params
  * [3/14] capture => pcm_prepare
  * [4/14] capture => pcm_start
  * [5/14] capture => waitsleep
  * [6/14] playback => pcm_open
  * [7/14] capture => pcm_stop
  ++ Frames [capture]: 0
!! Test stream permutation 0000015f failed !!

For reference, I ran the tool with my hda-intel card (all 3432 
combinations) and there were no errors.
So it's still doesn't cover all cases.

Cheers,
Eldad

Re: [PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage

[Nikolay Martynov]

Hi.

  I've tried this patch and can confirm that it fixes the issue with CM108
based USB headset I've described on the list several days ago (tried with
Skype).
  Thank you!


2013/8/25 Eldad Zack <eldad@fogrefinery.com>

> Hi,
>
> This patch series attempts to fix a long standing problem with the
> concurrent
> usage of playback and capture on implicit feedback devices.
>
> Applies against current for-next (HEAD
> 68538bf2bce557c3b5fe8c59b034d45352500db1 ).
>
> v2:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html
> v1:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html
>
>  * Changed "altset_idx" to "altsetting" thanks to Clemens' clarification.
>
>  #3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace
> calling
>      snd_pcm_open() and then snd_pcm_close() immediately.
>  #8: subs->altset_idx was left intact, since it corresponds to the array
> index.
>  #10: this patch now removed subs->altset_idx, but also changes the check
> to be
>       performed against fmt->altsetting.
>  #11: New. This changes the name of snd_usb_endpoint member alt_idx to
> altsetting
>       for the same reason Clemens described.
>
> Clemens and Daniel - thank you very much for your help!
>
> Cheers,
> Eldad
>
> Eldad Zack (11):
>   ALSA: usb-audio: remove unused parameter from sync_ep_set_params
>   ALSA: usb-audio: remove deactivate_endpoints()
>   ALSA: usb-audio: prevent NULL dereference on stop trigger
>   ALSA: usb-audio: don't deactivate URBs on in-use EP
>   ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate()
>   ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error
>   ALSA: usb-audio: correct ep use_count semantics (add set_param flag)
>   ALSA: usb-audio: conditional interface altsetting
>   ALSA: usb-audio: conditional concurrent usage of endpoint
>   ALSA: usb-audio: remove altset_idx from snd_usb_substream
>   ALSA: usb-audio: rename alt_idx to altsetting
>
>  sound/usb/card.h     |   4 +-
>  sound/usb/endpoint.c |  82 ++++++++++++++++++-------
>  sound/usb/endpoint.h |  11 +++-
>  sound/usb/pcm.c      | 168
> ++++++++++++++++++++++++++++++++++++++-------------
>  sound/usb/pcm.h      |   2 +
>  sound/usb/proc.c     |   4 +-
>  6 files changed, 201 insertions(+), 70 deletions(-)
>
> --
> 1.8.1.5
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
Martynov Nikolay.
Email: mar.kolya@gmail.com

Re: [PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage

[Eldad Zack]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3193 bytes --]


On Mon, 26 Aug 2013, Nikolay Martynov wrote:

> Hi.
> 
>   I've tried this patch and can confirm that it fixes the issue with CM108 based USB headset I've described on the list several days ago (tried with Skype).
>   Thank you!

That's great - I'm glad it solved your issue and thank you for testing!

There's still some issues I bump into at the moment, hopefully I will 
find the time to resolve them before the 3.13 window.

Cheers,
Eldad

> 
> 
> 2013/8/25 Eldad Zack <eldad@fogrefinery.com>
>       Hi,
> 
>       This patch series attempts to fix a long standing problem with the concurrent
>       usage of playback and capture on implicit feedback devices.
> 
>       Applies against current for-next (HEAD 68538bf2bce557c3b5fe8c59b034d45352500db1 ).
> 
>       v2:
>        http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html
>       v1:
>        http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html
> 
>        * Changed "altset_idx" to "altsetting" thanks to Clemens' clarification.
> 
>        #3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace calling
>            snd_pcm_open() and then snd_pcm_close() immediately.
>        #8: subs->altset_idx was left intact, since it corresponds to the array index.
>        #10: this patch now removed subs->altset_idx, but also changes the check to be
>             performed against fmt->altsetting.
>        #11: New. This changes the name of snd_usb_endpoint member alt_idx to altsetting
>             for the same reason Clemens described.
> 
>       Clemens and Daniel - thank you very much for your help!
> 
>       Cheers,
>       Eldad
> 
>       Eldad Zack (11):
>         ALSA: usb-audio: remove unused parameter from sync_ep_set_params
>         ALSA: usb-audio: remove deactivate_endpoints()
>         ALSA: usb-audio: prevent NULL dereference on stop trigger
>         ALSA: usb-audio: don't deactivate URBs on in-use EP
>         ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate()
>         ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error
>         ALSA: usb-audio: correct ep use_count semantics (add set_param flag)
>         ALSA: usb-audio: conditional interface altsetting
>         ALSA: usb-audio: conditional concurrent usage of endpoint
>         ALSA: usb-audio: remove altset_idx from snd_usb_substream
>         ALSA: usb-audio: rename alt_idx to altsetting
> 
>        sound/usb/card.h     |   4 +-
>        sound/usb/endpoint.c |  82 ++++++++++++++++++-------
>        sound/usb/endpoint.h |  11 +++-
>        sound/usb/pcm.c      | 168 ++++++++++++++++++++++++++++++++++++++-------------
>        sound/usb/pcm.h      |   2 +
>        sound/usb/proc.c     |   4 +-
>        6 files changed, 201 insertions(+), 70 deletions(-)
> 
>       --
>       1.8.1.5
> 
>       _______________________________________________
>       Alsa-devel mailing list
>       Alsa-devel@alsa-project.org
>       http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> 
> 
> 
> --
> Martynov Nikolay.
> Email: mar.kolya@gmail.com
> 
> 

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