Skip to contentSkip to conversation

Refactor set_format()

RFC PATCH Superseded Eldad Zack 2 participants 13 messages Jul 19, 2013 – Jul 20, 2013 Sound

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Eldad Zack

author
#0 lore raw

cover Refactor set_format()

Hi,

This is a low-priority series (applies against current for-next). I had the idea of refactoring set_format to make it easier to read (I actually indended to look at the implicit feedback setup race, but stumbled on this again).

This series is not intended to be applied as-is (it has some whitespace issues and I wrote the commit messages quickly), but to get some feedback - if you think it's a good idea in general, and also specific feedback to individual patches. Assuming it makes sense, I'll fix the whitespace issues, commit messages and anything else before resubmitting.

Cheers,
Eldad

Eldad Zack (10):
ALSA: usb-audio: remove disabled debug code in set_format
ALSA: usb-audio: remove assignment from if condition
ALSA: usb-audio: separate sync endpoint setting from set_format
ALSA: usb-audio: move implicit fb quirks to separate function
ALSA: usb-audio: reverse condition logic in set_sync_endpoint
ALSA: usb-audio: do not initialize and check implicit_fb
ALSA: usb-audio: remove is_playback from implicit feedback quirks
ALSA: usb-audio: remove implicit_fb from quirk
ALSA: usb-audio: clarify implicit fb quirk for Roland
ALSA: usb-audio: check alts is not NULL before adding ep

 sound/usb/pcm.c | 249 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 144 insertions(+), 105 deletions(-)
Signature · 1 line
1.8.1.5
author
#1 lore raw

RFC PATCH 1/10 ALSA: usb-audio: remove disabled debug code in set_format

Code block does not compile when enabled.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 9 ---------
 1 file changed, 9 deletions(-)
sound/usb/pcm.c +0 -9
index 15b151e..3d3e8d1 100644
@@ -486,15 +486,6 @@ add_sync_ep:
 
 	snd_usb_set_format_quirk(subs, fmt);
 
-#if 0
-	printk(KERN_DEBUG
-	       "setting done: format = %d, rate = %d..%d, channels = %d\n",
-	       fmt->format, fmt->rate_min, fmt->rate_max, fmt->channels);
-	printk(KERN_DEBUG
-	       "  datapipe = 0x%0x, syncpipe = 0x%0x\n",
-	       subs->datapipe, subs->syncpipe);
-#endif
-
 	return 0;
 }
Signature · 1 line
1.8.1.5
author
#2 lore raw

RFC PATCH 2/10 ALSA: usb-audio: remove assignment from if condition

In reply to Eldad Zack

Following general kernel style.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
sound/usb/pcm.c +2 -1
index 3d3e8d1..be5c7c2 100644
@@ -479,7 +479,8 @@ add_sync_ep:
 		subs->data_endpoint->sync_master = subs->sync_endpoint;
 	}
 
-	if ((err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt)) < 0)
+	err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt);
+	if (err < 0)
 		return err;
 
 	subs->cur_audiofmt = fmt;
Signature · 1 line
1.8.1.5
author
#3 lore raw

RFC PATCH 3/10 ALSA: usb-audio: separate sync endpoint setting from set_format

In reply to Eldad Zack

Setting the sync endpoint currently takes up about half of set_format(). Move it to a dedicated function.
No functional change.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 128 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 55 deletions(-)
sound/usb/pcm.c +73 -55
index be5c7c2..f667b7f 100644
@@ -327,64 +327,17 @@ static int search_roland_implicit_fb(struct usb_device *dev, int ifnum,
 	return 0;
 }
 
