[PATCH RFC 0/9] ALSA: usb-audio: fix playback/capture concurrent usage

[PATCH RFC 0/9] ALSA: usb-audio: fix playback/capture concurrent usage

[Eldad Zack]

Hi,

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

As Clemens found out, when implicit feedback is used, playback and capture
cannot be used in the same time - unless started at the same time (e.g., with
jack).
While a substream is active, attempting to start a substream in the opposite
direction will fail and also (on some devices?) cause the currently active stream
to stop.

There are multiple issues that contribute to this:

 1. URBs are unconditionally deactivated in hw_free. On implicit feedback devices,
    the sync_endpoint (= data_endpoint of the capture) will be deactivated here.
    Fixed in patch #4.

 2. Changing the alternate settings of an interface might break currently running
    streams. This happens on my device, but I suspect other devices too.
    Fixed in patch #7.

 3. The endpoint use_count has a subtle issue (see patch #9). This causes jack
    to work fine, while using two separate programs (most probably) fails.
    In short: set_format is called from hw_params, but use_count is incremented
    only later (prepare or trigger).

 4. Concurrent usage should be allowed but only if the hw_params match up (patch #8)

One problem with this series is that I query the device for the altsettting.
This might not be reliable for all devices, so do I plan to remove this.
Of course, I've only tested this on my device, but it seems to work good and
I couldn't break it so far, so hopefully I got this right.

Can someone give it a try on other devices (also on non-implicit-feedback)?

Cheers,
Eldad


Eldad Zack (9):
  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: conditional interface altsetting
  ALSA: usb-audio: conditional concurrent usage of endpoint
  ALSA: usb-audio: correct ep use_count semantics (add set_param flag)

 sound/usb/card.h     |   1 +
 sound/usb/endpoint.c |  65 +++++++++++++++------
 sound/usb/endpoint.h |  11 +++-
 sound/usb/pcm.c      | 155 +++++++++++++++++++++++++++++++++++++++------------
 4 files changed, 176 insertions(+), 56 deletions(-)

-- 
1.8.1.5

[PATCH RFC 1/9] 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 659950e..13e3685 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -700,8 +700,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;
 
@@ -793,7 +792,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 RFC 2/9] 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 15b151e..f612c4e 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,
@@ -697,7 +681,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 RFC 3/9] 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.

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

---
Note sure if the oops happens because of pcm_stream is NULLified, or
somewhere else. Needs a closer look.

I suspect this condition does not happen in practice since the call to
snd_usb_endpoint_deactivate() will unconditionally deactivate the URBs.
---
 sound/usb/pcm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index f612c4e..aa1e21f 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1166,6 +1166,8 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
 		subs->interface = -1;
 	}
 
+	subs->data_endpoint->prepare_data_urb = NULL;
+	subs->data_endpoint->retire_data_urb = NULL;
 	subs->pcm_substream = NULL;
 	snd_usb_autosuspend(subs->stream->chip);
 
@@ -1492,6 +1494,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;
@@ -1522,6 +1526,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 RFC 4/9] 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 13e3685..00d8418 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -940,12 +940,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 RFC 5/9] 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 00d8418..28ddc2b 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -927,28 +927,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 RFC 6/9] 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 aa1e21f..b610231 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 RFC 7/9] 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.

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

---
This patch needs revising. It adds a get_interface function, which
should not be neccesary (possibly unreliable on some devices?).
---
 sound/usb/pcm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b610231..ff74e89 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -216,6 +216,69 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 	}
 }
 
