[alsa-devel] [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks
In endpoint.c, bail out earlier in case the stream is stopped. Also clear the flag from deactivate_urbs() in case the chip's shutdown flag is set.
Signed-off-by: Daniel Mack zonque@gmail.com --- sound/usb/endpoint.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index e690690..a75bdf4 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb) urb->status == -ENODEV || /* device removed */ urb->status == -ECONNRESET || /* unlinked */ urb->status == -ESHUTDOWN || /* device disabled */ - ep->chip->shutdown)) /* device disconnected */ + ep->chip->shutdown) || /* device disconnected */ + !test_bit(EP_FLAG_RUNNING, &ep->flags)) goto exit_clear;
if (usb_pipeout(ep->pipe)) { @@ -522,13 +523,13 @@ static int deactivate_urbs(struct snd_usb_endpoint *ep, int force, int can_sleep unsigned int i; int async;
+ clear_bit(EP_FLAG_RUNNING, &ep->flags); + if (!force && ep->chip->shutdown) /* to be sure... */ return -EBADFD;
async = !can_sleep && ep->chip->async_unlink;
- clear_bit(EP_FLAG_RUNNING, &ep->flags); - INIT_LIST_HEAD(&ep->ready_playback_urbs); ep->next_packet_read_pos = 0; ep->next_packet_write_pos = 0;
The rework of the snd-usb endpoint logic moved the calls to snd_usb_set_interface() into the snd_usb_endpoint implemenation. This changed the order in which these calls are issued to the device, and thereby caused regressions for some webcams.
Fix this by moving the calls back to pcm.c for now to make it work again and use snd_usb_endpoint_activate() to really tear down all remaining URBs in the flight, consequently fixing another regression caused by USB packets on the wire after altsetting 0 has been selected.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-and-tested-by: Philipp Dreimann philipp@dreimann.net Reported-by: Joseph Salisbury joseph.salisbury@canonical.com --- sound/usb/endpoint.c | 73 +++++--------------------------------------------- sound/usb/pcm.c | 55 ++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 89 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index a75bdf4..4bc5778 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -415,7 +415,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, { struct list_head *p; struct snd_usb_endpoint *ep; - int ret, is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK; + int is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
mutex_lock(&chip->mutex);
@@ -435,16 +435,6 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, type == SND_USB_ENDPOINT_TYPE_DATA ? "data" : "sync", ep_num);
- /* select the alt setting once so the endpoints become valid */ - ret = usb_set_interface(chip->dev, alts->desc.bInterfaceNumber, - alts->desc.bAlternateSetting); - if (ret < 0) { - snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n", - __func__, ret); - ep = NULL; - goto __exit_unlock; - } - ep = kzalloc(sizeof(*ep), GFP_KERNEL); if (!ep) goto __exit_unlock; @@ -832,9 +822,6 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) if (++ep->use_count != 1) return 0;
- if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags))) - return -EINVAL; - /* just to be sure */ deactivate_urbs(ep, 0, 1); wait_clear_urbs(ep); @@ -912,9 +899,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, if (snd_BUG_ON(ep->use_count == 0)) return;
- if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags))) - return; - if (--ep->use_count == 0) { deactivate_urbs(ep, force, can_sleep); ep->data_subs = NULL; @@ -928,42 +912,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, }
/** - * snd_usb_endpoint_activate: activate an snd_usb_endpoint - * - * @ep: the endpoint to activate - * - * If the endpoint is not currently in use, this functions will select the - * correct alternate interface setting for the interface of this endpoint. - * - * 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_activate(struct snd_usb_endpoint *ep) -{ - if (ep->use_count != 0) - return 0; - - if (!ep->chip->shutdown && - !test_and_set_bit(EP_FLAG_ACTIVATED, &ep->flags)) { - int ret; - - ret = usb_set_interface(ep->chip->dev, ep->iface, ep->alt_idx); - if (ret < 0) { - snd_printk(KERN_ERR "%s() usb_set_interface() failed, ret = %d\n", - __func__, ret); - clear_bit(EP_FLAG_ACTIVATED, &ep->flags); - return ret; - } - - return 0; - } - - return -EBUSY; -} - -/** * snd_usb_endpoint_deactivate: deactivate an snd_usb_endpoint * * @ep: the endpoint to deactivate @@ -981,24 +929,15 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) if (!ep) return -EINVAL;
+ deactivate_urbs(ep, 1, 1); + wait_clear_urbs(ep); + if (ep->use_count != 0) return 0;
- if (!ep->chip->shutdown && - test_and_clear_bit(EP_FLAG_ACTIVATED, &ep->flags)) { - int ret; - - ret = usb_set_interface(ep->chip->dev, ep->iface, 0); - if (ret < 0) { - snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n", - __func__, ret); - return ret; - } + clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
- return 0; - } - - return -EBUSY; + return 0; }
/** diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f0ede13..26778a9 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -261,19 +261,6 @@ static void stop_endpoints(struct snd_usb_substream *subs, force, can_sleep, wait); }
-static int activate_endpoints(struct snd_usb_substream *subs) -{ - if (subs->sync_endpoint) { - int ret; - - ret = snd_usb_endpoint_activate(subs->sync_endpoint); - if (ret < 0) - return ret; - } - - return snd_usb_endpoint_activate(subs->data_endpoint); -} - static int deactivate_endpoints(struct snd_usb_substream *subs) { int reta, retb; @@ -314,6 +301,31 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) 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; + } + subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip, alts, fmt->endpoint, subs->direction, SND_USB_ENDPOINT_TYPE_DATA); @@ -460,12 +472,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, mutex_lock(&subs->stream->chip->shutdown_mutex); /* format changed */ stop_endpoints(subs, 0, 0, 0); - deactivate_endpoints(subs); - - ret = activate_endpoints(subs); - if (ret < 0) - goto unlock; - ret = snd_usb_endpoint_set_params(subs->data_endpoint, hw_params, fmt, subs->sync_endpoint); if (ret < 0) @@ -500,6 +506,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->period_bytes = 0; mutex_lock(&subs->stream->chip->shutdown_mutex); stop_endpoints(subs, 0, 1, 1); + deactivate_endpoints(subs); mutex_unlock(&subs->stream->chip->shutdown_mutex); return snd_pcm_lib_free_vmalloc_buffer(substream); } @@ -938,16 +945,20 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) { - int ret; struct snd_usb_stream *as = snd_pcm_substream_chip(substream); struct snd_usb_substream *subs = &as->substream[direction];
stop_endpoints(subs, 0, 0, 0); - ret = deactivate_endpoints(subs); + + if (!as->chip->shutdown && subs->interface >= 0) { + usb_set_interface(subs->dev, subs->interface, 0); + subs->interface = -1; + } + subs->pcm_substream = NULL; snd_usb_autosuspend(subs->stream->chip);
- return ret; + return 0; }
/* Since a URB can handle only a single linear buffer, we must use double
At Thu, 12 Jul 2012 15:19:47 +0200, Daniel Mack wrote:
The rework of the snd-usb endpoint logic moved the calls to snd_usb_set_interface() into the snd_usb_endpoint implemenation. This changed the order in which these calls are issued to the device, and thereby caused regressions for some webcams.
Fix this by moving the calls back to pcm.c for now to make it work again and use snd_usb_endpoint_activate() to really tear down all remaining URBs in the flight, consequently fixing another regression caused by USB packets on the wire after altsetting 0 has been selected.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-and-tested-by: Philipp Dreimann philipp@dreimann.net Reported-by: Joseph Salisbury joseph.salisbury@canonical.com
Looks like the patch contains lots of coding-style issues. Could you fix what checkpatch.pl compains and resubmit?
thanks,
Takashi
sound/usb/endpoint.c | 73 +++++--------------------------------------------- sound/usb/pcm.c | 55 ++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 89 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index a75bdf4..4bc5778 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -415,7 +415,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, { struct list_head *p; struct snd_usb_endpoint *ep;
- int ret, is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
int is_playback = direction == SNDRV_PCM_STREAM_PLAYBACK;
mutex_lock(&chip->mutex);
@@ -435,16 +435,6 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, type == SND_USB_ENDPOINT_TYPE_DATA ? "data" : "sync", ep_num);
- /* select the alt setting once so the endpoints become valid */
- ret = usb_set_interface(chip->dev, alts->desc.bInterfaceNumber,
alts->desc.bAlternateSetting);- if (ret < 0) {
snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n",__func__, ret);ep = NULL;goto __exit_unlock;- }
- ep = kzalloc(sizeof(*ep), GFP_KERNEL); if (!ep) goto __exit_unlock;
@@ -832,9 +822,6 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) if (++ep->use_count != 1) return 0;
- if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags)))
return -EINVAL;- /* just to be sure */ deactivate_urbs(ep, 0, 1); wait_clear_urbs(ep);
@@ -912,9 +899,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, if (snd_BUG_ON(ep->use_count == 0)) return;
- if (snd_BUG_ON(!test_bit(EP_FLAG_ACTIVATED, &ep->flags)))
return;- if (--ep->use_count == 0) { deactivate_urbs(ep, force, can_sleep); ep->data_subs = NULL;
@@ -928,42 +912,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, }
/**
- snd_usb_endpoint_activate: activate an snd_usb_endpoint
- @ep: the endpoint to activate
- If the endpoint is not currently in use, this functions will select the
- correct alternate interface setting for the interface of this endpoint.
- 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_activate(struct snd_usb_endpoint *ep) -{
- if (ep->use_count != 0)
return 0;- if (!ep->chip->shutdown &&
!test_and_set_bit(EP_FLAG_ACTIVATED, &ep->flags)) {int ret;ret = usb_set_interface(ep->chip->dev, ep->iface, ep->alt_idx);if (ret < 0) {snd_printk(KERN_ERR "%s() usb_set_interface() failed, ret = %d\n",__func__, ret);clear_bit(EP_FLAG_ACTIVATED, &ep->flags);return ret;}return 0;- }
- return -EBUSY;
-}
-/**
- snd_usb_endpoint_deactivate: deactivate an snd_usb_endpoint
- @ep: the endpoint to deactivate
@@ -981,24 +929,15 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) if (!ep) return -EINVAL;
- deactivate_urbs(ep, 1, 1);
- wait_clear_urbs(ep);
- if (ep->use_count != 0) return 0;
- if (!ep->chip->shutdown &&
test_and_clear_bit(EP_FLAG_ACTIVATED, &ep->flags)) {int ret;ret = usb_set_interface(ep->chip->dev, ep->iface, 0);if (ret < 0) {snd_printk(KERN_ERR "%s(): usb_set_interface() failed, ret = %d\n",__func__, ret);return ret;}
- clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
return 0;- }
- return -EBUSY;
- return 0;
}
/** diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f0ede13..26778a9 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -261,19 +261,6 @@ static void stop_endpoints(struct snd_usb_substream *subs, force, can_sleep, wait); }
-static int activate_endpoints(struct snd_usb_substream *subs) -{
- if (subs->sync_endpoint) {
int ret;ret = snd_usb_endpoint_activate(subs->sync_endpoint);if (ret < 0)return ret;- }
- return snd_usb_endpoint_activate(subs->data_endpoint);
-}
static int deactivate_endpoints(struct snd_usb_substream *subs) { int reta, retb; @@ -314,6 +301,31 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) 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;}- subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip, alts, fmt->endpoint, subs->direction, SND_USB_ENDPOINT_TYPE_DATA);
@@ -460,12 +472,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, mutex_lock(&subs->stream->chip->shutdown_mutex); /* format changed */ stop_endpoints(subs, 0, 0, 0);
deactivate_endpoints(subs);ret = activate_endpoints(subs);if (ret < 0)goto unlock;- ret = snd_usb_endpoint_set_params(subs->data_endpoint, hw_params, fmt, subs->sync_endpoint); if (ret < 0)
@@ -500,6 +506,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->period_bytes = 0; mutex_lock(&subs->stream->chip->shutdown_mutex); stop_endpoints(subs, 0, 1, 1);
- deactivate_endpoints(subs); mutex_unlock(&subs->stream->chip->shutdown_mutex); return snd_pcm_lib_free_vmalloc_buffer(substream);
} @@ -938,16 +945,20 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) {
int ret; struct snd_usb_stream *as = snd_pcm_substream_chip(substream); struct snd_usb_substream *subs = &as->substream[direction];
stop_endpoints(subs, 0, 0, 0);
ret = deactivate_endpoints(subs);
if (!as->chip->shutdown && subs->interface >= 0) {usb_set_interface(subs->dev, subs->interface, 0);subs->interface = -1;}- subs->pcm_substream = NULL; snd_usb_autosuspend(subs->stream->chip);
- return ret;
- return 0;
}
/* Since a URB can handle only a single linear buffer, we must use double
1.7.10.4
On 13.07.2012 07:58, Takashi Iwai wrote:
At Thu, 12 Jul 2012 15:19:47 +0200, Daniel Mack wrote:
The rework of the snd-usb endpoint logic moved the calls to snd_usb_set_interface() into the snd_usb_endpoint implemenation. This changed the order in which these calls are issued to the device, and thereby caused regressions for some webcams.
Fix this by moving the calls back to pcm.c for now to make it work again and use snd_usb_endpoint_activate() to really tear down all remaining URBs in the flight, consequently fixing another regression caused by USB packets on the wire after altsetting 0 has been selected.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-and-tested-by: Philipp Dreimann philipp@dreimann.net Reported-by: Joseph Salisbury joseph.salisbury@canonical.com
Looks like the patch contains lots of coding-style issues. Could you fix what checkpatch.pl compains and resubmit?
Oops, sorry. Was working on a different machine for this, hence I didn't notice.
Daniel
At Fri, 13 Jul 2012 08:08:26 +0200, Daniel Mack wrote:
On 13.07.2012 07:58, Takashi Iwai wrote:
At Thu, 12 Jul 2012 15:19:47 +0200, Daniel Mack wrote:
The rework of the snd-usb endpoint logic moved the calls to snd_usb_set_interface() into the snd_usb_endpoint implemenation. This changed the order in which these calls are issued to the device, and thereby caused regressions for some webcams.
Fix this by moving the calls back to pcm.c for now to make it work again and use snd_usb_endpoint_activate() to really tear down all remaining URBs in the flight, consequently fixing another regression caused by USB packets on the wire after altsetting 0 has been selected.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-and-tested-by: Philipp Dreimann philipp@dreimann.net Reported-by: Joseph Salisbury joseph.salisbury@canonical.com
Looks like the patch contains lots of coding-style issues. Could you fix what checkpatch.pl compains and resubmit?
Oops, sorry. Was working on a different machine for this, hence I didn't notice.
Applied now. Thanks.
Takashi
Daniel Mack wrote:
In endpoint.c, bail out earlier in case the stream is stopped. ... @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb) urb->status == -ENODEV || /* device removed */ urb->status == -ECONNRESET || /* unlinked */ urb->status == -ESHUTDOWN || /* device disabled */
ep->chip->shutdown)) /* device disconnected */
ep->chip->shutdown) || /* device disconnected */!test_bit(EP_FLAG_RUNNING, &ep->flags))
Is this really needed? The URBs will be unlinked at the same time.
Regards, Clemens
Hi Clemens,
On 12.07.2012 16:29, Clemens Ladisch wrote:
Daniel Mack wrote:
In endpoint.c, bail out earlier in case the stream is stopped. ... @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb) urb->status == -ENODEV || /* device removed */ urb->status == -ECONNRESET || /* unlinked */ urb->status == -ESHUTDOWN || /* device disabled */
ep->chip->shutdown)) /* device disconnected */
ep->chip->shutdown) || /* device disconnected */!test_bit(EP_FLAG_RUNNING, &ep->flags))Is this really needed? The URBs will be unlinked at the same time.
This just brings the code in sync with what we had before. If URBs are just unlinked but not killed, they will return with data payload, and in case of implicit feedback streams, the retire code could issue new output packets. Might be I overlooked a reason why that could never happen, but even if I did, the check here doesn't hurt, right?
Daniel
Daniel Mack wrote:
On 12.07.2012 16:29, Clemens Ladisch wrote:
Daniel Mack wrote:
In endpoint.c, bail out earlier in case the stream is stopped. ... @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb) urb->status == -ENODEV || /* device removed */ urb->status == -ECONNRESET || /* unlinked */ urb->status == -ESHUTDOWN || /* device disabled */
ep->chip->shutdown)) /* device disconnected */
ep->chip->shutdown) || /* device disconnected */!test_bit(EP_FLAG_RUNNING, &ep->flags))Is this really needed? The URBs will be unlinked at the same time.
This just brings the code in sync with what we had before. If URBs are just unlinked but not killed, they will return with data payload, and in case of implicit feedback streams, the retire code could issue new output packets.
And is that bad?
The EP_FLAG_RUNNING bit is cleared immediately before the URBs are unlinked, and some URB could have been completed regularly immediately before that. So if there is any case where the additional test_bit() call prevents submitting a new playback packet, there is another similar case where you've just got such a playback packet anyway.
In other words: if that test_bit() call were necessary to protect against something bad, it wouldn't be sufficient.
The driver ensures that (implicit feedback) playback URBs are unlinked only after the corresponding capture URBs have been completely killed, doesn't it?
Regards, Clemens
On 12.07.2012 17:20, Clemens Ladisch wrote:
Daniel Mack wrote:
On 12.07.2012 16:29, Clemens Ladisch wrote:
Daniel Mack wrote:
In endpoint.c, bail out earlier in case the stream is stopped. ... @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb) urb->status == -ENODEV || /* device removed */ urb->status == -ECONNRESET || /* unlinked */ urb->status == -ESHUTDOWN || /* device disabled */
ep->chip->shutdown)) /* device disconnected */
ep->chip->shutdown) || /* device disconnected */!test_bit(EP_FLAG_RUNNING, &ep->flags))Is this really needed? The URBs will be unlinked at the same time.
This just brings the code in sync with what we had before. If URBs are just unlinked but not killed, they will return with data payload, and in case of implicit feedback streams, the retire code could issue new output packets.
And is that bad?
The EP_FLAG_RUNNING bit is cleared immediately before the URBs are unlinked, and some URB could have been completed regularly immediately before that. So if there is any case where the additional test_bit() call prevents submitting a new playback packet, there is another similar case where you've just got such a playback packet anyway.
In other words: if that test_bit() call were necessary to protect against something bad, it wouldn't be sufficient.
The driver ensures that (implicit feedback) playback URBs are unlinked only after the corresponding capture URBs have been completely killed, doesn't it?
You're right. Especially because sending out packets in implicit feedback mode happens from queue_pending_output_urbs() which has an extra chip.
So we can drop the whole patch then. Thanks for the review!
Daniel
participants (3)
-
Clemens Ladisch -
Daniel Mack -
Takashi Iwai