-/*
- * find a matching format and set up the interface
- */
-static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
+
+static int set_sync_endpoint(struct snd_usb_substream *subs,
+			     struct audioformat *fmt,
+			     struct usb_host_interface *alts,
+			     struct usb_interface_descriptor *altsd)
 {
-	struct usb_device *dev = subs->dev;
-	struct usb_host_interface *alts;
-	struct usb_interface_descriptor *altsd;
 	struct usb_interface *iface;
-	unsigned int ep, attr;
+	struct usb_device *dev = subs->dev;
 	int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
-	int err, implicit_fb = 0;
-
-	iface = usb_ifnum_to_if(dev, fmt->iface);
-	if (WARN_ON(!iface))
-		return -EINVAL;
-	alts = &iface->altsetting[fmt->altset_idx];
-	altsd = get_iface_desc(alts);
-	if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting))
-		return -EINVAL;
-
-	if (fmt == subs->cur_audiofmt)
-		return 0;
-
-	/* close the old interface */
-	if (subs->interface >= 0 && subs->interface != fmt->iface) {
-		err = usb_set_interface(subs->dev, subs->interface, 0);
-		if (err < 0) {
-			snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n",
-				dev->devnum, fmt->iface, fmt->altsetting, err);
-			return -EIO;
-		}
-		subs->interface = -1;
-		subs->altset_idx = 0;
-	}
-
-	/* set interface */
-	if (subs->interface != fmt->iface ||
-	    subs->altset_idx != fmt->altset_idx) {
-		err = usb_set_interface(dev, fmt->iface, fmt->altsetting);
-		if (err < 0) {
-			snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
-				   dev->devnum, fmt->iface, fmt->altsetting, err);
-			return -EIO;
-		}
-		snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
-				fmt->iface, fmt->altsetting);
-		subs->interface = fmt->iface;
-		subs->altset_idx = fmt->altset_idx;
-
-		snd_usb_set_interface_quirk(dev);
-	}
-
-	subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
-						   alts, fmt->endpoint, subs->direction,
-						   SND_USB_ENDPOINT_TYPE_DATA);
-	if (!subs->data_endpoint)
-		return -EINVAL;
+	unsigned int ep, attr;
+	int implicit_fb = 0;
 
 	/* we need a sync pipe in async OUT or adaptive IN mode */
 	/* check the number of EP, since some devices have broken

@@ -479,6 +432,71 @@ add_sync_ep:
 		subs->data_endpoint->sync_master = subs->sync_endpoint;
 	}
 
+	return 0;
+}
+
+/*
+ * find a matching format and set up the interface
+ */
+static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
+{
+	struct usb_device *dev = subs->dev;
+	struct usb_host_interface *alts;
+	struct usb_interface_descriptor *altsd;
+	struct usb_interface *iface;
+	int err;
+
+	iface = usb_ifnum_to_if(dev, fmt->iface);
+	if (WARN_ON(!iface))
+		return -EINVAL;
+	alts = &iface->altsetting[fmt->altset_idx];
+	altsd = get_iface_desc(alts);
+	if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting))
+		return -EINVAL;
+
+	if (fmt == subs->cur_audiofmt)
+		return 0;
+
+	/* close the old interface */
+	if (subs->interface >= 0 && subs->interface != fmt->iface) {
+		err = usb_set_interface(subs->dev, subs->interface, 0);
+		if (err < 0) {
+			snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n",
+				dev->devnum, fmt->iface, fmt->altsetting, err);
+			return -EIO;
+		}
+		subs->interface = -1;
+		subs->altset_idx = 0;
+	}
+
+	/* set interface */
+	if (subs->interface != fmt->iface ||
+	    subs->altset_idx != fmt->altset_idx) {
+		err = usb_set_interface(dev, fmt->iface, fmt->altsetting);
+		if (err < 0) {
+			snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n",
+				   dev->devnum, fmt->iface, fmt->altsetting, err);
+			return -EIO;
+		}
+		snd_printdd(KERN_INFO "setting usb interface %d:%d\n",
+				fmt->iface, fmt->altsetting);
+		subs->interface = fmt->iface;
+		subs->altset_idx = fmt->altset_idx;
+
+		snd_usb_set_interface_quirk(dev);
+	}
+
+	subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+						   alts, fmt->endpoint, subs->direction,
+						   SND_USB_ENDPOINT_TYPE_DATA);
+
+	if (!subs->data_endpoint)
+		return -EINVAL;
+
+	err = set_sync_endpoint(subs, fmt, alts, altsd);
+	if (err < 0)
+		return err;
+
 	err = snd_usb_init_pitch(subs->stream->chip, fmt->iface, alts, fmt);
 	if (err < 0)
 		return err;