+static int get_interface(struct usb_device *dev, int ifnum)
+{
+	int ret;
+	char buf;
+	struct usb_interface *iface = usb_ifnum_to_if(dev, ifnum);
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+			      USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
+			      0, iface->altsetting[0].desc.bInterfaceNumber,
+			      &buf, sizeof(buf), USB_CTRL_GET_TIMEOUT);
+
+	if (ret == sizeof(buf))
+		return buf;
+
+	return -ERANGE;
+}
+
+static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
+			      int altset_idx)
+{
+	struct snd_usb_substream *other_subs =
+		&subs->stream->substream[subs->direction ^ 1];
+	int err;
+
+	snd_printdd(KERN_DEBUG "%s: set interface ifnum %d altset_idx %d\n", __func__, ifnum, altset_idx);
+
+	if (subs == NULL)
+		return usb_set_interface(subs->dev, ifnum, altset_idx);
+
+	if (other_subs->data_endpoint && other_subs->data_endpoint->use_count != 0) {
+		int cur_altset_idx = get_interface(subs->dev, ifnum);
+
+		snd_printdd(KERN_DEBUG "%s: set interface (cur %d) ifnum %d altset_idx %d\n", __func__,
+			cur_altset_idx, ifnum, altset_idx);
+
+		if (cur_altset_idx == altset_idx)
+			return 0;
+
+		if (cur_altset_idx > 0 && altset_idx == 0)
+			return 0;
+	}
+
+	err = usb_set_interface(subs->dev, ifnum, altset_idx);
+	snd_printdd(KERN_DEBUG "%s: set interface ifnum %d altset_idx %d err %d\n", __func__, ifnum, altset_idx, err);
+	if (err < 0)
+		return err;
+
+	subs->interface = altset_idx == 0 ? -1 : ifnum;
+	subs->altset_idx = altset_idx;
+
+	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 +305,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
@@ -338,20 +401,18 @@ 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);
@@ -359,8 +420,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);
 	}
@@ -1163,7 +1222,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 RFC 8/9] 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 | 36 ++++++++++++++++++++++++++++++++++--
 sound/usb/endpoint.h |  9 ++++++++-
 sound/usb/pcm.c      | 51 +++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 83 insertions(+), 13 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 28ddc2b..4241154 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -737,6 +737,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
  *
@@ -747,6 +775,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.
@@ -758,11 +787,14 @@ 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->use_count != 0) {
+	if ((!subs && ep->use_count != 0) ||
+	    !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;
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 ff74e89..5547e70 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -608,6 +608,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. */
@@ -644,9 +645,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;
 }
 
 /*
@@ -666,7 +676,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;
 
@@ -692,16 +703,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) {
@@ -735,9 +762,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 RFC 9/9] 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>
---
 sound/usb/card.h     |  1 +
 sound/usb/endpoint.c | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

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 4241154..a56e769 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -469,6 +469,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:
@@ -792,13 +794,15 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 {
 	int err;
 
-	if ((!subs && ep->use_count != 0) ||
-	    !snd_usb_endpoint_may_set_params(subs, pcm_format, channels,
-					     period_bytes, rate)) {
+	if (ep->param_set &&
+	   ((!subs && ep->use_count != 0) ||
+	     !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;
 	}
+	ep->param_set = true;
 
 	/* release old buffers, if any */
 	release_urbs(ep, 0);
@@ -950,6 +954,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

Re: [PATCH RFC 7/9] ALSA: usb-audio: conditional interface altsetting

[Clemens Ladisch]

Eldad Zack wrote:
> On some devices, if the endpoint for the other direction is in use,
> setting one interface will shutdown the other (in use) endpoint.

There are (non-UAC) devices where both endpoints are in the same
interface.

> This patch needs revising. It adds a get_interface function, which
> should not be neccesary (possibly unreliable on some devices?).

struct usb_interface has cur_altsetting.


Regards,
Clemens

Re: [PATCH RFC 7/9] ALSA: usb-audio: conditional interface altsetting

[Eldad Zack]

Hi Clemens,

On Mon, 19 Aug 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > On some devices, if the endpoint for the other direction is in use,
> > setting one interface will shutdown the other (in use) endpoint.
> 
> There are (non-UAC) devices where both endpoints are in the same
> interface.

That's good to know. But doesn't setting the alternate the interface on
behalf of one ep while the other ep is in use distrub anything too?
Or did you mean something else?

> 
> > This patch needs revising. It adds a get_interface function, which
> > should not be neccesary (possibly unreliable on some devices?).
> 
> struct usb_interface has cur_altsetting.

Ah, didn't look there. Thanks!

Hmm, so is there a reason why is it also kept in snd_usb_substream 
(altset_idx)?
>From a quick check I see that it is only used in set_format, and using 
iface->cur_altsetting would be the same, so it might be redundant. I'll 
look into that next.

Cheers,
Eldad