[alsa-devel] [PATCH v2 00/10] ALSA: usb-audio: fix playback/capture concurrent usage
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(-)
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.
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;
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 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,
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 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; }
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
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
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);
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");
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
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
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
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);
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
participants (3)
-
Clemens Ladisch -
Daniel Mack -
Eldad Zack