On Thu, 23 Apr 2020 19:29:08 +0200, Takashi Iwai wrote: > > On Thu, 23 Apr 2020 18:57:34 +0200, > Alexander Tsoy wrote: > > > > And some further notes: > > > > - I removed locking from snd_usb_endpoint_next_packet_size() and this > > seems completely fixed an issue with large URBs I reported here: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > > > So playing at 96 kHz, driver packs 48 frames per URB and no more audio > > discontinuities. > > Hmm, that's weird. > > If removing the lock from snd_usb_endpoint_next_packet_size() really > fixes the problem, it implies the lock contention. But as far as I > see the code performed in this lock isn't conflicting so much. The > URB processing shouldn't happen in parallel for the same EP. BTW, one potential racy code I found while looking at the code is the list management in queue_pending_output_urbs(). The fix patch is below. Takashi --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -321,17 +321,17 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) ep->next_packet_read_pos %= MAX_URBS; /* take URB out of FIFO */ - if (!list_empty(&ep->ready_playback_urbs)) + if (!list_empty(&ep->ready_playback_urbs)) { ctx = list_first_entry(&ep->ready_playback_urbs, struct snd_urb_ctx, ready_list); + list_del_init(&ctx->ready_list); + } } spin_unlock_irqrestore(&ep->lock, flags); if (ctx == NULL) return; - list_del_init(&ctx->ready_list); - /* copy over the length information */ for (i = 0; i < packet->packets; i++) ctx->packet_size[i] = packet->packet_size[i];