Signature · 1 line
1.8.1.5
author
#4 lore raw

RFC PATCH 4/10 ALSA: usb-audio: move implicit fb quirks to separate function

In reply to Eldad Zack

Separate implicit feedback quirks from setting a sync endpoint (which may also be explicit feedback or async).

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 71 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 17 deletions(-)
sound/usb/pcm.c +54 -17
index f667b7f..549964a 100644
@@ -327,24 +327,16 @@ static int search_roland_implicit_fb(struct usb_device *dev, int ifnum,
 	return 0;
 }
 
-
-static int set_sync_endpoint(struct snd_usb_substream *subs,
-			     struct audioformat *fmt,
-			     struct usb_host_interface *alts,
-			     struct usb_interface_descriptor *altsd)
+static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
+					 struct usb_device *dev,
+					 struct usb_interface_descriptor *altsd,
+					 unsigned int attr)
 {
+	struct usb_host_interface *alts;
 	struct usb_interface *iface;
-	struct usb_device *dev = subs->dev;
 	int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
-	unsigned int ep, attr;
 	int implicit_fb = 0;
-
-	/* we need a sync pipe in async OUT or adaptive IN mode */
-	/* check the number of EP, since some devices have broken
-	 * descriptors which fool us.  if it has only one EP,
-	 * assume it as adaptive-out or sync-in.
-	 */
-	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
+	unsigned int ep;
 
 	switch (subs->stream->chip->usb_id) {
 	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */

@@ -381,13 +373,59 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	    altsd->bInterfaceProtocol == 2 &&
 	    altsd->bNumEndpoints == 1 &&
 	    USB_ID_VENDOR(subs->stream->chip->usb_id) == 0x0582 /* Roland */ &&
-	    search_roland_implicit_fb(dev, altsd->bInterfaceNumber + 1,
+	    search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
 				      altsd->bAlternateSetting,
 				      &alts, &ep) >= 0) {
 		implicit_fb = 1;
 		goto add_sync_ep;
 	}
 
+	/* No quirk */
+	return 0;
+
+add_sync_ep:
+	subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+						   alts, ep, !subs->direction,
+						   implicit_fb ?
+							SND_USB_ENDPOINT_TYPE_DATA :
+							SND_USB_ENDPOINT_TYPE_SYNC);
+	if (!subs->sync_endpoint)
+		return -EINVAL;
+
+	subs->data_endpoint->sync_master = subs->sync_endpoint;
+
+	return 0;
+}
+
+
+
+static int set_sync_endpoint(struct snd_usb_substream *subs,
+			     struct audioformat *fmt,
+			     struct usb_device *dev,
+			     struct usb_host_interface *alts,
+			     struct usb_interface_descriptor *altsd)
+{
+	int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
+	unsigned int ep, attr;
+	int implicit_fb = 0;
+	int err;
+
+	if (subs->sync_endpoint)
+		return 0;
+	
+	/* we need a sync pipe in async OUT or adaptive IN mode */
+	/* check the number of EP, since some devices have broken
+	 * descriptors which fool us.  if it has only one EP,
+	 * assume it as adaptive-out or sync-in.
+	 */
+	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
+
+	err = set_sync_ep_implicit_fb_quirk(subs, dev, altsd, attr);
+	if (err < 0)
+		return err;
+	if (subs->sync_endpoint)
+		return 0;
+
 	if (((is_playback && attr == USB_ENDPOINT_SYNC_ASYNC) ||
 	     (!is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE)) &&
 	    altsd->bNumEndpoints >= 2) {

@@ -420,7 +458,6 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 		implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
 				== USB_ENDPOINT_USAGE_IMPLICIT_FB;
 
-add_sync_ep:
 		subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
 							   alts, ep, !subs->direction,
 							   implicit_fb ?

@@ -493,7 +530,7 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 	if (!subs->data_endpoint)
 		return -EINVAL;
 
-	err = set_sync_endpoint(subs, fmt, alts, altsd);
+	err = set_sync_endpoint(subs, fmt, dev, alts, altsd);
 	if (err < 0)
 		return err;
Signature · 1 line
1.8.1.5
author
#5 lore raw

RFC PATCH 5/10 ALSA: usb-audio: reverse condition logic in set_sync_endpoint

In reply to Eldad Zack

Reverse logic on the conditions required to qualify for a sync endpoint and remove one level of indendation.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 81 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 39 deletions(-)
sound/usb/pcm.c +42 -39
index 549964a..b6dee7a 100644
@@ -426,48 +426,51 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	if (subs->sync_endpoint)
 		return 0;
 
-	if (((is_playback && attr == USB_ENDPOINT_SYNC_ASYNC) ||
-	     (!is_playback && attr == USB_ENDPOINT_SYNC_ADAPTIVE)) &&
-	    altsd->bNumEndpoints >= 2) {
-		/* check sync-pipe endpoint */
-		/* ... and check descriptor size before accessing bSynchAddress
-		   because there is a version of the SB Audigy 2 NX firmware lacking
-		   the audio fields in the endpoint descriptors */
-		if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
-		    (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
-		     get_endpoint(alts, 1)->bSynchAddress != 0 &&
-		     !implicit_fb)) {
-			snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
-				   dev->devnum, fmt->iface, fmt->altsetting,
-				   get_endpoint(alts, 1)->bmAttributes,
-				   get_endpoint(alts, 1)->bLength,
-				   get_endpoint(alts, 1)->bSynchAddress);
-			return -EINVAL;
-		}
-		ep = get_endpoint(alts, 1)->bEndpointAddress;
-		if (!implicit_fb &&
-		    get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
-		    (( is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
-		     (!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
-			snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
-				   dev->devnum, fmt->iface, fmt->altsetting,
-				   is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
-			return -EINVAL;
-		}
+	if (altsd->bNumEndpoints < 2)
+		return 0;
 
-		implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
-				== USB_ENDPOINT_USAGE_IMPLICIT_FB;
+	if ((is_playback && attr != USB_ENDPOINT_SYNC_ASYNC) ||
+	     (!is_playback && attr != USB_ENDPOINT_SYNC_ADAPTIVE))
+	     return 0;
+
+	/* check sync-pipe endpoint */
+	/* ... and check descriptor size before accessing bSynchAddress
+	   because there is a version of the SB Audigy 2 NX firmware lacking
+	   the audio fields in the endpoint descriptors */
+	if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
+	    (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+	     get_endpoint(alts, 1)->bSynchAddress != 0 &&
+	     !implicit_fb)) {
+		snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
+			   dev->devnum, fmt->iface, fmt->altsetting,
+			   get_endpoint(alts, 1)->bmAttributes,
+			   get_endpoint(alts, 1)->bLength,
+			   get_endpoint(alts, 1)->bSynchAddress);
+		return -EINVAL;
+	}
+	ep = get_endpoint(alts, 1)->bEndpointAddress;
+	if (!implicit_fb &&
+	    get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+	    (( is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
+	     (!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
+		snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
+			   dev->devnum, fmt->iface, fmt->altsetting,
+			   is_playback, ep, get_endpoint(alts, 0)->bSynchAddress);
+		return -EINVAL;
+	}
 
-		subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
-							   alts, ep, !subs->direction,
-							   implicit_fb ?
-								SND_USB_ENDPOINT_TYPE_DATA :
-								SND_USB_ENDPOINT_TYPE_SYNC);
-		if (!subs->sync_endpoint)
-			return -EINVAL;
+	implicit_fb = (get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_USAGE_MASK)
+			== USB_ENDPOINT_USAGE_IMPLICIT_FB;
 
-		subs->data_endpoint->sync_master = subs->sync_endpoint;
-	}
+	subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+						   alts, ep, !subs->direction,
+						   implicit_fb ?
+							SND_USB_ENDPOINT_TYPE_DATA :
+							SND_USB_ENDPOINT_TYPE_SYNC);
+	if (!subs->sync_endpoint)
+		return -EINVAL;
+
+	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
 	return 0;
 }
