[alsa-devel] [PATCH RFC 00/10] 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(-)
Code block does not compile when enabled.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 15b151e..3d3e8d1 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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; }
Following general kernel style.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 3d3e8d1..be5c7c2 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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;
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 eldad@fogrefinery.com --- sound/usb/pcm.c | 128 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 73 insertions(+), 55 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index be5c7c2..f667b7f 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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;
Separate implicit feedback quirks from setting a sync endpoint (which may also be explicit feedback or async).
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 71 +++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 17 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f667b7f..549964a 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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;
Reverse logic on the conditions required to qualify for a sync endpoint and remove one level of indendation.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 81 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 549964a..b6dee7a 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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; }
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 eldad@fogrefinery.com --- sound/usb/pcm.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index b6dee7a..8865833 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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",
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 eldad@fogrefinery.com --- sound/usb/pcm.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 8865833..c14cb78 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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 &&
Since the quirks all apply to implicit feedback, there's no need to set and check for it later.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index c14cb78..9f9639d 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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;
Move the vendor ID check to the outside and also propogate the error code.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 9f9639d..0704823 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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 */
Eldad Zack wrote:
Move the vendor ID check to the outside and also propogate the error code.
- if (...
search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,altsd->bAlternateSetting,&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
On Sat, 20 Jul 2013, Clemens Ladisch wrote:
Eldad Zack wrote:
Move the vendor ID check to the outside and also propogate the error code.
- if (...
search_roland_implicit_fb(subs->dev, altsd->bInterfaceNumber + 1,altsd->bAlternateSetting,&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.
Right, thanks! I'll change it to return 0.
Cheers, Eldad
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 eldad@fogrefinery.com --- sound/usb/pcm.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 0704823..e70f70b 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -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);
participants (2)
-
Clemens Ladisch -
Eldad Zack