[alsa-devel] [PATCH v3 00/11] ALSA: usb-audio: fix playback/capture concurrent usage
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
Applies against current for-next (HEAD 68538bf2bce557c3b5fe8c59b034d45352500db1 ).
v2: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html v1: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html
* Changed "altset_idx" to "altsetting" thanks to Clemens' clarification.
#3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace calling snd_pcm_open() and then snd_pcm_close() immediately. #8: subs->altset_idx was left intact, since it corresponds to the array index. #10: this patch now removed subs->altset_idx, but also changes the check to be performed against fmt->altsetting. #11: New. This changes the name of snd_usb_endpoint member alt_idx to altsetting for the same reason Clemens described.
Clemens and Daniel - thank you very much for your help!
Cheers, Eldad
Eldad Zack (11): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: rename alt_idx to altsetting
sound/usb/card.h | 4 +- sound/usb/endpoint.c | 82 ++++++++++++++++++------- sound/usb/endpoint.h | 11 +++- sound/usb/pcm.c | 168 ++++++++++++++++++++++++++++++++++++++------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 201 insertions(+), 70 deletions(-)
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;
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);
If an endpoint uses another endpoint for synchronization, and the other endpoint is stopped, an oops will occur on NULL dereference. Clearing the prepare/retire callbacks solves this issue.
v2: Thanks to Daniel Mack, fixed (an irnoic) NULL dereference when the pcm substream is opened and closed immediately.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 9ec401a..4227d34 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1199,6 +1199,11 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) subs->interface = -1; }
+ if (subs->data_endpoint) { + subs->data_endpoint->prepare_data_urb = NULL; + subs->data_endpoint->retire_data_urb = NULL; + } + subs->pcm_substream = NULL; snd_usb_autosuspend(subs->stream->chip);
@@ -1525,6 +1530,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: + subs->data_endpoint->prepare_data_urb = NULL; + subs->data_endpoint->retire_data_urb = NULL; stop_endpoints(subs, false); subs->running = 0; return 0; @@ -1555,6 +1562,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: + subs->data_endpoint->retire_data_urb = NULL; stop_endpoints(subs, false); subs->running = 0; return 0;
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;
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);
If setting the interface fails, the SUBSTREAM_FLAG_SYNC_EP_STARTED should be cleared.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 4227d34..ef9664f 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -246,6 +246,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) subs->sync_endpoint->iface, subs->sync_endpoint->alt_idx); if (err < 0) { + clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR "%d:%d:%d: cannot set interface (%d)\n", subs->dev->devnum,
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); } }
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Thanks to Clemens Ladisch for pointing out that cur_altsetting in usb_interface can be used to determine the current altsetting index as well as the correct naming convention for the altsetting number.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: * Removed get_interface usage. * Fixed NULL dereference for explicit feedback - now other_subs will be set only after testing that subs is not NULL. v3: * Changed "altset_idx" to "altsetting". --- sound/usb/pcm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 8 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index ef9664f..a1332d0 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -216,6 +216,71 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, } }
+static int get_cur_altsetting(struct usb_device *dev, int ifnum) +{ + struct usb_interface *iface; + struct usb_interface_descriptor *altsd; + + if (ifnum == -1) + return 0; + + iface = usb_ifnum_to_if(dev, ifnum); + if (!iface) + return -EINVAL; + + if (!iface->cur_altsetting) + return -EINVAL; + altsd = &(iface->cur_altsetting->desc); + + return altsd->bAlternateSetting; +} + +static int subs_set_interface(struct snd_usb_substream *subs, int ifnum, + int altsetting) +{ + struct snd_usb_substream *other_subs; + int err; + + snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n", + __func__, ifnum, altsetting); + + if (subs == NULL) + return usb_set_interface(subs->dev, ifnum, altsetting); + other_subs = &(subs->stream->substream[subs->direction ^ 1]); + + if (other_subs->data_endpoint && + other_subs->data_endpoint->use_count != 0) { + int cur_altsetting = get_cur_altsetting(subs->dev, ifnum); + + if (cur_altsetting == altsetting) + return 0; + + if (cur_altsetting > 0 && altsetting == 0) + return 0; + } + + err = usb_set_interface(subs->dev, ifnum, altsetting); + snd_printdd(KERN_DEBUG "%s: Interface %d alts set to %d (err %d)\n", + __func__, ifnum, altsetting, err); + if (err < 0) + return err; + + subs->interface = altsetting == 0 ? -1 : ifnum; + + return 0; +} + +int snd_usb_set_interface(struct snd_usb_substream *subs, int ifnum, + int altset_idx) +{ + return subs_set_interface(subs, ifnum, altset_idx); +} + +int snd_usb_close_interface(struct snd_usb_substream *subs, int ifnum) +{ + return subs_set_interface(subs, ifnum, 0); +} + static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) { int err; @@ -242,9 +307,9 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
if (subs->data_endpoint->iface != subs->sync_endpoint->iface || subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) { - err = usb_set_interface(subs->dev, - subs->sync_endpoint->iface, - subs->sync_endpoint->alt_idx); + err = snd_usb_set_interface(subs, + subs->sync_endpoint->iface, + subs->sync_endpoint->alt_idx); if (err < 0) { clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR @@ -467,20 +532,19 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
/* close the old interface */ if (subs->interface >= 0 && subs->interface != fmt->iface) { - err = usb_set_interface(subs->dev, subs->interface, 0); + err = snd_usb_close_interface(subs, subs->interface); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n", dev->devnum, fmt->iface, fmt->altsetting, err); return -EIO; } - subs->interface = -1; subs->altset_idx = 0; }
/* set interface */ if (subs->interface != fmt->iface || subs->altset_idx != fmt->altset_idx) { - err = usb_set_interface(dev, fmt->iface, fmt->altsetting); + err = snd_usb_set_interface(subs, fmt->iface, fmt->altsetting); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n", dev->devnum, fmt->iface, fmt->altsetting, err); @@ -488,7 +552,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) } snd_printdd(KERN_INFO "setting usb interface %d:%d\n", fmt->iface, fmt->altsetting); - subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx;
snd_usb_set_interface_quirk(dev); @@ -1196,7 +1259,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) stop_endpoints(subs, true);
if (!as->chip->shutdown && subs->interface >= 0) { - usb_set_interface(subs->dev, subs->interface, 0); + snd_usb_close_interface(subs, subs->interface); subs->interface = -1; }
If the current endpoint is in use, check if the hardware parameters match. If they do, allow the usage. This also requires setting the parameters in case an data endpoint is used as a synchornization source, and it is first set by its sink endpoint.
Add the same check to hw_params, to prevent overwriting the current params. Clear the hardware parameteres on hw_free only if the other endpoint is not in use.
No change for sync endpoints (explicit feedback).
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- sound/usb/endpoint.h | 9 ++++++++- sound/usb/pcm.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 91 insertions(+), 15 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 6ff36f5..7e04ebb 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -742,6 +742,34 @@ out_of_memory: return -ENOMEM; }
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate) +{ + /* no associated substream */ + if (!subs) + return true; + + /* data_endpoint not yet initialized */ + if (!subs->data_endpoint) + return true; + + if (subs->data_endpoint->use_count == 0) + return true; + + if (subs->pcm_format != pcm_format || + subs->channels != channels || + subs->period_bytes != period_bytes || + subs->cur_rate != rate) + return false; + + snd_printdd(KERN_INFO "ep #%x in use, parameters match\n", + subs->data_endpoint->ep_num); + return true; +} + /** * snd_usb_endpoint_set_params: configure an snd_usb_endpoint * @@ -752,6 +780,7 @@ out_of_memory: * @rate: the frame rate. * @fmt: the USB audio format information * @sync_ep: the sync endpoint to use, if any + * @subs: the substream that uses this endpoint as data endpoint * * Determine the number of URBs to be used on this endpoint. * An endpoint must be configured before it can be started. @@ -763,14 +792,23 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, unsigned int period_bytes, unsigned int rate, struct audioformat *fmt, - struct snd_usb_endpoint *sync_ep) + struct snd_usb_endpoint *sync_ep, + struct snd_usb_substream *subs) { int err;
if (ep->param_set || ep->use_count != 0) { - snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n", - ep->ep_num); - return -EBUSY; + if (!subs || + !snd_usb_endpoint_may_set_params(subs, pcm_format, + channels, period_bytes, + rate)) { + snd_printk(KERN_WARNING + "Unable to change format on ep #%x: already in use\n", + ep->ep_num); + return -EBUSY; + } + /* Endpoint already set with the same parameters */ + return 0; } ep->param_set = true;
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index c928e0c..64f2b08 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -14,7 +14,8 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, unsigned int period_bytes, unsigned int rate, struct audioformat *fmt, - struct snd_usb_endpoint *sync_ep); + struct snd_usb_endpoint *sync_ep, + struct snd_usb_substream *subs);
int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); @@ -30,4 +31,10 @@ void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sender, const struct urb *urb);
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate); + #endif /* __USBAUDIO_ENDPOINT_H */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index a1332d0..d58fbcf 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -645,6 +645,7 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) subs->period_bytes, subs->cur_rate, subs->cur_audiofmt, + NULL, NULL);
/* Try to find the best matching audioformat. */ @@ -681,9 +682,18 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) sync_period_bytes, subs->cur_rate, sync_fp, - NULL); + NULL, + sync_subs); + if (ret < 0) + return ret;
- return ret; + sync_subs->pcm_format = subs->pcm_format; + sync_subs->period_bytes = sync_period_bytes; + sync_subs->channels = sync_fp->channels; + sync_subs->cur_rate = subs->cur_rate; + sync_subs->data_endpoint = subs->sync_endpoint; + + return 0; }
/* @@ -703,7 +713,8 @@ static int configure_endpoint(struct snd_usb_substream *subs) subs->period_bytes, subs->cur_rate, subs->cur_audiofmt, - subs->sync_endpoint); + subs->sync_endpoint, + subs); if (ret < 0) return ret;
@@ -729,16 +740,32 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, struct snd_usb_substream *subs = substream->runtime->private_data; struct audioformat *fmt; int ret; + snd_pcm_format_t pcm_format; + unsigned int channels; + unsigned int period_bytes; + unsigned int rate;
ret = snd_pcm_lib_alloc_vmalloc_buffer(substream, params_buffer_bytes(hw_params)); if (ret < 0) return ret;
- subs->pcm_format = params_format(hw_params); - subs->period_bytes = params_period_bytes(hw_params); - subs->channels = params_channels(hw_params); - subs->cur_rate = params_rate(hw_params); + pcm_format = params_format(hw_params); + period_bytes = params_period_bytes(hw_params); + channels = params_channels(hw_params); + rate = params_rate(hw_params); + + if (!snd_usb_endpoint_may_set_params(subs, pcm_format, channels, + period_bytes, rate)) { + snd_printk(KERN_WARNING "Unable to set hw_params on substream (dir: %d), data EP in use\n", + subs->direction); + return -EBUSY; + } + + subs->pcm_format = pcm_format; + subs->period_bytes = period_bytes; + subs->channels = channels; + subs->cur_rate = rate;
fmt = find_format(subs); if (!fmt) { @@ -772,9 +799,13 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) { struct snd_usb_substream *subs = substream->runtime->private_data;
- subs->cur_audiofmt = NULL; - subs->cur_rate = 0; - subs->period_bytes = 0; + if (!subs->data_endpoint || subs->data_endpoint->use_count == 0) { + subs->cur_audiofmt = NULL; + subs->cur_rate = 0; + subs->period_bytes = 0; + subs->channels = 0; + } + down_read(&subs->stream->chip->shutdown_rwsem); if (!subs->stream->chip->shutdown) { stop_endpoints(subs, true);
We only need to check the current altsetting in one place, so there's no benefit from tracking it. This patch replaces that one check with get_cur_altset_idx() added in a previous patch and in the proc substream dump routine.
Thanks to Clemens Ladisch for pointing out the difference between altset_idx and altsetting - the check is changed accordingly to match against fmt->altsetting.
Note that the proc dump now returns the altsetting number and not the array index.
This allows removing altset_idx from snd_usb_substream.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: corrected check. --- sound/usb/card.h | 1 - sound/usb/pcm.c | 8 ++------ sound/usb/pcm.h | 2 ++ sound/usb/proc.c | 4 +++- 4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 4061ee1..0fd3f8b 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -117,7 +117,6 @@ struct snd_usb_substream { unsigned int channels_max; /* max channels in the all audiofmts */ unsigned int cur_rate; /* current rate (for hw_params callback) */ unsigned int period_bytes; /* current period bytes (for hw_params callback) */ - unsigned int altset_idx; /* USB data format: index of alternate setting */ unsigned int txfr_quirk:1; /* allow sub-frame alignment */ unsigned int fmt_type; /* USB audio format type (1-3) */ unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index d58fbcf..5018b30 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -216,7 +216,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, } }
-static int get_cur_altsetting(struct usb_device *dev, int ifnum) +int get_cur_altsetting(struct usb_device *dev, int ifnum) { struct usb_interface *iface; struct usb_interface_descriptor *altsd; @@ -538,12 +538,11 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) dev->devnum, fmt->iface, fmt->altsetting, err); return -EIO; } - subs->altset_idx = 0; }
/* set interface */ if (subs->interface != fmt->iface || - subs->altset_idx != fmt->altset_idx) { + get_cur_altsetting(subs->dev, subs->interface) != fmt->altsetting) { err = snd_usb_set_interface(subs, fmt->iface, fmt->altsetting); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n", @@ -552,7 +551,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) } snd_printdd(KERN_INFO "setting usb interface %d:%d\n", fmt->iface, fmt->altsetting); - subs->altset_idx = fmt->altset_idx;
snd_usb_set_interface_quirk(dev); } @@ -784,7 +782,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, return ret;
subs->interface = fmt->iface; - subs->altset_idx = fmt->altset_idx; subs->need_setup_ep = true;
return 0; @@ -1268,7 +1265,6 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) struct snd_usb_substream *subs = &as->substream[direction];
subs->interface = -1; - subs->altset_idx = 0; runtime->hw = snd_usb_hardware; runtime->private_data = subs; subs->pcm_substream = substream; diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h index df7a003..0c9cb8c 100644 --- a/sound/usb/pcm.h +++ b/sound/usb/pcm.h @@ -10,5 +10,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, struct usb_host_interface *alts, struct audioformat *fmt);
+int get_cur_altsetting(struct usb_device *dev, int ifnum); +
#endif /* __USBAUDIO_PCM_H */ diff --git a/sound/usb/proc.c b/sound/usb/proc.c index 5f761ab..ab6aafe 100644 --- a/sound/usb/proc.c +++ b/sound/usb/proc.c @@ -27,6 +27,7 @@ #include "card.h" #include "endpoint.h" #include "proc.h" +#include "pcm.h"
/* convert our full speed USB rate into sampling rate in Hz */ static inline unsigned get_full_speed_hz(unsigned int usb_rate) @@ -140,7 +141,8 @@ static void proc_dump_substream_status(struct snd_usb_substream *subs, struct sn if (subs->running) { snd_iprintf(buffer, " Status: Running\n"); snd_iprintf(buffer, " Interface = %d\n", subs->interface); - snd_iprintf(buffer, " Altset = %d\n", subs->altset_idx); + snd_iprintf(buffer, " Altset = %d\n", + get_cur_altsetting(subs->dev, subs->interface)); proc_dump_ep_status(subs, subs->data_endpoint, subs->sync_endpoint, buffer); } else { snd_iprintf(buffer, " Status: Stop\n");
As Clemens Ladisch kindly explained: "Please note that there are two methods to identify alternate settings: the number, which is the value in bAlternateSetting, and the index, which is the index in the descriptor array. There might be some wording in the USB spec that these two values must be the same, but in reality, [insert standard rant about firmware writers], bAlternateSetting must be treated as a random ID value."
This patch changes the name to express the correct usage semantics. No functional change.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/card.h | 2 +- sound/usb/endpoint.c | 6 +++--- sound/usb/pcm.c | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 0fd3f8b..ce204d8 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -96,7 +96,7 @@ struct snd_usb_endpoint { unsigned int syncinterval; /* P for adaptive mode, 0 otherwise */ unsigned char silence_value; unsigned int stride; - int iface, alt_idx; + int iface, altsetting; int skip_packets; /* quirks for devices to ignore the first n packets in a stream */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 7e04ebb..86aece0 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -426,9 +426,9 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, list_for_each_entry(ep, &chip->ep_list, list) { if (ep->ep_num == ep_num && ep->iface == alts->desc.bInterfaceNumber && - ep->alt_idx == alts->desc.bAlternateSetting) { + ep->altsetting == alts->desc.bAlternateSetting) { snd_printdd(KERN_DEBUG "Re-using EP %x in iface %d,%d @%p\n", - ep_num, ep->iface, ep->alt_idx, ep); + ep_num, ep->iface, ep->altsetting, ep); goto __exit_unlock; } } @@ -447,7 +447,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, ep->type = type; ep->ep_num = ep_num; ep->iface = alts->desc.bInterfaceNumber; - ep->alt_idx = alts->desc.bAlternateSetting; + ep->altsetting = alts->desc.bAlternateSetting; INIT_LIST_HEAD(&ep->ready_playback_urbs); ep_num &= USB_ENDPOINT_NUMBER_MASK;
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 5018b30..283e6c0 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -306,17 +306,17 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) struct snd_usb_endpoint *ep = subs->sync_endpoint;
if (subs->data_endpoint->iface != subs->sync_endpoint->iface || - subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) { + subs->data_endpoint->altsetting != subs->sync_endpoint->altsetting) { err = snd_usb_set_interface(subs, subs->sync_endpoint->iface, - subs->sync_endpoint->alt_idx); + subs->sync_endpoint->altsetting); if (err < 0) { clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR "%d:%d:%d: cannot set interface (%d)\n", subs->dev->devnum, subs->sync_endpoint->iface, - subs->sync_endpoint->alt_idx, err); + subs->sync_endpoint->altsetting, err); return -EIO; } }
On Sun, 25 Aug 2013, Eldad Zack wrote:
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
My crude regression tester caught a problem causing the streams to fail with this order (though it doesn't trip any WARNs or oops):
++ Test stream permutation 0000013f * [1/14] capture => pcm_open * [2/14] capture => pcm_hw_params * [3/14] capture => pcm_prepare * [4/14] capture => pcm_start * [5/14] capture => waitsleep * [6/14] capture => pcm_stop ++ Frames [capture]: 39360 * [7/14] playback => pcm_open * [8/14] playback => pcm_hw_params * [9/14] capture => pcm_close * [10/14] playback => pcm_prepare * [11/14] playback => pcm_start * [12/14] playback => waitsleep * [13/14] playback => pcm_stop ++ Frames [playback]: 101760 * [14/14] playback => pcm_close ++ Test stream permutation 0000015f * [1/14] capture => pcm_open * [2/14] capture => pcm_hw_params * [3/14] capture => pcm_prepare * [4/14] capture => pcm_start * [5/14] capture => waitsleep * [6/14] playback => pcm_open * [7/14] capture => pcm_stop ++ Frames [capture]: 0 !! Test stream permutation 0000015f failed !!
For reference, I ran the tool with my hda-intel card (all 3432 combinations) and there were no errors. So it's still doesn't cover all cases.
Cheers, Eldad
Hi.
I've tried this patch and can confirm that it fixes the issue with CM108 based USB headset I've described on the list several days ago (tried with Skype). Thank you!
2013/8/25 Eldad Zack eldad@fogrefinery.com
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
Applies against current for-next (HEAD 68538bf2bce557c3b5fe8c59b034d45352500db1 ).
v2:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html v1:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html
- Changed "altset_idx" to "altsetting" thanks to Clemens' clarification.
#3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace calling snd_pcm_open() and then snd_pcm_close() immediately. #8: subs->altset_idx was left intact, since it corresponds to the array index. #10: this patch now removed subs->altset_idx, but also changes the check to be performed against fmt->altsetting. #11: New. This changes the name of snd_usb_endpoint member alt_idx to altsetting for the same reason Clemens described.
Clemens and Daniel - thank you very much for your help!
Cheers, Eldad
Eldad Zack (11): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: rename alt_idx to altsetting
sound/usb/card.h | 4 +- sound/usb/endpoint.c | 82 ++++++++++++++++++------- sound/usb/endpoint.h | 11 +++- sound/usb/pcm.c | 168 ++++++++++++++++++++++++++++++++++++++------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 201 insertions(+), 70 deletions(-)
-- 1.8.1.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 26 Aug 2013, Nikolay Martynov wrote:
Hi.
I've tried this patch and can confirm that it fixes the issue with CM108 based USB headset I've described on the list several days ago (tried with Skype). Thank you!
That's great - I'm glad it solved your issue and thank you for testing!
There's still some issues I bump into at the moment, hopefully I will find the time to resolve them before the 3.13 window.
Cheers, Eldad
2013/8/25 Eldad Zack eldad@fogrefinery.com Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices. Applies against current for-next (HEAD 68538bf2bce557c3b5fe8c59b034d45352500db1 ). v2: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065649.html v1: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065517.html * Changed "altset_idx" to "altsetting" thanks to Clemens' clarification. #3: Thanks to Daniel's test, fixed a NULL dereference caused by userspace calling snd_pcm_open() and then snd_pcm_close() immediately. #8: subs->altset_idx was left intact, since it corresponds to the array index. #10: this patch now removed subs->altset_idx, but also changes the check to be performed against fmt->altsetting. #11: New. This changes the name of snd_usb_endpoint member alt_idx to altsetting for the same reason Clemens described. Clemens and Daniel - thank you very much for your help! Cheers, Eldad Eldad Zack (11): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: rename alt_idx to altsetting sound/usb/card.h | 4 +- sound/usb/endpoint.c | 82 ++++++++++++++++++------- sound/usb/endpoint.h | 11 +++- sound/usb/pcm.c | 168 ++++++++++++++++++++++++++++++++++++++------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 201 insertions(+), 70 deletions(-) -- 1.8.1.5 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel-- Martynov Nikolay. Email: mar.kolya@gmail.com
participants (2)
-
Eldad Zack -
Nikolay Martynov