Signature · 1 line
1.8.1.5
author
#6 lore raw

RFC PATCH 6/10 ALSA: usb-audio: do not initialize and check implicit_fb

In reply to Eldad Zack

Since implicit_fb is not changed, !implicit_fb will always be true - it is set only after these checks.
Similarly, there's also no need to set it at the top of the function.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
sound/usb/pcm.c +3 -7
index b6dee7a..8865833 100644
@@ -397,8 +397,6 @@ add_sync_ep:
 	return 0;
 }
 
-
-
 static int set_sync_endpoint(struct snd_usb_substream *subs,
 			     struct audioformat *fmt,
 			     struct usb_device *dev,

@@ -407,7 +405,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 {
 	int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
 	unsigned int ep, attr;
-	int implicit_fb = 0;
+	int implicit_fb;
 	int err;
 
 	if (subs->sync_endpoint)

@@ -439,8 +437,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 	   the audio fields in the endpoint descriptors */
 	if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
 	    (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
-	     get_endpoint(alts, 1)->bSynchAddress != 0 &&
-	     !implicit_fb)) {
+	     get_endpoint(alts, 1)->bSynchAddress != 0)) {
 		snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. bmAttributes %02x, bLength %d, bSynchAddress %02x\n",
 			   dev->devnum, fmt->iface, fmt->altsetting,
 			   get_endpoint(alts, 1)->bmAttributes,

@@ -449,8 +446,7 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 		return -EINVAL;
 	}
 	ep = get_endpoint(alts, 1)->bEndpointAddress;
