Re: [PATCH] usb-audio: Fix boot-time crackling for Generic USB Audio device

[Takashi Iwai]

On Tue, 14 Jul 2026 15:02:09 +0200,
Zhang Heng wrote:
> 
> > On Tue, 14 Jul 2026 03:58:08 +0200,
> > Zhang Heng wrote:
> >>> On Mon, Jul 13, 2026 at 04:10:48PM +0800, Zhang Heng wrote:
> >>>> +		/* Generic USB Audio (0x1e0b:d01e): use SIA for consistent scheduling */
> >>>> +		if (ep->chip->usb_id == USB_ID(0x1e0b, 0xd01e))
> >>>> +			u->urb->transfer_flags |= URB_ISO_ASAP;
> >>> Not a maintainer, just a bystander who was Cc'd -- two small notes on the
> >>> form of the patch, plus one question I can't answer myself.
> >>> 
> >>> A per-device usb_id comparison in the endpoint.c fast path seems like the
> >>> kind of thing the quirk_flags_table in quirks.c exists to avoid. Would a
> >>> QUIRK_FLAG_ISO_ASAP (set in the table, tested here as
> >>> chip->quirk_flags & QUIRK_FLAG_ISO_ASAP) work for you? It keeps endpoint.c
> >>> device-agnostic, and the next device with the same symptom becomes a
> >>> one-line table entry rather than another if.
> >> First of all, let me clarify: this issue occurs when there is startup music,
> >> but it plays normally after entering the system. There is a heavy creaking
> >> sound when the system is turned on here. Based on the information from dmesg
> >> and syslog, there are a large number of xhci hcd frame synchronization
> >> failures. I tried adding URB_ISO_ASAP here, and it will be much better,
> >> with only a little noise.
> > OK, but it still makes sense to deal URB_ISO_ASAP workaround more
> > generically.
> > 
> >>>> +	if (ep->chip->usb_id == USB_ID(0x1e0b, 0xd01e) &&
> >>>> +	    ep->type == SND_USB_ENDPOINT_TYPE_SYNC)
> >>>> +		ep->skip_packets = 4;
> >>> The block immediately above this one already does exactly
> >>> "type == SND_USB_ENDPOINT_TYPE_SYNC -> skip_packets = 4"; adding the ID to
> >>> that condition would avoid the duplicate if. Also, the changelog doesn't
> >>> say what this hunk contributes on its own -- is URB_ISO_ASAP alone
> >>> insufficient, and if so, what does skipping the first 4 sync packets fix
> >>> that ASAP doesn't? Right now the two changes are indistinguishable in the
> >>> commit message, and skip_packets = 4 reads as belt-and-braces.
> >> As mentioned above, there is still a bit of noise when only adding
> >> URB_ISO_ASAP,
> >> 
> >> but skip-packets=4 can solve this problem.
> > Well, one missing thing is to understand why this fixes.
> > Originally, the skip_packets=4 for Playback Design devices was
> > introduced for bogus feedback packets at the start of the stream long
> > time ago.  But that's the only known device that needs it.  Does your
> > device send also 4 bogus packets?
> > 
> > The skip_packets=16 for M-Audio devices are rather for avoiding the
> > latency.  And, speaking of latency, I have a patch for the lowlatency
> > support of implicit fb mode, but never had a test environment.
> > Could you check the patch below and see the patch below has any
> > positive/negative influence?  Just to be sure.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > -- 8< --
> > index 682b6c1fe76b..6d144e39a849 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -265,6 +265,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip,
> >   	return 0;
> >   }
> >   +/* stop both data and sync endpoints */
> >   static bool stop_endpoints(struct snd_usb_substream *subs, bool keep_pending)
> >   {
> >   	bool stopped = 0;
> > @@ -280,6 +281,24 @@ static bool stop_endpoints(struct snd_usb_substream *subs, bool keep_pending)
> >   	return stopped;
> >   }
> >   +/* only start sync endpoint */
> > +static int start_sync_endpoint(struct snd_usb_substream *subs)
> > +{
> > +	int err;
> > +
> > +	if (subs->sync_endpoint &&
> > +	    !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) {
> > +		err = snd_usb_endpoint_start(subs->sync_endpoint);
> > +		if (err < 0) {
> > +			clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
> > +			return err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* start both data and sync endpoints */
> >   static int start_endpoints(struct snd_usb_substream *subs)
> >   {
> >   	int err;
> > @@ -295,14 +314,9 @@ static int start_endpoints(struct snd_usb_substream *subs)
> >   		}
> >   	}
> >   -	if (subs->sync_endpoint &&
> > -	    !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) {
> > -		err = snd_usb_endpoint_start(subs->sync_endpoint);
> > -		if (err < 0) {
> > -			clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
> > -			goto error;
> > -		}
> > -	}
> > +	err = start_sync_endpoint(subs);
> > +	if (err < 0)
> > +		goto error;
> >     	return 0;
> >   @@ -656,12 +670,16 @@ static int
> > lowlatency_playback_available(struct snd_pcm_runtime *runtime,
> >   		return false;
> >   	if (in_free_wheeling_mode(runtime))
> >   		return false;
> > -	/* implicit feedback mode has own operation mode */
> > -	if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint))
> > -		return false;
> >   	return true;
> >   }
> >   +/* return true if it's a normal playback (not in implicit fb) */
> > +static bool is_normal_lowlatency_playback(struct snd_usb_substream *subs)
> > +{
> > +	return subs->lowlatency_playback &&
> > +		!snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint);
> > +}
> > +
> >   /*
> >    * prepare callback
> >    *
> > @@ -709,9 +727,11 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> >   	runtime->delay = 0;
> >     	subs->lowlatency_playback =
> > lowlatency_playback_available(runtime, subs);
> > -	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> > -	    !subs->lowlatency_playback) {
> > -		ret = start_endpoints(subs);
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		if (!subs->lowlatency_playback)
> > +			ret = start_endpoints(subs);
> > +		else if (snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint))
> > +			ret = start_sync_endpoint(subs);
> >   		/* if XRUN happens at starting streams (possibly with implicit
> >   		 * fb case), restart again, but only try once.
> >   		 */
> > @@ -1539,7 +1559,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
> >   		frame_limit = subs->frame_limit + ep->max_urb_frames;
> >   		transfer_done = subs->transfer_done;
> >   -		if (subs->lowlatency_playback &&
> > +		if (is_normal_lowlatency_playback(subs) &&
> >   		    runtime->state != SNDRV_PCM_STATE_DRAINING) {
> >   			unsigned int hwptr = subs->hwptr_done / stride;
> >   @@ -1625,7 +1645,8 @@ static int prepare_playback_urb(struct
> > snd_usb_substream *subs,
> >   			subs->trigger_tstamp_pending_update = false;
> >   		}
> >   -		if (period_elapsed && !subs->running &&
> > subs->lowlatency_playback) {
> > +		if (period_elapsed && !subs->running &&
> > +		    is_normal_lowlatency_playback(subs)) {
> >   			subs->period_elapsed_pending = 1;
> >   			period_elapsed = 0;
> >   		}
> > @@ -1677,7 +1698,7 @@ 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)
> > +	if (!is_normal_lowlatency_playback(subs) || !subs->running)
> >   		return 0;
> >   	ep = subs->data_endpoint;
> >   	if (!ep)
> > @@ -1705,6 +1726,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
> >   					      prepare_playback_urb,
> >   					      retire_playback_urb,
> >   					      subs);
> > +		/* start EPs for both normal and implicit-fb modes */
> >   		if (subs->lowlatency_playback &&
> >   		    cmd == SNDRV_PCM_TRIGGER_START) {
> >   			if (in_free_wheeling_mode(substream->runtime))
> Regarding your patch, I am unable to test it because my current
> kernel is based on a certain stable version of 5.4. The
> differences in pcm.c are too significant, so the patch cannot
> be applied directly.

Please verify whether the problem really exists with the latest
kernel at first.  At least we have to be sure about the workaround
with URB_ISO_ASAP for the latest kernel before taking to the upstream.


thanks,

Takashi