[PATCH v2 00/10] ALSA: usb-audio: fix playback/capture concurrent usage

[PATCH v2 00/10] 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.

Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).

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

Thanks to Clemens I was able to get rid of the get_interface workaround, so I
believe it's ready now, unless there's any other issues.

Other changes:
* Cleaned up and moved the set_param flag patch to #7.
* Fixed a NULL dereference in patch #8 for explicit feedback.
* Fixed a logic error in patch #9.
* Added patch #10.

Cheers,
Eldad

Eldad Zack (10):
  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

 sound/usb/card.h     |   2 +-
 sound/usb/endpoint.c |  76 +++++++++++++++++-------
 sound/usb/endpoint.h |  11 +++-
 sound/usb/pcm.c      | 161 ++++++++++++++++++++++++++++++++++++++-------------
 sound/usb/pcm.h      |   2 +
 sound/usb/proc.c     |   4 +-
 6 files changed, 192 insertions(+), 64 deletions(-)

-- 
1.8.1.5

[PATCH v2 01/10] 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 v2 02/10] 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 v2 03/10] 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>
---
 sound/usb/pcm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 9ec401a..19b0eb4 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1199,6 +1199,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);
 
@@ -1525,6 +1527,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 +1559,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 v2 04/10] 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 v2 05/10] 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 v2 06/10] 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 19b0eb4..bdc3325 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 v2 07/10] 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 v2 08/10] ALSA: usb-audio: conditional interface altsetting

[Eldad Zack]

On some devices, if the endpoint for the other direction is in use,
setting the interface altsetting 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.

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.
---
 sound/usb/pcm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 10 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index bdc3325..140e2e4 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -216,6 +216,72 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 	}
 }
 
+static int get_cur_altset_idx(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 altset_idx)
+{
+	struct snd_usb_substream *other_subs;
+	int err;
+
+	snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n",
+		    __func__, ifnum, altset_idx);
+
+	if (subs == NULL)
+		return usb_set_interface(subs->dev, ifnum, altset_idx);
+	other_subs = &(subs->stream->substream[subs->direction ^ 1]);
+
+	if (other_subs->data_endpoint &&
+	    other_subs->data_endpoint->use_count != 0) {
+		int cur_altset_idx = get_cur_altset_idx(subs->dev, ifnum);
+
+		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: Interface %d alts set to %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 +308,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 +533,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);
@@ -488,8 +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 +1258,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

Re: [PATCH v2 08/10] ALSA: usb-audio: conditional interface altsetting

[Clemens Ladisch]

Eldad Zack wrote:
> This patch moves all of the alternate setting operations for pcm
> ops to one function which checks if we can do so.

> +static int get_cur_altset_idx(struct usb_device *dev, int ifnum)
> +{
> +	...
> +	return altsd->bAlternateSetting;

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], and bAlternateSetting
must be treated as a random ID value.

In struct audioformat, both values are stored in the altsetting and
altset_idx fields; struct usb_substream has only altset_idx.

As far as I can see, you code always uses the name altset_idx for the
number.


Regards,
Clemens

Re: [PATCH v2 08/10] ALSA: usb-audio: conditional interface altsetting

[Eldad Zack]

On Thu, 22 Aug 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > This patch moves all of the alternate setting operations for pcm
> > ops to one function which checks if we can do so.
> 
> > +static int get_cur_altset_idx(struct usb_device *dev, int ifnum)
> > +{
> > +	...
> > +	return altsd->bAlternateSetting;
> 
> 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], and bAlternateSetting

Yes, the size of mixer_quirks says it all...

> must be treated as a random ID value.
> 
> In struct audioformat, both values are stored in the altsetting and
> altset_idx fields; struct usb_substream has only altset_idx.
> 
> As far as I can see, you code always uses the name altset_idx for the
> number.

You are right of course. I meant the ID - I'll change the name to
"altsetting". And I'll have to change patch #10 as well, because it 
actually wrong there.

Thanks again, Clemens!

Cheers,
Eldad

[PATCH v2 09/10] 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>
---
v2: Fixed logic error in the concurrent usage check
(snd_usb_endpoint_set_params)

---
 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 140e2e4..a72f755 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -644,6 +644,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. */