-	if (!implicit_fb &&
-	    get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
+	if  (get_endpoint(alts, 0)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
 	    (( is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress | USB_DIR_IN)) ||
 	     (!is_playback && ep != (unsigned int)(get_endpoint(alts, 0)->bSynchAddress & ~USB_DIR_IN)))) {
 		snd_printk(KERN_ERR "%d:%d:%d : invalid sync pipe. is_playback %d, ep %02x, bSynchAddress %02x\n",
Signature · 1 line
1.8.1.5
author
#7 lore raw

RFC PATCH 7/10 ALSA: usb-audio: remove is_playback from implicit feedback quirks

In reply to Eldad Zack

An implicit feedback endpoint can only be a capture source. The consumer (sink) of the implicit feedback endpoint is therefore limited to playback EPs.
Check if the target endpoint is a playback first and remove redundant checks.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)
sound/usb/pcm.c +19 -21
index 8865833..c14cb78 100644
@@ -334,41 +334,39 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 {
 	struct usb_host_interface *alts;
 	struct usb_interface *iface;
-	int is_playback = subs->direction == SNDRV_PCM_STREAM_PLAYBACK;
 	int implicit_fb = 0;
 	unsigned int ep;
 
+	/* Implicit feedback sync EPs consumers are always playback EPs */
+	if (subs->direction != SNDRV_PCM_STREAM_PLAYBACK)
+		return 0;
+
 	switch (subs->stream->chip->usb_id) {
 	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
 	case USB_ID(0x0763, 0x2031): /* M-Audio Fast Track C600 */
-		if (is_playback) {
-			implicit_fb = 1;
-			ep = 0x81;
-			iface = usb_ifnum_to_if(dev, 3);
+		implicit_fb = 1;
+		ep = 0x81;
+		iface = usb_ifnum_to_if(dev, 3);
 
-			if (!iface || iface->num_altsetting == 0)
-				return -EINVAL;
+		if (!iface || iface->num_altsetting == 0)
+			return -EINVAL;
 
-			alts = &iface->altsetting[1];
-			goto add_sync_ep;
-		}
+		alts = &iface->altsetting[1];
+		goto add_sync_ep;
 		break;
 	case USB_ID(0x0763, 0x2080): /* M-Audio FastTrack Ultra */
 	case USB_ID(0x0763, 0x2081):
-		if (is_playback) {
-			implicit_fb = 1;
-			ep = 0x81;
-			iface = usb_ifnum_to_if(dev, 2);
+		implicit_fb = 1;
+		ep = 0x81;
+		iface = usb_ifnum_to_if(dev, 2);
 
-			if (!iface || iface->num_altsetting == 0)
-				return -EINVAL;
+		if (!iface || iface->num_altsetting == 0)
+			return -EINVAL;
 
-			alts = &iface->altsetting[1];
-			goto add_sync_ep;
-		}
+		alts = &iface->altsetting[1];
+		goto add_sync_ep;
 	}
-	if (is_playback &&
-	    attr == USB_ENDPOINT_SYNC_ASYNC &&
+	if (attr == USB_ENDPOINT_SYNC_ASYNC &&
 	    altsd->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
 	    altsd->bInterfaceProtocol == 2 &&
 	    altsd->bNumEndpoints == 1 &&
Signature · 1 line
1.8.1.5
author
#8 lore raw

RFC PATCH 8/10 ALSA: usb-audio: remove implicit_fb from quirk

In reply to Eldad Zack

Since the quirks all apply to implicit feedback, there's no need to set and check for it later.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
sound/usb/pcm.c +1 -7
index c14cb78..9f9639d 100644
@@ -334,7 +334,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 {
 	struct usb_host_interface *alts;
 	struct usb_interface *iface;
-	int implicit_fb = 0;
 	unsigned int ep;
 
 	/* Implicit feedback sync EPs consumers are always playback EPs */

@@ -344,7 +343,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 	switch (subs->stream->chip->usb_id) {
 	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
 	case USB_ID(0x0763, 0x2031): /* M-Audio Fast Track C600 */
-		implicit_fb = 1;
 		ep = 0x81;
 		iface = usb_ifnum_to_if(dev, 3);
 

@@ -356,7 +354,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 		break;
 	case USB_ID(0x0763, 0x2080): /* M-Audio FastTrack Ultra */
 	case USB_ID(0x0763, 0x2081):
-		implicit_fb = 1;
 		ep = 0x81;
 		iface = usb_ifnum_to_if(dev, 2);
 

@@ -374,7 +371,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 	    search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
 				      altsd->bAlternateSetting,
 				      &alts, &ep) >= 0) {
-		implicit_fb = 1;
 		goto add_sync_ep;
 	}
 

@@ -384,9 +380,7 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 add_sync_ep:
 	subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
 						   alts, ep, !subs->direction,
-						   implicit_fb ?
-							SND_USB_ENDPOINT_TYPE_DATA :
-							SND_USB_ENDPOINT_TYPE_SYNC);
+						   SND_USB_ENDPOINT_TYPE_DATA);
 	if (!subs->sync_endpoint)
 		return -EINVAL;
Signature · 1 line
1.8.1.5
author
#9 lore raw

RFC PATCH 9/10 ALSA: usb-audio: clarify implicit fb quirk for Roland

In reply to Eldad Zack

Move the vendor ID check to the outside and also propogate the error code.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
sound/usb/pcm.c +14 -9
index 9f9639d..0704823 100644
@@ -363,15 +363,20 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 		alts = &iface->altsetting[1];
 		goto add_sync_ep;
 	}
-	if (attr == USB_ENDPOINT_SYNC_ASYNC &&
-	    altsd->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
-	    altsd->bInterfaceProtocol == 2 &&
-	    altsd->bNumEndpoints == 1 &&
-	    USB_ID_VENDOR(subs->stream->chip->usb_id) == 0x0582 /* Roland */ &&
-	    search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
-				      altsd->bAlternateSetting,
-				      &alts, &ep) >= 0) {
-		goto add_sync_ep;
+
+	if (USB_ID_VENDOR(subs->stream->chip->usb_id) == 0x0582) { /* Roland */
+		if (attr == USB_ENDPOINT_SYNC_ASYNC &&
+		    altsd->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
+		    altsd->bInterfaceProtocol == 2 &&
+		    altsd->bNumEndpoints == 1) {
+			int err = search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
+							altsd->bAlternateSetting,
+						        &alts, &ep);
+			if (err < 0)
+				return err;
+
+			goto add_sync_ep;
+		}
 	}
 
 	/* No quirk */
