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

[Zhang Heng]

> 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.