@@ -680,9 +681,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;
 }
 
 /*
@@ -702,7 +712,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;
 
@@ -728,16 +739,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) {
@@ -771,9 +798,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 v2 10/10] 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.
This allows removing altset_idx from snd_usb_substream.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/card.h | 1 -
 sound/usb/pcm.c  | 7 ++-----
 sound/usb/pcm.h  | 2 ++
 sound/usb/proc.c | 4 +++-
 4 files changed, 7 insertions(+), 7 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 a72f755..8cdc064 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_altset_idx(struct usb_device *dev, int ifnum)
+int get_cur_altset_idx(struct usb_device *dev, int ifnum)
 {
 	struct usb_interface *iface;
 	struct usb_interface_descriptor *altsd;
@@ -266,7 +266,6 @@ static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
 		return err;
 
 	subs->interface = altset_idx == 0 ? -1 : ifnum;
-	subs->altset_idx = altset_idx;
 
 	return 0;
 }
@@ -543,7 +542,7 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 
 	/* set interface */
 	if (subs->interface != fmt->iface ||
-	    subs->altset_idx != fmt->altset_idx) {
+	    get_cur_altset_idx(subs->dev, subs->interface) != fmt->altset_idx) {
 		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",
@@ -783,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;
@@ -1267,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..3270e11 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_altset_idx(struct usb_device *dev, int ifnum);
+
 
 #endif /* __USBAUDIO_PCM_H */
diff --git a/sound/usb/proc.c b/sound/usb/proc.c
index 5f761ab..44080dd 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_altset_idx(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

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

[Daniel Mack]

On 21.08.2013 23:37, Eldad Zack wrote:
> Hi,
> 
> This patch series attempts to fix a long starting problem with the concurrent
> usage of playback and capture on implicit feedback devices.
> 
> Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).

Thanks a lot for working on this! I'll give that series a test run tonight.


Daniel

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

[Eldad Zack]

On Thu, 22 Aug 2013, Daniel Mack wrote:

> On 21.08.2013 23:37, Eldad Zack wrote:
> > Hi,
> > 
> > This patch series attempts to fix a long starting problem with the concurrent
> > usage of playback and capture on implicit feedback devices.
> > 
> > Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).
> 
> Thanks a lot for working on this! I'll give that series a test run tonight.

That's great, thanks!

Cheers,
Eldad

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

[Daniel Mack]

On 22.08.2013 19:43, Eldad Zack wrote:
> On Thu, 22 Aug 2013, Daniel Mack wrote:
>> On 21.08.2013 23:37, Eldad Zack wrote:
>>> Hi,
>>>
>>> This patch series attempts to fix a long starting problem with the concurrent
>>> usage of playback and capture on implicit feedback devices.
>>>
>>> Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).
>>
>> Thanks a lot for working on this! I'll give that series a test run tonight.
> 
> That's great, thanks!

So, with your patches applied, I'm getting the NULL pointer Oops below
just by plugging in a UAC2 sound card.

Note that on this system, pulseaudio is running and grabs the new card
immediately, then closes it again. That might be different on your machine.

Do you want me to dig deeper or do you have an idea already?


Best regards,
Daniel


(sorry for the broken formatting):

Aug 23 03:32:46 glossy kernel: [  511.728348] BUG: unable to handle
kernel NULL pointer dereference at 00000018
Aug 23 03:32:46 glossy kernel: [  511.728559] IP: [<f90631a2>]
snd_usb_pcm_close.isra.13+0x42/0x70 [snd_usb_audio]
Aug 23 03:32:46 glossy kernel: [  511.728773] *pde = 00000000
Aug 23 03:32:46 glossy kernel: [  511.728859] Oops: 0002 [#1] SMP
Aug 23 03:32:46 glossy kernel: [  511.728959] Modules linked in:
snd_usb_audio snd_usbmidi_lib arc4 brcmsmac mac80211 bcma brcmutil
cfg80211 cordic rfcomm bnep parport_pc ppdev snd_hda_codec_realtek
snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi
snd_seq_midi_event snd_seq snd_timer joydev snd_seq_device btusb
bluetooth snd i915 soundcore snd_page_alloc samsung_laptop psmouse
mac_hid serio_raw drm_kms_helper drm i2c_algo_bit video binfmt_misc lp
parport sky2
Aug 23 03:32:46 glossy kernel: [  511.730307] CPU: 1 PID: 1673 Comm:
pulseaudio Tainted: G        W    3.11.0-rc5-custom+ #22
Aug 23 03:32:46 glossy kernel: [  511.730510] Hardware name: SAMSUNG
ELECTRONICS CO., LTD. N150P/N210P/N220P          /N150P/N210P/N220P
     , BIOS 01KY.M008.20100430.RHU 04/30/2010
Aug 23 03:32:46 glossy kernel: [  511.730844] task: f50772c0 ti:
f4d92000 task.ti: f4d92000
Aug 23 03:32:46 glossy kernel: [  511.730980] EIP: 0060:[<f90631a2>]
EFLAGS: 00010286 CPU: 1
Aug 23 03:32:46 glossy kernel: [  511.731134] EIP is at
snd_usb_pcm_close.isra.13+0x42/0x70 [snd_usb_audio]
Aug 23 03:32:46 glossy kernel: [  511.731307] EAX: 00000000 EBX:
f6b64e10 ECX: f9061fc0 EDX: ffffffff
Aug 23 03:32:46 glossy kernel: [  511.731461] ESI: f6b64e00 EDI:
d97a0540 EBP: f4d93f18 ESP: f4d93f10
Aug 23 03:32:46 glossy kernel: [  511.731619]  DS: 007b ES: 007b FS:
00d8 GS: 00e0 SS: 0068
Aug 23 03:32:46 glossy kernel: [  511.731760] CR0: 80050033 CR2:
00000018 CR3: 34fc4000 CR4: 000007d0
Aug 23 03:32:46 glossy kernel: [  511.731918] Stack:
Aug 23 03:32:46 glossy kernel: [  511.731977]  ecf13e00 ecf13e00
f4d93f20 f9063202 f4d93f30 f8426fcc f6b64800 ecf13e00
Aug 23 03:32:46 glossy kernel: [  511.732025]  f4d93f50 f842705b
00000000 d9623d50 f6b648e8 d97a0540 00000008 f4f1f7a4
Aug 23 03:32:46 glossy kernel: [  511.732025]  f4d93f80 c11463b6
00000001 00000000 00000000 d97a0548 f60c8d90 d43b3900
Aug 23 03:32:46 glossy kernel: [  511.732025] Call Trace:
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<f9063202>]
snd_usb_playback_close+0x12/0x20 [snd_usb_audio]
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<f8426fcc>]
snd_pcm_release_substream+0x5c/0xb0 [snd_pcm]
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<f842705b>]
snd_pcm_release+0x3b/0xa0 [snd_pcm]
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<c11463b6>]
__fput+0xc6/0x1f0
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<c114651d>]
____fput+0xd/0x10
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<c1059fa1>]
task_work_run+0x81/0xa0
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<c1002189>]
do_notify_resume+0x49/0x70
Aug 23 03:32:46 glossy kernel: [  511.732025]  [<c159b113>]
work_notifysig+0x24/0x29

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