Signature · 1 line
1.8.1.5
#10 lore raw

Re: [PATCH RFC 09/10] ALSA: usb-audio: clarify implicit fb quirk for Roland

Quoted 6 lines · Eldad Zack

Move the vendor ID check to the outside and also propogate the error code.

(quoted diff) +0 -3
-	if (...
-         search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
-				      altsd->bAlternateSetting,

Review

Quoted 10 lines
(quoted diff) +7 -2
-				      &alts, &ep) >= 0) {
-		goto add_sync_ep;
 
+			int err = search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
+							altsd->bAlternateSetting,
+						        &alts, &ep);
+			if (err < 0)
+				return err;
+
+			goto add_sync_ep;

The error code change is not a refactoring, but a logic change.

In this case, a nonexistent feedback endpoint should not be considered a fatal error.

Regards,
Clemens

author
#11 lore raw

Re: [PATCH RFC 09/10] ALSA: usb-audio: clarify implicit fb quirk for Roland

Quoted 20 lines · Clemens Ladisch
Quoted 6 lines · Eldad Zack

Move the vendor ID check to the outside and also propogate the error code.

(quoted diff) +0 -3
-	if (...
-         search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
-				      altsd->bAlternateSetting,

Review

Quoted 10 lines
(quoted diff) +7 -2
-				      &alts, &ep) >= 0) {
-		goto add_sync_ep;
 
+			int err = search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,
+							altsd->bAlternateSetting,
+						        &alts, &ep);
+			if (err < 0)
+				return err;
+
+			goto add_sync_ep;

