[PATCH 0/9] ALSA: usb-audio: More fixes / improvements on PCM
Hi, this is a patch set for USB-audio to fix and improve the PCM stream handling. The primary target is the enhancement of the low-latency playback that was introduced recently for dealing with the small period size (that is used typically by JACK). There are a few other fixes that are relevant with it. The current patches are found in topic/usb-audio branch of sound.git tree, too. Takashi === Takashi Iwai (9): ALSA: usb-audio: Restrict rates for the shared clocks ALSA: usb-audio: Fix possible race at sync of urb completions ALSA: usb-audio: Rename early_playback_start flag with lowlatency_playback ALSA: usb-audio: Disable low-latency playback for free-wheel mode ALSA: usb-audio: Disable low-latency mode for implicit feedback sync ALSA: usb-audio: Check available frames for the next packet size ALSA: usb-audio: Add spinlock to stop_urbs() ALSA: usb-audio: Improved lowlatency playback support ALSA: usb-audio: Avoid killing in-flight URBs during draining sound/usb/card.h | 11 ++- sound/usb/endpoint.c | 230 +++++++++++++++++++++++++++++++------------ sound/usb/endpoint.h | 13 ++- sound/usb/pcm.c | 157 ++++++++++++++++++++++------- 4 files changed, 305 insertions(+), 106 deletions(-) -- 2.26.2
When a single clock source is shared among several endpoints, we have to keep the same rate on all active endpoints as long as the clock is being used. For dealing with such a case, this patch adds one more check in the hw params constraint for the rate to take the shared clocks into account. The current rate is evaluated from the endpoint list that applies the same clock source. BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1190418 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 21 +++++++++++++++++++++ sound/usb/endpoint.h | 1 + sound/usb/pcm.c | 9 +++++++++ 4 files changed, 32 insertions(+) diff --git a/sound/usb/card.h b/sound/usb/card.h index 5b19901f305a..3329ce710cb9 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -136,6 +136,7 @@ struct snd_usb_endpoint { unsigned int cur_period_frames; unsigned int cur_period_bytes; unsigned int cur_buffer_periods; + unsigned char cur_clock; spinlock_t lock; struct list_head list; diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 533919a28856..29c4865966f5 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -722,6 +722,7 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip, ep->cur_period_frames = params_period_size(params); ep->cur_period_bytes = ep->cur_period_frames * ep->cur_frame_bytes; ep->cur_buffer_periods = params_periods(params); + ep->cur_clock = fp->clock; if (ep->type == SND_USB_ENDPOINT_TYPE_SYNC) endpoint_set_syncinterval(chip, ep); @@ -833,6 +834,7 @@ void snd_usb_endpoint_close(struct snd_usb_audio *chip, ep->altsetting = 0; ep->cur_audiofmt = NULL; ep->cur_rate = 0; + ep->cur_clock = 0; ep->iface_ref = NULL; usb_audio_dbg(chip, "EP 0x%x closed\n", ep->ep_num); } @@ -1340,6 +1342,25 @@ int snd_usb_endpoint_configure(struct snd_usb_audio *chip, return err; } +/* get the current rate set to the given clock by any endpoint */ +int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int clock) +{ + struct snd_usb_endpoint *ep; + int rate = 0; + + if (!clock) + return 0; + mutex_lock(&chip->mutex); + list_for_each_entry(ep, &chip->ep_list, list) { + if (ep->cur_clock == clock && ep->cur_rate) { + rate = ep->cur_rate; + break; + } + } + mutex_unlock(&chip->mutex); + return rate; +} + /** * snd_usb_endpoint_start: start an snd_usb_endpoint * diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index a668f675b52b..a1099ec37e1c 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -19,6 +19,7 @@ void snd_usb_endpoint_close(struct snd_usb_audio *chip, struct snd_usb_endpoint *ep); int snd_usb_endpoint_configure(struct snd_usb_audio *chip, struct snd_usb_endpoint *ep); +int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int clock); bool snd_usb_endpoint_compatible(struct snd_usb_audio *chip, struct snd_usb_endpoint *ep, diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 5dc9266180e3..19392117de9e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -734,6 +734,7 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { struct snd_usb_substream *subs = rule->private; + struct snd_usb_audio *chip = subs->stream->chip; const struct audioformat *fp; struct snd_interval *it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); unsigned int rmin, rmax, r; @@ -745,6 +746,14 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params, list_for_each_entry(fp, &subs->fmt_list, list) { if (!hw_check_valid_format(subs, params, fp)) continue; + r = snd_usb_endpoint_get_clock_rate(chip, fp->clock); + if (r > 0) { + if (!snd_interval_test(it, r)) + continue; + rmin = min(rmin, r); + rmax = max(rmax, r); + continue; + } if (fp->rate_table && fp->nr_rates) { for (i = 0; i < fp->nr_rates; i++) { r = fp->rate_table[i]; -- 2.26.2
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB. For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */ diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) } set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } } @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep); + atomic_dec(&ep->submitted_urbs); /* decrement at last */ return; } @@ -513,6 +515,7 @@ static void snd_complete_urb(struct urb *urb) exit_clear: clear_bit(ctx->index, &ep->active_mask); + atomic_dec(&ep->submitted_urbs); } /* @@ -596,6 +599,7 @@ int snd_usb_add_endpoint(struct snd_usb_audio *chip, int ep_num, int type) ep->type = type; ep->ep_num = ep_num; INIT_LIST_HEAD(&ep->ready_playback_urbs); + atomic_set(&ep->submitted_urbs, 0); is_playback = ((ep_num & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT); ep_num &= USB_ENDPOINT_NUMBER_MASK; @@ -861,7 +865,7 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) return 0; do { - alive = bitmap_weight(&ep->active_mask, ep->nurbs); + alive = atomic_read(&ep->submitted_urbs); if (!alive) break; @@ -1441,6 +1445,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) goto __error; } set_bit(i, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } usb_audio_dbg(ep->chip, "%d URBs submitted for EP 0x%x\n", -- 2.26.2
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
return; }
@@ -513,6 +515,7 @@ static void snd_complete_urb(struct urb *urb)
exit_clear: clear_bit(ctx->index, &ep->active_mask);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); }
/* @@ -596,6 +599,7 @@ int snd_usb_add_endpoint(struct snd_usb_audio *chip, int ep_num, int type) ep->type = type; ep->ep_num = ep_num; INIT_LIST_HEAD(&ep->ready_playback_urbs); + atomic_set(&ep->submitted_urbs, 0);
is_playback = ((ep_num & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT); ep_num &= USB_ENDPOINT_NUMBER_MASK; @@ -861,7 +865,7 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) return 0;
do { - alive = bitmap_weight(&ep->active_mask, ep->nurbs); + alive = atomic_read(&ep->submitted_urbs); if (!alive) break;
@@ -1441,6 +1445,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) goto __error; } set_bit(i, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); }
usb_audio_dbg(ep->chip, "%d URBs submitted for EP 0x%x\n", -- 2.26.2
On Fri, 18 Apr 2025 12:35:32 +0200, Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention. Takashi
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer. list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb(); And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement. [1] https://lore.kernel.org/lkml/CABXGCsOposU1A_HavA_jmtkJMKhDZgh5m1b_YJK1Es5wyE...
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...? Takashi
[1] https://lore.kernel.org/lkml/CABXGCsOposU1A_HavA_jmtkJMKhDZgh5m1b_YJK1Es5wyE...
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well. Hillf
On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote:
USB-audio driver tries to sync with the clear of all pending URBs in wait_clear_urbs(), and it waits for all bits in active_mask getting cleared. This works fine for the normal operations, but when a stream is managed in the implicit feedback mode, there is still a very thin race window: namely, in snd_complete_usb(), the active_mask bit for the current URB is once cleared before re-submitted in queue_pending_output_urbs(). If wait_clear_urbs() is called during that period, it may pass the test and go forward even though there may be a still pending URB.
For covering it, this patch adds a new counter to each endpoint to keep the number of in-flight URBs, and changes wait_clear_urbs() checking this number instead. The counter is decremented at the end of URB complete, hence the reference is kept as long as the URB complete is in process.
Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 3329ce710cb9..746a765b2437 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,6 +97,7 @@ struct snd_usb_endpoint { unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ + atomic_t submitted_urbs; /* currently submitted urbs */ char *syncbuf; /* sync buffer for all sync URBs */ dma_addr_t sync_dma; /* DMA address of syncbuf */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 29c4865966f5..06241568abf7 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) }
set_bit(ctx->index, &ep->active_mask); + atomic_inc(&ep->submitted_urbs); } }
@@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) clear_bit(ctx->index, &ep->active_mask); spin_unlock_irqrestore(&ep->lock, flags); queue_pending_output_urbs(ep);
smp_mb();
+ atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
Still don't get it. Which reads and writes are you trying to solve? And more importantly, does it actually help at all? If yes, I'd happily take the patch, of course. thanks, Takashi
On Sun, 20 Apr 2025 09:49:46 +0200 Takashi Iwai wrote:
On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote:
On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote: > USB-audio driver tries to sync with the clear of all pending URBs in > wait_clear_urbs(), and it waits for all bits in active_mask getting > cleared. This works fine for the normal operations, but when a stream > is managed in the implicit feedback mode, there is still a very thin > race window: namely, in snd_complete_usb(), the active_mask bit for > the current URB is once cleared before re-submitted in > queue_pending_output_urbs(). If wait_clear_urbs() is called during > that period, it may pass the test and go forward even though there may > be a still pending URB. > > For covering it, this patch adds a new counter to each endpoint to > keep the number of in-flight URBs, and changes wait_clear_urbs() > checking this number instead. The counter is decremented at the end > of URB complete, hence the reference is kept as long as the URB > complete is in process. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/usb/card.h | 1 + > sound/usb/endpoint.c | 7 ++++++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 3329ce710cb9..746a765b2437 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -97,6 +97,7 @@ struct snd_usb_endpoint { > unsigned int nominal_queue_size; /* total buffer sizes in URBs */ > unsigned long active_mask; /* bitmask of active urbs */ > unsigned long unlink_mask; /* bitmask of unlinked urbs */ > + atomic_t submitted_urbs; /* currently submitted urbs */ > char *syncbuf; /* sync buffer for all sync URBs */ > dma_addr_t sync_dma; /* DMA address of syncbuf */ > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index 29c4865966f5..06241568abf7 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) > } > > set_bit(ctx->index, &ep->active_mask); > + atomic_inc(&ep->submitted_urbs); > } > } > > @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) > clear_bit(ctx->index, &ep->active_mask); > spin_unlock_irqrestore(&ep->lock, flags); > queue_pending_output_urbs(ep);
smp_mb();
> + atomic_dec(&ep->submitted_urbs); /* decrement at last */
Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
Still don't get it. Which reads and writes are you trying to solve?
See a simpler UAF case IIUIC. cpu1 cpu2 atomic_dec(&ep->submitted_urbs); if (!atomic_read(&ep->submitted_urbs)) kfree(ep); a = ep->xxx; // UAF
And more importantly, does it actually help at all? If yes, I'd
Yes and No. Yes because of the fix mentioned in ipc wrt mb.
happily take the patch, of course.
Provided a repro, a fix like the following diff should have been posted. Hillf --- x/sound/usb/endpoint.c +++ y/sound/usb/endpoint.c @@ -569,8 +569,11 @@ static void snd_complete_urb(struct urb snd_usb_queue_pending_output_urbs(ep, false); /* decrement at last, and check xrun */ if (atomic_dec_and_test(&ep->submitted_urbs) && - !snd_usb_endpoint_implicit_feedback_sink(ep)) + !snd_usb_endpoint_implicit_feedback_sink(ep)) { notify_xrun(ep); + smp_mb(); + atomic_dec(&ep->submitted_urbs); + } return; } @@ -602,7 +605,8 @@ static void snd_complete_urb(struct urb exit_clear: clear_bit(ctx->index, &ep->active_mask); - atomic_dec(&ep->submitted_urbs); + if (atomic_dec_and_test(&ep->submitted_urbs)) + atomic_dec(&ep->submitted_urbs); } /* @@ -1004,17 +1008,17 @@ static int wait_clear_urbs(struct snd_us do { alive = atomic_read(&ep->submitted_urbs); - if (!alive) - break; + if (alive < 0) + goto update; schedule_timeout_uninterruptible(1); } while (time_before(jiffies, end_time)); - if (alive) - usb_audio_err(ep->chip, + usb_audio_err(ep->chip, "timeout: still %d active urbs on EP #%x\n", alive, ep->ep_num); +update: if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) { ep->sync_sink = NULL; snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
On Mon, 21 Apr 2025 07:18:31 +0200, Hillf Danton wrote:
On Sun, 20 Apr 2025 09:49:46 +0200 Takashi Iwai wrote:
On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote: > On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote: > > USB-audio driver tries to sync with the clear of all pending URBs in > > wait_clear_urbs(), and it waits for all bits in active_mask getting > > cleared. This works fine for the normal operations, but when a stream > > is managed in the implicit feedback mode, there is still a very thin > > race window: namely, in snd_complete_usb(), the active_mask bit for > > the current URB is once cleared before re-submitted in > > queue_pending_output_urbs(). If wait_clear_urbs() is called during > > that period, it may pass the test and go forward even though there may > > be a still pending URB. > > > > For covering it, this patch adds a new counter to each endpoint to > > keep the number of in-flight URBs, and changes wait_clear_urbs() > > checking this number instead. The counter is decremented at the end > > of URB complete, hence the reference is kept as long as the URB > > complete is in process. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/usb/card.h | 1 + > > sound/usb/endpoint.c | 7 ++++++- > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/sound/usb/card.h b/sound/usb/card.h > > index 3329ce710cb9..746a765b2437 100644 > > --- a/sound/usb/card.h > > +++ b/sound/usb/card.h > > @@ -97,6 +97,7 @@ struct snd_usb_endpoint { > > unsigned int nominal_queue_size; /* total buffer sizes in URBs */ > > unsigned long active_mask; /* bitmask of active urbs */ > > unsigned long unlink_mask; /* bitmask of unlinked urbs */ > > + atomic_t submitted_urbs; /* currently submitted urbs */ > > char *syncbuf; /* sync buffer for all sync URBs */ > > dma_addr_t sync_dma; /* DMA address of syncbuf */ > > > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > > index 29c4865966f5..06241568abf7 100644 > > --- a/sound/usb/endpoint.c > > +++ b/sound/usb/endpoint.c > > @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) > > } > > > > set_bit(ctx->index, &ep->active_mask); > > + atomic_inc(&ep->submitted_urbs); > > } > > } > > > > @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) > > clear_bit(ctx->index, &ep->active_mask); > > spin_unlock_irqrestore(&ep->lock, flags); > > queue_pending_output_urbs(ep); > > smp_mb(); > > > + atomic_dec(&ep->submitted_urbs); /* decrement at last */ > > Does it match the comment to add a mb?
How...? I don't understand your intention.
In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
Still don't get it. Which reads and writes are you trying to solve?
See a simpler UAF case IIUIC.
cpu1 cpu2 atomic_dec(&ep->submitted_urbs);
if (!atomic_read(&ep->submitted_urbs)) kfree(ep);
a = ep->xxx; // UAF
That's what I don't get it. Which place does this UAF happen, more specifically? In the whole conversations, the context is missing, and you provided only a snippet of the patch.
And more importantly, does it actually help at all? If yes, I'd
Yes and No. Yes because of the fix mentioned in ipc wrt mb.
happily take the patch, of course.
Provided a repro, a fix like the following diff should have been posted.
No idea about repro and the report itself, so hard to judge. Takashi
Hillf
--- x/sound/usb/endpoint.c +++ y/sound/usb/endpoint.c @@ -569,8 +569,11 @@ static void snd_complete_urb(struct urb snd_usb_queue_pending_output_urbs(ep, false); /* decrement at last, and check xrun */ if (atomic_dec_and_test(&ep->submitted_urbs) && - !snd_usb_endpoint_implicit_feedback_sink(ep)) + !snd_usb_endpoint_implicit_feedback_sink(ep)) { notify_xrun(ep); + smp_mb(); + atomic_dec(&ep->submitted_urbs); + } return; }
@@ -602,7 +605,8 @@ static void snd_complete_urb(struct urb
exit_clear: clear_bit(ctx->index, &ep->active_mask); - atomic_dec(&ep->submitted_urbs); + if (atomic_dec_and_test(&ep->submitted_urbs)) + atomic_dec(&ep->submitted_urbs); }
/* @@ -1004,17 +1008,17 @@ static int wait_clear_urbs(struct snd_us
do { alive = atomic_read(&ep->submitted_urbs); - if (!alive) - break; + if (alive < 0) + goto update;
schedule_timeout_uninterruptible(1); } while (time_before(jiffies, end_time));
- if (alive) - usb_audio_err(ep->chip, + usb_audio_err(ep->chip, "timeout: still %d active urbs on EP #%x\n", alive, ep->ep_num);
+update: if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) { ep->sync_sink = NULL; snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
On Mon, 21 Apr 2025 09:23:45 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 07:18:31 +0200, Hillf Danton wrote:
On Sun, 20 Apr 2025 09:49:46 +0200 Takashi Iwai wrote:
On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote:
On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote: > On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote: > > On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote: > > > USB-audio driver tries to sync with the clear of all pending URBs in > > > wait_clear_urbs(), and it waits for all bits in active_mask getting > > > cleared. This works fine for the normal operations, but when a stream > > > is managed in the implicit feedback mode, there is still a very thin > > > race window: namely, in snd_complete_usb(), the active_mask bit for > > > the current URB is once cleared before re-submitted in > > > queue_pending_output_urbs(). If wait_clear_urbs() is called during > > > that period, it may pass the test and go forward even though there may > > > be a still pending URB. > > > > > > For covering it, this patch adds a new counter to each endpoint to > > > keep the number of in-flight URBs, and changes wait_clear_urbs() > > > checking this number instead. The counter is decremented at the end > > > of URB complete, hence the reference is kept as long as the URB > > > complete is in process. > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > sound/usb/card.h | 1 + > > > sound/usb/endpoint.c | 7 ++++++- > > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/sound/usb/card.h b/sound/usb/card.h > > > index 3329ce710cb9..746a765b2437 100644 > > > --- a/sound/usb/card.h > > > +++ b/sound/usb/card.h > > > @@ -97,6 +97,7 @@ struct snd_usb_endpoint { > > > unsigned int nominal_queue_size; /* total buffer sizes in URBs */ > > > unsigned long active_mask; /* bitmask of active urbs */ > > > unsigned long unlink_mask; /* bitmask of unlinked urbs */ > > > + atomic_t submitted_urbs; /* currently submitted urbs */ > > > char *syncbuf; /* sync buffer for all sync URBs */ > > > dma_addr_t sync_dma; /* DMA address of syncbuf */ > > > > > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > > > index 29c4865966f5..06241568abf7 100644 > > > --- a/sound/usb/endpoint.c > > > +++ b/sound/usb/endpoint.c > > > @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) > > > } > > > > > > set_bit(ctx->index, &ep->active_mask); > > > + atomic_inc(&ep->submitted_urbs); > > > } > > > } > > > > > > @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) > > > clear_bit(ctx->index, &ep->active_mask); > > > spin_unlock_irqrestore(&ep->lock, flags); > > > queue_pending_output_urbs(ep); > > > > smp_mb(); > > > > > + atomic_dec(&ep->submitted_urbs); /* decrement at last */ > > > > Does it match the comment to add a mb? > > How...? I don't understand your intention. > In addition to the UAF report [1], I saw a customer report of list corruption of linux-6.1.99 on arm64 this week without reproducer.
list corruption list_add_tail(); push_back_to_ready_list(); snd_complete_urb();
And after another look at this patch I wonder if the race can not be erased without the certainty that ep will be no longer used after the atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
Still don't get it. Which reads and writes are you trying to solve?
See a simpler UAF case IIUIC.
cpu1 cpu2 atomic_dec(&ep->submitted_urbs);
if (!atomic_read(&ep->submitted_urbs)) kfree(ep);
a = ep->xxx; // UAF
That's what I don't get it. Which place does this UAF happen, more specifically? In the whole conversations, the context is missing, and you provided only a snippet of the patch.
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF. Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example? Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please. Hillf
On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
On Mon, 21 Apr 2025 09:23:45 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 07:18:31 +0200, Hillf Danton wrote:
On Sun, 20 Apr 2025 09:49:46 +0200 Takashi Iwai wrote:
On Sat, 19 Apr 2025 10:04:08 +0200, Hillf Danton wrote:
On Sat, 19 Apr 2025 08:50:46 +0200 Takashi Iwai wrote:
On Fri, 18 Apr 2025 16:45:17 +0200, Hillf Danton wrote: > On Fri, 18 Apr 2025 13:08:32 +0200 Takashi Iwai wrote: > > On Fri, 18 Apr 2025 12:35:32 +0200 Hillf Danton wrote: > > > On Wed, 29 Sep 2021 10:08:37 +0200 Takashi Iwai wrote: > > > > USB-audio driver tries to sync with the clear of all pending URBs in > > > > wait_clear_urbs(), and it waits for all bits in active_mask getting > > > > cleared. This works fine for the normal operations, but when a stream > > > > is managed in the implicit feedback mode, there is still a very thin > > > > race window: namely, in snd_complete_usb(), the active_mask bit for > > > > the current URB is once cleared before re-submitted in > > > > queue_pending_output_urbs(). If wait_clear_urbs() is called during > > > > that period, it may pass the test and go forward even though there may > > > > be a still pending URB. > > > > > > > > For covering it, this patch adds a new counter to each endpoint to > > > > keep the number of in-flight URBs, and changes wait_clear_urbs() > > > > checking this number instead. The counter is decremented at the end > > > > of URB complete, hence the reference is kept as long as the URB > > > > complete is in process. > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > --- > > > > sound/usb/card.h | 1 + > > > > sound/usb/endpoint.c | 7 ++++++- > > > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/sound/usb/card.h b/sound/usb/card.h > > > > index 3329ce710cb9..746a765b2437 100644 > > > > --- a/sound/usb/card.h > > > > +++ b/sound/usb/card.h > > > > @@ -97,6 +97,7 @@ struct snd_usb_endpoint { > > > > unsigned int nominal_queue_size; /* total buffer sizes in URBs */ > > > > unsigned long active_mask; /* bitmask of active urbs */ > > > > unsigned long unlink_mask; /* bitmask of unlinked urbs */ > > > > + atomic_t submitted_urbs; /* currently submitted urbs */ > > > > char *syncbuf; /* sync buffer for all sync URBs */ > > > > dma_addr_t sync_dma; /* DMA address of syncbuf */ > > > > > > > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > > > > index 29c4865966f5..06241568abf7 100644 > > > > --- a/sound/usb/endpoint.c > > > > +++ b/sound/usb/endpoint.c > > > > @@ -451,6 +451,7 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) > > > > } > > > > > > > > set_bit(ctx->index, &ep->active_mask); > > > > + atomic_inc(&ep->submitted_urbs); > > > > } > > > > } > > > > > > > > @@ -488,6 +489,7 @@ static void snd_complete_urb(struct urb *urb) > > > > clear_bit(ctx->index, &ep->active_mask); > > > > spin_unlock_irqrestore(&ep->lock, flags); > > > > queue_pending_output_urbs(ep); > > > > > > smp_mb(); > > > > > > > + atomic_dec(&ep->submitted_urbs); /* decrement at last */ > > > > > > Does it match the comment to add a mb? > > > > How...? I don't understand your intention. > > > In addition to the UAF report [1], I saw a customer report of list > corruption of linux-6.1.99 on arm64 this week without reproducer. > > list corruption > list_add_tail(); > push_back_to_ready_list(); > snd_complete_urb(); > > And after another look at this patch I wonder if the race can not be > erased without the certainty that ep will be no longer used after the > atomic decrement.
But why adding more barrier if you perform the atomic op...?
Because atomic op != ordering, see 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") for detail for example. And c5b2cbdbdac5 ("ipc/mqueue.c: update/document memory barriers") as well.
Still don't get it. Which reads and writes are you trying to solve?
See a simpler UAF case IIUIC.
cpu1 cpu2 atomic_dec(&ep->submitted_urbs);
if (!atomic_read(&ep->submitted_urbs)) kfree(ep);
a = ep->xxx; // UAF
That's what I don't get it. Which place does this UAF happen, more specifically? In the whole conversations, the context is missing, and you provided only a snippet of the patch.
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF.
Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example?
Before the patch, wait_clear_urbs() might return earlier than actually all pending eps are finished, so it can be UAF.
Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please.
Well, your concern about the missing barrier -- that would wait_clear_urbs() missing the refcount decrement, hence it would be rather to make the return delayed. So it shouldn't lead to further UAF, but at most it might lead to an unnecessary delay. That said, I'm willing to take a fix even for a theoretical issue if it clarifies what it really fixes. But scratching a random surface isn't what we want. Takashi
On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF.
Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example?
Before the patch, wait_clear_urbs() might return earlier than actually all pending eps are finished, so it can be UAF.
Got it.
Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please.
Well, your concern about the missing barrier -- that would wait_clear_urbs() missing the refcount decrement, hence it would be rather to make the return delayed. So it shouldn't lead to further UAF, but at most it might lead to an unnecessary delay.
That said, I'm willing to take a fix even for a theoretical issue if it clarifies what it really fixes. But scratching a random surface isn't what we want.
Thank you for shedding light on the race, given a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb() use the urb routines instead to close the race. Note the one second timeout is also cut. Hillf --- x/sound/usb/endpoint.c +++ y/sound/usb/endpoint.c @@ -996,24 +996,14 @@ void snd_usb_endpoint_suspend(struct snd */ static int wait_clear_urbs(struct snd_usb_endpoint *ep) { - unsigned long end_time = jiffies + msecs_to_jiffies(1000); - int alive; - if (atomic_read(&ep->state) != EP_STATE_STOPPING) return 0; - do { - alive = atomic_read(&ep->submitted_urbs); - if (!alive) - break; - - schedule_timeout_uninterruptible(1); - } while (time_before(jiffies, end_time)); + for (int i = 0; i < ep->nurbs; i++) { + struct snd_urb_ctx *u = &ep->urb[i]; - if (alive) - usb_audio_err(ep->chip, - "timeout: still %d active urbs on EP #%x\n", - alive, ep->ep_num); + usb_kill_urb(u->urb); + } if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) { ep->sync_sink = NULL;
On Tue, 22 Apr 2025 01:38:59 +0200, Hillf Danton wrote:
On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF.
Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example?
Before the patch, wait_clear_urbs() might return earlier than actually all pending eps are finished, so it can be UAF.
Got it.
Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please.
Well, your concern about the missing barrier -- that would wait_clear_urbs() missing the refcount decrement, hence it would be rather to make the return delayed. So it shouldn't lead to further UAF, but at most it might lead to an unnecessary delay.
That said, I'm willing to take a fix even for a theoretical issue if it clarifies what it really fixes. But scratching a random surface isn't what we want.
Thank you for shedding light on the race, given a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb()
use the urb routines instead to close the race.
I'm afraid that it can break things as of this form; the stopped stream might be restarted without reinitializing URBs. That is, this isn't called only from disconnect or close, but also just for stopping the stream in the middle, too. Takashi
Note the one second timeout is also cut.
Hillf
--- x/sound/usb/endpoint.c +++ y/sound/usb/endpoint.c @@ -996,24 +996,14 @@ void snd_usb_endpoint_suspend(struct snd */ static int wait_clear_urbs(struct snd_usb_endpoint *ep) { - unsigned long end_time = jiffies + msecs_to_jiffies(1000); - int alive; - if (atomic_read(&ep->state) != EP_STATE_STOPPING) return 0;
- do { - alive = atomic_read(&ep->submitted_urbs); - if (!alive) - break; - - schedule_timeout_uninterruptible(1); - } while (time_before(jiffies, end_time)); + for (int i = 0; i < ep->nurbs; i++) { + struct snd_urb_ctx *u = &ep->urb[i];
- if (alive) - usb_audio_err(ep->chip, - "timeout: still %d active urbs on EP #%x\n", - alive, ep->ep_num); + usb_kill_urb(u->urb); + }
if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) { ep->sync_sink = NULL;
On Tue, 22 Apr 2025 09:03:20 +0200 Takashi Iwai wrote:
On Tue, 22 Apr 2025 01:38:59 +0200, Hillf Danton wrote:
On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF.
Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example?
Before the patch, wait_clear_urbs() might return earlier than actually all pending eps are finished, so it can be UAF.
Got it.
Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please.
Well, your concern about the missing barrier -- that would wait_clear_urbs() missing the refcount decrement, hence it would be rather to make the return delayed. So it shouldn't lead to further UAF, but at most it might lead to an unnecessary delay.
That said, I'm willing to take a fix even for a theoretical issue if it clarifies what it really fixes. But scratching a random surface isn't what we want.
Thank you for shedding light on the race, given a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb()
use the urb routines instead to close the race.
I'm afraid that it can break things as of this form; the stopped stream might be restarted without reinitializing URBs. That is, this isn't called only from disconnect or close, but also just for stopping the stream in the middle, too.
I'd like to test the change proposed locally, so please tippoint me to the test programs that could trigger the break. Hillf
On Tue, 22 Apr 2025 12:29:31 +0200, Hillf Danton wrote:
On Tue, 22 Apr 2025 09:03:20 +0200 Takashi Iwai wrote:
On Tue, 22 Apr 2025 01:38:59 +0200, Hillf Danton wrote:
On Mon, 21 Apr 2025 16:36:30 +0200 Takashi Iwai wrote:
On Mon, 21 Apr 2025 12:43:42 +0200, Hillf Danton wrote:
I misread "Which reads and writes are you trying to solve?" though I showed the read/write, but it is a bad case particulay with UAF.
Could you tell us what will happen if the race is not fixed? Could ep be freed with in-flight urbs for example?
Before the patch, wait_clear_urbs() might return earlier than actually all pending eps are finished, so it can be UAF.
Got it.
Is it still race if the wait loop in wait_clear_urbs() ends before the urb complete callbace completes, given the last sentence in your commit message? If nope, igore my noise please.
Well, your concern about the missing barrier -- that would wait_clear_urbs() missing the refcount decrement, hence it would be rather to make the return delayed. So it shouldn't lead to further UAF, but at most it might lead to an unnecessary delay.
That said, I'm willing to take a fix even for a theoretical issue if it clarifies what it really fixes. But scratching a random surface isn't what we want.
Thank you for shedding light on the race, given a) the mb in 26fbe9772b8c ("USB: core: Fix hang in usb_kill_urb by adding memory barriers") b) the urb complete callback is invoked in giveback, see __usb_hcd_giveback_urb()
use the urb routines instead to close the race.
I'm afraid that it can break things as of this form; the stopped stream might be restarted without reinitializing URBs. That is, this isn't called only from disconnect or close, but also just for stopping the stream in the middle, too.
I'd like to test the change proposed locally, so please tippoint me to the test programs that could trigger the break.
Well, you need to follow the code logic. The function isn't called only from release / disconnect, but also from the code pattern: trigger(STOP) -> prepare -> sync_stop and the URBs aren't released / re-initialized in this case. For the problem you raised, I suppose it's better to stick with your first approach with the manual barriers. But you'd need to describe exactly what it does and why it needed as a proper patch. thanks, Takashi
This is a preparation patch for the upcoming low-latency improvement changes. Rename early_playback_start flag with lowlatency_playback as it's more intuitive. The new flag is basically a reverse meaning. Along with the rename, factor out the code to set the flag to a function. This makes the complex condition checks simpler. Also, the same flag is introduced to snd_usb_endpoint, too, that is carried from the snd_usb_substream flag. Currently the endpoint flag isn't still referred, but will be used in later patches. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 3 ++- sound/usb/endpoint.c | 4 ++++ sound/usb/pcm.c | 29 ++++++++++++++++++++--------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/sound/usb/card.h b/sound/usb/card.h index 746a765b2437..a00caa1db37e 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -126,6 +126,7 @@ struct snd_usb_endpoint { int skip_packets; /* quirks for devices to ignore the first n packets in a stream */ bool implicit_fb_sync; /* syncs with implicit feedback */ + bool lowlatency_playback; /* low-latency playback mode */ bool need_setup; /* (re-)need for configure? */ /* for hw constraints */ @@ -190,7 +191,7 @@ struct snd_usb_substream { } dsd_dop; bool trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */ - bool early_playback_start; /* early start needed for playback? */ + bool lowlatency_playback; /* low-latency playback mode */ struct media_ctl *media_ctl; }; diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 06241568abf7..8e164d71d9ac 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -794,6 +794,10 @@ void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep, { ep->prepare_data_urb = prepare; ep->retire_data_urb = retire; + if (data_subs) + ep->lowlatency_playback = data_subs->lowlatency_playback; + else + ep->lowlatency_playback = false; WRITE_ONCE(ep->data_subs, data_subs); } diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 19392117de9e..4dd7f1c9e2af 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -581,6 +581,22 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) return 0; } +/* check whether early start is needed for playback stream */ +static int lowlatency_playback_available(struct snd_usb_substream *subs) +{ + struct snd_usb_audio *chip = subs->stream->chip; + + if (subs->direction == SNDRV_PCM_STREAM_CAPTURE) + return false; + /* disabled via module option? */ + if (!chip->lowlatency) + return false; + /* too short periods? */ + if (subs->data_endpoint->nominal_queue_size >= subs->buffer_bytes) + return false; + return true; +} + /* * prepare callback * @@ -614,13 +630,8 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) subs->period_elapsed_pending = 0; runtime->delay = 0; - /* check whether early start is needed for playback stream */ - subs->early_playback_start = - subs->direction == SNDRV_PCM_STREAM_PLAYBACK && - (!chip->lowlatency || - (subs->data_endpoint->nominal_queue_size >= subs->buffer_bytes)); - - if (subs->early_playback_start) + subs->lowlatency_playback = lowlatency_playback_available(subs); + if (!subs->lowlatency_playback) ret = start_endpoints(subs); unlock: @@ -1412,7 +1423,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, subs->trigger_tstamp_pending_update = false; } - if (period_elapsed && !subs->running && !subs->early_playback_start) { + if (period_elapsed && !subs->running && subs->lowlatency_playback) { subs->period_elapsed_pending = 1; period_elapsed = 0; } @@ -1466,7 +1477,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea prepare_playback_urb, retire_playback_urb, subs); - if (!subs->early_playback_start && + if (subs->lowlatency_playback && cmd == SNDRV_PCM_TRIGGER_START) { err = start_endpoints(subs); if (err < 0) { -- 2.26.2
The free-wheel stream operation like dmix may not update the appl_ptr appropriately, and it doesn't fit with the low-latency playback mode. Disable the low-latency playback operation when the stream is set up in such a mode. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/pcm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 4dd7f1c9e2af..84b03a32ee23 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -582,7 +582,8 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) } /* check whether early start is needed for playback stream */ -static int lowlatency_playback_available(struct snd_usb_substream *subs) +static int lowlatency_playback_available(struct snd_pcm_runtime *runtime, + struct snd_usb_substream *subs) { struct snd_usb_audio *chip = subs->stream->chip; @@ -591,6 +592,9 @@ static int lowlatency_playback_available(struct snd_usb_substream *subs) /* disabled via module option? */ if (!chip->lowlatency) return false; + /* free-wheeling mode? (e.g. dmix) */ + if (runtime->stop_threshold > runtime->buffer_size) + return false; /* too short periods? */ if (subs->data_endpoint->nominal_queue_size >= subs->buffer_bytes) return false; @@ -630,7 +634,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) subs->period_elapsed_pending = 0; runtime->delay = 0; - subs->lowlatency_playback = lowlatency_playback_available(subs); + subs->lowlatency_playback = lowlatency_playback_available(runtime, subs); if (!subs->lowlatency_playback) ret = start_endpoints(subs); -- 2.26.2
When a playback stream runs in the implicit feedback mode, its operation is passive and won't start unless the capture packet is received. This behavior contradicts with the low-latency playback mode, and we should turn off lowlatency_playback flag accordingly. In theory, we may take the low-latency mode when the playback-first quirk is set, but it still conflicts with the later operation with the fixed packet numbers, so it's disabled all together for now. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/pcm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 84b03a32ee23..ec7eeb1b82b8 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -595,6 +595,9 @@ static int lowlatency_playback_available(struct snd_pcm_runtime *runtime, /* free-wheeling mode? (e.g. dmix) */ if (runtime->stop_threshold > runtime->buffer_size) return false; + /* implicit feedback mode has own operation mode */ + if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint)) + return false; /* too short periods? */ if (subs->data_endpoint->nominal_queue_size >= subs->buffer_bytes) return false; -- 2.26.2
This is yet more preparation for the upcoming changes. Extend snd_usb_endpoint_next_packet_size() to check the available frames and return -EAGAIN if the next packet size is equal or exceeds the given size. This will be needed for avoiding XRUN during the low latency operation. As of this patch, avail=0 is passed, i.e. the check is skipped and no behavior change. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/endpoint.c | 51 +++++++++++++++++++++++++++++++------------- sound/usb/endpoint.h | 3 ++- sound/usb/pcm.c | 2 +- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 8e164d71d9ac..1f757a7eeafe 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -148,18 +148,23 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep) * This won't be used for implicit feedback which takes the packet size * returned from the sync source */ -static int slave_next_packet_size(struct snd_usb_endpoint *ep) +static int slave_next_packet_size(struct snd_usb_endpoint *ep, + unsigned int avail) { unsigned long flags; + unsigned int phase; int ret; if (ep->fill_max) return ep->maxframesize; spin_lock_irqsave(&ep->lock, flags); - ep->phase = (ep->phase & 0xffff) - + (ep->freqm << ep->datainterval); - ret = min(ep->phase >> 16, ep->maxframesize); + phase = (ep->phase & 0xffff) + (ep->freqm << ep->datainterval); + ret = min(phase >> 16, ep->maxframesize); + if (avail && ret >= avail) + ret = -EAGAIN; + else + ep->phase = phase; spin_unlock_irqrestore(&ep->lock, flags); return ret; @@ -169,20 +174,25 @@ static int slave_next_packet_size(struct snd_usb_endpoint *ep) * Return the number of samples to be sent in the next packet * for adaptive and synchronous endpoints */ -static int next_packet_size(struct snd_usb_endpoint *ep) +static int next_packet_size(struct snd_usb_endpoint *ep, unsigned int avail) { + unsigned int sample_accum; int ret; if (ep->fill_max) return ep->maxframesize; - ep->sample_accum += ep->sample_rem; - if (ep->sample_accum >= ep->pps) { - ep->sample_accum -= ep->pps; + sample_accum += ep->sample_rem; + if (sample_accum >= ep->pps) { + sample_accum -= ep->pps; ret = ep->packsize[1]; } else { ret = ep->packsize[0]; } + if (avail && ret >= avail) + ret = -EAGAIN; + else + ep->sample_accum = sample_accum; return ret; } @@ -190,16 +200,27 @@ static int next_packet_size(struct snd_usb_endpoint *ep) /* * snd_usb_endpoint_next_packet_size: Return the number of samples to be sent * in the next packet + * + * If the size is equal or exceeds @avail, don't proceed but return -EAGAIN + * Exception: @avail = 0 for skipping the check. */ int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep, - struct snd_urb_ctx *ctx, int idx) + struct snd_urb_ctx *ctx, int idx, + unsigned int avail) { - if (ctx->packet_size[idx]) - return ctx->packet_size[idx]; - else if (ep->sync_source) - return slave_next_packet_size(ep); + unsigned int packet; + + packet = ctx->packet_size[idx]; + if (packet) { + if (avail && packet >= avail) + return -EAGAIN; + return packet; + } + + if (ep->sync_source) + return slave_next_packet_size(ep, avail); else - return next_packet_size(ep); + return next_packet_size(ep, avail); } static void call_retire_callback(struct snd_usb_endpoint *ep, @@ -263,7 +284,7 @@ static void prepare_silent_urb(struct snd_usb_endpoint *ep, unsigned int length; int counts; - counts = snd_usb_endpoint_next_packet_size(ep, ctx, i); + counts = snd_usb_endpoint_next_packet_size(ep, ctx, i, 0); length = counts * ep->stride; /* number of silent bytes */ offset = offs * ep->stride + extra * i; urb->iso_frame_desc[i].offset = offset; diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index a1099ec37e1c..1f1a72535a64 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -46,6 +46,7 @@ void snd_usb_endpoint_free_all(struct snd_usb_audio *chip); int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep); int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep, - struct snd_urb_ctx *ctx, int idx); + struct snd_urb_ctx *ctx, int idx, + unsigned int avail); #endif /* __USBAUDIO_ENDPOINT_H */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index ec7eeb1b82b8..8ad48c35c559 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1365,7 +1365,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, spin_lock_irqsave(&subs->lock, flags); subs->frame_limit += ep->max_urb_frames; for (i = 0; i < ctx->packets; i++) { - counts = snd_usb_endpoint_next_packet_size(ep, ctx, i); + counts = snd_usb_endpoint_next_packet_size(ep, ctx, i, 0); /* set up descriptor */ urb->iso_frame_desc[i].offset = frames * stride; urb->iso_frame_desc[i].length = counts * stride; -- 2.26.2
In theory, stop_urbs() may be called concurrently. Although we have the state check beforehand, it's safer to apply ep->lock during the critical list head manipulations. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/endpoint.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 1f757a7eeafe..c32022672319 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -927,6 +927,7 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep) static int stop_urbs(struct snd_usb_endpoint *ep, bool force) { unsigned int i; + unsigned long flags; if (!force && atomic_read(&ep->running)) return -EBUSY; @@ -934,9 +935,11 @@ static int stop_urbs(struct snd_usb_endpoint *ep, bool force) if (!ep_state_update(ep, EP_STATE_RUNNING, EP_STATE_STOPPING)) return 0; + spin_lock_irqsave(&ep->lock, flags); INIT_LIST_HEAD(&ep->ready_playback_urbs); ep->next_packet_head = 0; ep->next_packet_queued = 0; + spin_unlock_irqrestore(&ep->lock, flags); for (i = 0; i < ep->nurbs; i++) { if (test_bit(i, &ep->active_mask)) { -- 2.26.2
This is another attempt to improve further the handling of playback stream in the low latency mode. The latest workaround in commit 4267c5a8f313 ("ALSA: usb-audio: Work around for XRUN with low latency playback") revealed that submitting URBs forcibly in advance may trigger XRUN easily. In the classical mode, this problem was avoided by practically delaying the submission of the actual data with the pre-submissions of silent data before triggering the stream start. But that is exactly what we want to avoid. Now, in this patch, instead of the previous workaround, we take a similar approach as used in the implicit feedback mode. The URBs are queued at the PCM trigger start like before, but we check whether the buffer has been already filled enough before each submission, and stop queuing if the data overcomes the threshold. The remaining URBs are kept in the ready list, and they will be retrieved in the URB complete callback of other (already queued) URBs. In the complete callback, we try to fill the data and submit as much as possible again. When there is no more available in-flight URBs that may handle the pending data, we'll check in PCM ack callback and submit and process URBs there in addition. In this way, the amount of in-flight URBs may vary dynamically and flexibly depending on the available data without hitting XRUN. The following things are changed to achieve the behavior above: * The endpoint prepare callback is changed to return an error code; when there is no enough data available, it may return -EAGAIN. Currently only prepare_playback_urb() returns the error. The evaluation of the available data is a bit messy here; we can't check with snd_pcm_avail() at the point of prepare callback (as runtime->status->hwptr hasn't been updated yet), hence we manually estimate the appl_ptr and compare with the internal hwptr_done to calculate the available frames. * snd_usb_endpoint_start() doesn't submit full URBs if the prepare callback returns -EAGAIN, and puts the remaining URBs to the ready list for the later submission. * snd_complete_urb() treats the URBs in the low-latency mode similarly like the implicit feedback mode, and submissions are done in (now exported) snd_usb_queue_pending_output_urbs(). * snd_usb_queue_pending_output_urbs() again checks the error value from the prepare callback. If it's -EAGAIN for the normal stream (i.e. not implicit feedback mode), we push it back to the ready list again. * PCM ack callback is introduced for the playback stream, and it calls snd_usb_queue_pending_output_urbs() if there is no in-flight URB while the stream is running. This corresponds to the case where the system needs the appl_ptr update for re-submitting a new URB. * snd_usb_queue_pending_output_urbs() and the prepare EP callback receive in_stream_lock argument, which is a bool flag indicating the call path from PCM ack. It's needed for avoiding the deadlock of snd_pcm_period_elapsed() calls. * Set the new SNDRV_PCM_INFO_EXPLICIT_SYNC flag when the new low-latency mode is deployed. This assures catching each applptr update even in the mmap mode. Fixes: 4267c5a8f313 ("ALSA: usb-audio: Work around for XRUN with low latency playback") Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/card.h | 6 +- sound/usb/endpoint.c | 130 +++++++++++++++++++++++++++++-------------- sound/usb/endpoint.h | 7 ++- sound/usb/pcm.c | 102 ++++++++++++++++++++++++++------- 4 files changed, 177 insertions(+), 68 deletions(-) diff --git a/sound/usb/card.h b/sound/usb/card.h index a00caa1db37e..87f042d06ce0 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -74,8 +74,9 @@ struct snd_usb_endpoint { atomic_t state; /* running state */ - void (*prepare_data_urb) (struct snd_usb_substream *subs, - struct urb *urb); + int (*prepare_data_urb) (struct snd_usb_substream *subs, + struct urb *urb, + bool in_stream_lock); void (*retire_data_urb) (struct snd_usb_substream *subs, struct urb *urb); @@ -94,7 +95,6 @@ struct snd_usb_endpoint { struct list_head ready_playback_urbs; /* playback URB FIFO for implicit fb */ unsigned int nurbs; /* # urbs */ - unsigned int nominal_queue_size; /* total buffer sizes in URBs */ unsigned long active_mask; /* bitmask of active urbs */ unsigned long unlink_mask; /* bitmask of unlinked urbs */ atomic_t submitted_urbs; /* currently submitted urbs */ diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index c32022672319..0b336876e36d 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -307,8 +307,9 @@ static void prepare_silent_urb(struct snd_usb_endpoint *ep, /* * Prepare a PLAYBACK urb for submission to the bus. */ -static void prepare_outbound_urb(struct snd_usb_endpoint *ep, - struct snd_urb_ctx *ctx) +static int prepare_outbound_urb(struct snd_usb_endpoint *ep, + struct snd_urb_ctx *ctx, + bool in_stream_lock) { struct urb *urb = ctx->urb; unsigned char *cp = urb->transfer_buffer; @@ -320,9 +321,9 @@ static void prepare_outbound_urb(struct snd_usb_endpoint *ep, case SND_USB_ENDPOINT_TYPE_DATA: data_subs = READ_ONCE(ep->data_subs); if (data_subs && ep->prepare_data_urb) - ep->prepare_data_urb(data_subs, urb); - else /* no data provider, so send silence */ - prepare_silent_urb(ep, ctx); + return ep->prepare_data_urb(data_subs, urb, in_stream_lock); + /* no data provider, so send silence */ + prepare_silent_urb(ep, ctx); break; case SND_USB_ENDPOINT_TYPE_SYNC: @@ -351,13 +352,14 @@ static void prepare_outbound_urb(struct snd_usb_endpoint *ep, break; } + return 0; } /* * Prepare a CAPTURE or SYNC urb for submission to the bus. */ -static inline void prepare_inbound_urb(struct snd_usb_endpoint *ep, - struct snd_urb_ctx *urb_ctx) +static int prepare_inbound_urb(struct snd_usb_endpoint *ep, + struct snd_urb_ctx *urb_ctx) { int i, offs; struct urb *urb = urb_ctx->urb; @@ -382,6 +384,7 @@ static inline void prepare_inbound_urb(struct snd_usb_endpoint *ep, urb->iso_frame_desc[0].offset = 0; break; } + return 0; } /* notify an error as XRUN to the assigned PCM data substream */ @@ -417,6 +420,16 @@ next_packet_fifo_dequeue(struct snd_usb_endpoint *ep) return p; } +static void push_back_to_ready_list(struct snd_usb_endpoint *ep, + struct snd_urb_ctx *ctx) +{ + unsigned long flags; + + spin_lock_irqsave(&ep->lock, flags); + list_add_tail(&ctx->ready_list, &ep->ready_playback_urbs); + spin_unlock_irqrestore(&ep->lock, flags); +} + /* * Send output urbs that have been prepared previously. URBs are dequeued * from ep->ready_playback_urbs and in case there aren't any available @@ -427,12 +440,14 @@ next_packet_fifo_dequeue(struct snd_usb_endpoint *ep) * is that host controllers don't guarantee the order in which they return * inbound and outbound packets to their submitters. * - * This function is only used for implicit feedback endpoints. For endpoints - * driven by dedicated sync endpoints, URBs are immediately re-submitted - * from their completion handler. + * This function is used both for implicit feedback endpoints and in low- + * latency playback mode. */ -static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) +void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, + bool in_stream_lock) { + bool implicit_fb = snd_usb_endpoint_implicit_feedback_sink(ep); + while (ep_state_running(ep)) { unsigned long flags; @@ -441,14 +456,14 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) int err, i; spin_lock_irqsave(&ep->lock, flags); - if (ep->next_packet_queued > 0 && + if ((!implicit_fb || ep->next_packet_queued > 0) && !list_empty(&ep->ready_playback_urbs)) { /* take URB out of FIFO */ ctx = list_first_entry(&ep->ready_playback_urbs, struct snd_urb_ctx, ready_list); list_del_init(&ctx->ready_list); - - packet = next_packet_fifo_dequeue(ep); + if (implicit_fb) + packet = next_packet_fifo_dequeue(ep); } spin_unlock_irqrestore(&ep->lock, flags); @@ -456,11 +471,24 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) return; /* copy over the length information */ - for (i = 0; i < packet->packets; i++) - ctx->packet_size[i] = packet->packet_size[i]; + if (implicit_fb) { + for (i = 0; i < packet->packets; i++) + ctx->packet_size[i] = packet->packet_size[i]; + } /* call the data handler to fill in playback data */ - prepare_outbound_urb(ep, ctx); + err = prepare_outbound_urb(ep, ctx, in_stream_lock); + /* can be stopped during prepare callback */ + if (unlikely(!ep_state_running(ep))) + break; + if (err < 0) { + /* push back to ready list again for -EAGAIN */ + if (err == -EAGAIN) + push_back_to_ready_list(ep, ctx); + else + notify_xrun(ep); + return; + } err = usb_submit_urb(ctx->urb, GFP_ATOMIC); if (err < 0) { @@ -483,7 +511,6 @@ static void snd_complete_urb(struct urb *urb) { struct snd_urb_ctx *ctx = urb->context; struct snd_usb_endpoint *ep = ctx->ep; - unsigned long flags; int err; if (unlikely(urb->status == -ENOENT || /* unlinked */ @@ -504,17 +531,20 @@ static void snd_complete_urb(struct urb *urb) if (unlikely(!ep_state_running(ep))) goto exit_clear; - if (snd_usb_endpoint_implicit_feedback_sink(ep)) { - spin_lock_irqsave(&ep->lock, flags); - list_add_tail(&ctx->ready_list, &ep->ready_playback_urbs); + /* in low-latency and implicit-feedback modes, push back the + * URB to ready list at first, then process as much as possible + */ + if (ep->lowlatency_playback || + snd_usb_endpoint_implicit_feedback_sink(ep)) { + push_back_to_ready_list(ep, ctx); clear_bit(ctx->index, &ep->active_mask); - spin_unlock_irqrestore(&ep->lock, flags); - queue_pending_output_urbs(ep); + snd_usb_queue_pending_output_urbs(ep, false); atomic_dec(&ep->submitted_urbs); /* decrement at last */ return; } - prepare_outbound_urb(ep, ctx); + /* in non-lowlatency mode, no error handling for prepare */ + prepare_outbound_urb(ep, ctx, false); /* can be stopped during prepare callback */ if (unlikely(!ep_state_running(ep))) goto exit_clear; @@ -807,8 +837,9 @@ void snd_usb_endpoint_set_sync(struct snd_usb_audio *chip, * Pass NULL to deactivate each callback. */ void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep, - void (*prepare)(struct snd_usb_substream *subs, - struct urb *urb), + int (*prepare)(struct snd_usb_substream *subs, + struct urb *urb, + bool in_stream_lock), void (*retire)(struct snd_usb_substream *subs, struct urb *urb), struct snd_usb_substream *data_subs) @@ -1166,10 +1197,6 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep) INIT_LIST_HEAD(&u->ready_list); } - /* total buffer bytes of all URBs plus the next queue; - * referred in pcm.c - */ - ep->nominal_queue_size = maxsize * urb_packs * (ep->nurbs + 1); return 0; out_of_memory: @@ -1408,6 +1435,7 @@ int snd_usb_endpoint_get_clock_rate(struct snd_usb_audio *chip, int clock) */ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) { + bool is_playback = usb_pipeout(ep->pipe); int err; unsigned int i; @@ -1444,13 +1472,9 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) if (snd_usb_endpoint_implicit_feedback_sink(ep) && !(ep->chip->quirk_flags & QUIRK_FLAG_PLAYBACK_FIRST)) { - for (i = 0; i < ep->nurbs; i++) { - struct snd_urb_ctx *ctx = ep->urb + i; - list_add_tail(&ctx->ready_list, &ep->ready_playback_urbs); - } - usb_audio_dbg(ep->chip, "No URB submission due to implicit fb sync\n"); - return 0; + i = 0; + goto fill_rest; } for (i = 0; i < ep->nurbs; i++) { @@ -1459,10 +1483,18 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) if (snd_BUG_ON(!urb)) goto __error; - if (usb_pipeout(ep->pipe)) { - prepare_outbound_urb(ep, urb->context); - } else { - prepare_inbound_urb(ep, urb->context); + if (is_playback) + err = prepare_outbound_urb(ep, urb->context, true); + else + err = prepare_inbound_urb(ep, urb->context); + if (err < 0) { + /* stop filling at applptr */ + if (err == -EAGAIN) + break; + usb_audio_dbg(ep->chip, + "EP 0x%x: failed to prepare urb: %d\n", + ep->ep_num, err); + goto __error; } err = usb_submit_urb(urb, GFP_ATOMIC); @@ -1476,8 +1508,22 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) atomic_inc(&ep->submitted_urbs); } + if (!i) { + usb_audio_dbg(ep->chip, "XRUN at starting EP 0x%x\n", + ep->ep_num); + goto __error; + } + usb_audio_dbg(ep->chip, "%d URBs submitted for EP 0x%x\n", - ep->nurbs, ep->ep_num); + i, ep->ep_num); + + fill_rest: + /* put the remaining URBs to ready list */ + if (is_playback) { + for (; i < ep->nurbs; i++) + push_back_to_ready_list(ep, ep->urb + i); + } + return 0; __error: @@ -1629,7 +1675,7 @@ static void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, } spin_unlock_irqrestore(&ep->lock, flags); - queue_pending_output_urbs(ep); + snd_usb_queue_pending_output_urbs(ep, false); return; } diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 1f1a72535a64..6895d50d14d1 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -30,8 +30,9 @@ void snd_usb_endpoint_set_sync(struct snd_usb_audio *chip, struct snd_usb_endpoint *data_ep, struct snd_usb_endpoint *sync_ep); void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep, - void (*prepare)(struct snd_usb_substream *subs, - struct urb *urb), + int (*prepare)(struct snd_usb_substream *subs, + struct urb *urb, + bool in_stream_lock), void (*retire)(struct snd_usb_substream *subs, struct urb *urb), struct snd_usb_substream *data_subs); @@ -48,5 +49,7 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep); int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep, struct snd_urb_ctx *ctx, int idx, unsigned int avail); +void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, + bool in_stream_lock); #endif /* __USBAUDIO_ENDPOINT_H */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 8ad48c35c559..d5a14e5b9ad3 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -598,9 +598,6 @@ static int lowlatency_playback_available(struct snd_pcm_runtime *runtime, /* implicit feedback mode has own operation mode */ if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint)) return false; - /* too short periods? */ - if (subs->data_endpoint->nominal_queue_size >= subs->buffer_bytes) - return false; return true; } @@ -1095,6 +1092,10 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream) int ret; runtime->hw = snd_usb_hardware; + /* need an explicit sync to catch applptr update in low-latency mode */ + if (direction == SNDRV_PCM_STREAM_PLAYBACK && + as->chip->lowlatency) + runtime->hw.info |= SNDRV_PCM_INFO_EXPLICIT_SYNC; runtime->private_data = subs; subs->pcm_substream = substream; /* runtime PM is also done there */ @@ -1347,44 +1348,66 @@ static unsigned int copy_to_urb_quirk(struct snd_usb_substream *subs, return bytes; } -static void prepare_playback_urb(struct snd_usb_substream *subs, - struct urb *urb) +static int prepare_playback_urb(struct snd_usb_substream *subs, + struct urb *urb, + bool in_stream_lock) { struct snd_pcm_runtime *runtime = subs->pcm_substream->runtime; struct snd_usb_endpoint *ep = subs->data_endpoint; struct snd_urb_ctx *ctx = urb->context; - unsigned int counts, frames, bytes; + unsigned int frames, bytes; + int counts; + unsigned int transfer_done, frame_limit, avail = 0; int i, stride, period_elapsed = 0; unsigned long flags; + int err = 0; stride = ep->stride; frames = 0; ctx->queued = 0; urb->number_of_packets = 0; + spin_lock_irqsave(&subs->lock, flags); - subs->frame_limit += ep->max_urb_frames; + frame_limit = subs->frame_limit + ep->max_urb_frames; + transfer_done = subs->transfer_done; + + if (subs->lowlatency_playback && + runtime->status->state != SNDRV_PCM_STATE_DRAINING) { + unsigned int hwptr = subs->hwptr_done / stride; + + /* calculate the byte offset-in-buffer of the appl_ptr */ + avail = (runtime->control->appl_ptr - runtime->hw_ptr_base) + % runtime->buffer_size; + if (avail <= hwptr) + avail += runtime->buffer_size; + avail -= hwptr; + } + for (i = 0; i < ctx->packets; i++) { - counts = snd_usb_endpoint_next_packet_size(ep, ctx, i, 0); + counts = snd_usb_endpoint_next_packet_size(ep, ctx, i, avail); + if (counts < 0) + break; /* set up descriptor */ urb->iso_frame_desc[i].offset = frames * stride; urb->iso_frame_desc[i].length = counts * stride; frames += counts; + avail -= counts; urb->number_of_packets++; - subs->transfer_done += counts; - if (subs->transfer_done >= runtime->period_size) { - subs->transfer_done -= runtime->period_size; - subs->frame_limit = 0; + transfer_done += counts; + if (transfer_done >= runtime->period_size) { + transfer_done -= runtime->period_size; + frame_limit = 0; period_elapsed = 1; if (subs->fmt_type == UAC_FORMAT_TYPE_II) { - if (subs->transfer_done > 0) { + if (transfer_done > 0) { /* FIXME: fill-max mode is not * supported yet */ - frames -= subs->transfer_done; - counts -= subs->transfer_done; + frames -= transfer_done; + counts -= transfer_done; urb->iso_frame_desc[i].length = counts * stride; - subs->transfer_done = 0; + transfer_done = 0; } i++; if (i < ctx->packets) { @@ -1398,13 +1421,19 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, } } /* finish at the period boundary or after enough frames */ - if ((period_elapsed || - subs->transfer_done >= subs->frame_limit) && + if ((period_elapsed || transfer_done >= frame_limit) && !snd_usb_endpoint_implicit_feedback_sink(ep)) break; } - bytes = frames * stride; + if (!frames) { + err = -EAGAIN; + goto unlock; + } + + bytes = frames * stride; + subs->transfer_done = transfer_done; + subs->frame_limit = frame_limit; if (unlikely(ep->cur_format == SNDRV_PCM_FORMAT_DSD_U16_LE && subs->cur_audiofmt->dsd_dop)) { fill_playback_urb_dsd_dop(subs, urb, bytes); @@ -1434,10 +1463,19 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, subs->period_elapsed_pending = 1; period_elapsed = 0; } + + unlock: spin_unlock_irqrestore(&subs->lock, flags); + if (err < 0) + return err; urb->transfer_buffer_length = bytes; - if (period_elapsed) - snd_pcm_period_elapsed(subs->pcm_substream); + if (period_elapsed) { + if (in_stream_lock) + snd_pcm_period_elapsed_under_stream_lock(subs->pcm_substream); + else + snd_pcm_period_elapsed(subs->pcm_substream); + } + return 0; } /* @@ -1469,6 +1507,27 @@ static void retire_playback_urb(struct snd_usb_substream *subs, snd_pcm_period_elapsed(subs->pcm_substream); } +/* PCM ack callback for the playback stream; + * this plays a role only when the stream is running in low-latency mode. + */ +static int snd_usb_pcm_playback_ack(struct snd_pcm_substream *substream) +{ + struct snd_usb_substream *subs = substream->runtime->private_data; + struct snd_usb_endpoint *ep; + + if (!subs->lowlatency_playback || !subs->running) + return 0; + ep = subs->data_endpoint; + if (!ep) + return 0; + /* When no more in-flight URBs available, try to process the pending + * outputs here + */ + if (!ep->active_mask) + snd_usb_queue_pending_output_urbs(ep, true); + return 0; +} + static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substream, int cmd) { @@ -1572,6 +1631,7 @@ static const struct snd_pcm_ops snd_usb_playback_ops = { .trigger = snd_usb_substream_playback_trigger, .sync_stop = snd_usb_pcm_sync_stop, .pointer = snd_usb_pcm_pointer, + .ack = snd_usb_pcm_playback_ack, }; static const struct snd_pcm_ops snd_usb_capture_ops = { -- 2.26.2
While draining a stream, ALSA PCM core stops the stream by issuing snd_pcm_stop() after all data has been sent out. And, at PCM trigger stop, currently USB-audio driver kills the in-flight URBs explicitly, then at sync-stop ops, sync with the finish of all remaining URBs. This might result in a drop of the drained samples as most of USB-audio devices / hosts allow relatively long in-flight samples (as a sort of FIFO). For avoiding the trimming, this patch changes the stream-stop behavior during PCM draining state. Under that condition, the pending URBs won't be killed. The leftover in-flight URBs are caught by the sync-stop operation that shall be performed after the trigger-stop operation. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/endpoint.c | 14 +++++++++----- sound/usb/endpoint.h | 2 +- sound/usb/pcm.c | 16 ++++++++-------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 0b336876e36d..42c0d2db8ba8 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -955,7 +955,7 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep) * * This function moves the EP to STOPPING state if it's being RUNNING. */ -static int stop_urbs(struct snd_usb_endpoint *ep, bool force) +static int stop_urbs(struct snd_usb_endpoint *ep, bool force, bool keep_pending) { unsigned int i; unsigned long flags; @@ -972,6 +972,9 @@ static int stop_urbs(struct snd_usb_endpoint *ep, bool force) ep->next_packet_queued = 0; spin_unlock_irqrestore(&ep->lock, flags); + if (keep_pending) + return 0; + for (i = 0; i < ep->nurbs; i++) { if (test_bit(i, &ep->active_mask)) { if (!test_and_set_bit(i, &ep->unlink_mask)) { @@ -995,7 +998,7 @@ static int release_urbs(struct snd_usb_endpoint *ep, bool force) snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL); /* stop and unlink urbs */ - err = stop_urbs(ep, force); + err = stop_urbs(ep, force, false); if (err) return err; @@ -1527,7 +1530,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) return 0; __error: - snd_usb_endpoint_stop(ep); + snd_usb_endpoint_stop(ep, false); return -EPIPE; } @@ -1535,6 +1538,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) * snd_usb_endpoint_stop: stop an snd_usb_endpoint * * @ep: the endpoint to stop (may be NULL) + * @keep_pending: keep in-flight URBs * * A call to this function will decrement the running count of the endpoint. * In case the last user has requested the endpoint stop, the URBs will @@ -1545,7 +1549,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) * The caller needs to synchronize the pending stop operation via * snd_usb_endpoint_sync_pending_stop(). */ -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool keep_pending) { if (!ep) return; @@ -1560,7 +1564,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) if (!atomic_dec_return(&ep->running)) { if (ep->sync_source) WRITE_ONCE(ep->sync_source->sync_sink, NULL); - stop_urbs(ep, false); + stop_urbs(ep, false, keep_pending); } } diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 6895d50d14d1..6a9af04cf175 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -38,7 +38,7 @@ void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep, struct snd_usb_substream *data_subs); int snd_usb_endpoint_start(struct snd_usb_endpoint *ep); -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool keep_pending); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); void snd_usb_endpoint_suspend(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index d5a14e5b9ad3..f09c7380a923 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -219,16 +219,16 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, return 0; } -static bool stop_endpoints(struct snd_usb_substream *subs) +static bool stop_endpoints(struct snd_usb_substream *subs, bool keep_pending) { bool stopped = 0; if (test_and_clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) { - snd_usb_endpoint_stop(subs->sync_endpoint); + snd_usb_endpoint_stop(subs->sync_endpoint, keep_pending); stopped = true; } if (test_and_clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags)) { - snd_usb_endpoint_stop(subs->data_endpoint); + snd_usb_endpoint_stop(subs->data_endpoint, keep_pending); stopped = true; } return stopped; @@ -261,7 +261,7 @@ static int start_endpoints(struct snd_usb_substream *subs) return 0; error: - stop_endpoints(subs); + stop_endpoints(subs, false); return err; } @@ -437,7 +437,7 @@ static int configure_endpoints(struct snd_usb_audio *chip, if (subs->data_endpoint->need_setup) { /* stop any running stream beforehand */ - if (stop_endpoints(subs)) + if (stop_endpoints(subs, false)) sync_pending_stops(subs); err = snd_usb_endpoint_configure(chip, subs->data_endpoint); if (err < 0) @@ -572,7 +572,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->cur_audiofmt = NULL; mutex_unlock(&chip->mutex); if (!snd_usb_lock_shutdown(chip)) { - if (stop_endpoints(subs)) + if (stop_endpoints(subs, false)) sync_pending_stops(subs); close_endpoints(chip, subs); snd_usb_unlock_shutdown(chip); @@ -1559,7 +1559,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea return 0; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - stop_endpoints(subs); + stop_endpoints(subs, substream->runtime->status->state == SNDRV_PCM_STATE_DRAINING); snd_usb_endpoint_set_callback(subs->data_endpoint, NULL, NULL, NULL); subs->running = 0; @@ -1607,7 +1607,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream return 0; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - stop_endpoints(subs); + stop_endpoints(subs, false); fallthrough; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: snd_usb_endpoint_set_callback(subs->data_endpoint, -- 2.26.2
participants (2)
-
Hillf Danton -
Takashi Iwai