[Eldad Zack]

On Thu, 22 Aug 2013, Daniel Mack wrote:

> On 22.08.2013 19:43, Eldad Zack wrote:
> > On Thu, 22 Aug 2013, Daniel Mack wrote:
> >> On 21.08.2013 23:37, Eldad Zack wrote:
> >>> Hi,
> >>>
> >>> This patch series attempts to fix a long starting problem with the concurrent
> >>> usage of playback and capture on implicit feedback devices.
> >>>
> >>> Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).
> >>
> >> Thanks a lot for working on this! I'll give that series a test run tonight.
> > 
> > That's great, thanks!
> 
> So, with your patches applied, I'm getting the NULL pointer Oops below
> just by plugging in a UAC2 sound card.
> 
> Note that on this system, pulseaudio is running and grabs the new card
> immediately, then closes it again. That might be different on your machine.
> 
> Do you want me to dig deeper or do you have an idea already?

Oh, I think that data_endpoint just doesn't get initialized yet, 
because it opens and closes immediately.

Does this fix it?

Cheers,
Eldad

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 02fb1ea..283e6c0 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1290,8 +1290,11 @@ 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;
+	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);
 

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

[Eldad Zack]

On Thu, 22 Aug 2013, Eldad Zack wrote:
> On Thu, 22 Aug 2013, Daniel Mack wrote:
> > On 22.08.2013 19:43, Eldad Zack wrote:
> > > On Thu, 22 Aug 2013, Daniel Mack wrote:
> > >> On 21.08.2013 23:37, Eldad Zack wrote:
> > >>> Hi,
> > >>>
> > >>> This patch series attempts to fix a long starting problem with the concurrent
> > >>> usage of playback and capture on implicit feedback devices.
> > >>>
> > >>> Applies against current for-next (HEAD b43dd416be21bc8ad60984e13def032f01aaaa18 ).
> > >>
> > >> Thanks a lot for working on this! I'll give that series a test run tonight.
> > > 
> > > That's great, thanks!
> > 
> > So, with your patches applied, I'm getting the NULL pointer Oops below
> > just by plugging in a UAC2 sound card.
> > 
> > Note that on this system, pulseaudio is running and grabs the new card
> > immediately, then closes it again. That might be different on your machine.
> > 
> > Do you want me to dig deeper or do you have an idea already?
> 
> Oh, I think that data_endpoint just doesn't get initialized yet, 
> because it opens and closes immediately.
> 
> Does this fix it?

OK, I could easily reproduce this, though I don't have pulseaudio 
installed. Luckily I started working on a regression tester, so I 
modified it to just open and close (it's trivial, but if anyone is 
interested, I can post it). 
This fixes it for me.

Sorry about the oops and thank you very much for testing!

I also need to fix an error in the code (the altset_idx/altsetting issue 
Clemens pointed out), so you might want to wait until I post a revised 
patchset.

Cheers,
Eldad