The error code change is not a refactoring, but a logic change.

Quoted 1 line

In this case, a nonexistent feedback endpoint should not be considered a fatal error.

Right, thanks!
I'll change it to return 0.

Cheers,
Eldad

author
#12 lore raw

RFC PATCH 10/10 ALSA: usb-audio: check alts is not NULL before adding ep

In reply to Eldad Zack

snd_usb_add_endpoint assumes alts is not NULL. Before passing alts, check if it is the case (for future-proofing).
Since it is also an indication that a quirk is needed, remove the redundant goto statements.

Signed-off-by Eldad Zack <[email protected]>
---
 sound/usb/pcm.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
sound/usb/pcm.c +3 -7
index 0704823..e70f70b 100644
@@ -332,7 +332,7 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 					 struct usb_interface_descriptor *altsd,
 					 unsigned int attr)
 {
-	struct usb_host_interface *alts;
+	struct usb_host_interface *alts = NULL;
 	struct usb_interface *iface;
 	unsigned int ep;
 

@@ -350,7 +350,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 			return -EINVAL;
 
 		alts = &iface->altsetting[1];
-		goto add_sync_ep;
 		break;
 	case USB_ID(0x0763, 0x2080): /* M-Audio FastTrack Ultra */
 	case USB_ID(0x0763, 0x2081):

@@ -361,7 +360,6 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 			return -EINVAL;
 
 		alts = &iface->altsetting[1];
-		goto add_sync_ep;
 	}
 
 	if (USB_ID_VENDOR(subs->stream->chip->usb_id) == 0x0582) { /* Roland */

@@ -374,15 +372,13 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 						        &alts, &ep);
 			if (err < 0)
 				return err;
-
-			goto add_sync_ep;
 		}
 	}
 
 	/* No quirk */
-	return 0;
+	if (alts == NULL)
+		return 0;
 
-add_sync_ep:
 	subs->sync_endpoint = snd_usb_add_endpoint(subs->stream->chip,
 						   alts, ep, !subs->direction,
 						   SND_USB_ENDPOINT_TYPE_DATA);
Signature · 1 line
1.8.1.5