From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4FBB140DFDC for ; Fri, 8 May 2026 16:56:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778259384; cv=none; b=uKSQTwrKOy7Oq1fJR+8MdE7ZG5Q2ugveUK0ZJ7sWOpDJqkRDqgKB8nJy2apAEdaauzSO26G5D6AMVoknLyPnueqFBmyq9PtN2xLj/88u3tiDD8+WnkApH2S0nJjuJSMkGEIANwfE6rWcqR2jsz6+zGsu3W6zeUinpdQ7XIiVwC0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778259384; c=relaxed/simple; bh=xAPysj9jqBxnNH9ah7LQXapnO4WlMNmkFCZEkS3PQ8g=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=PLKXhJQ46j732iRuMDsNKuKedm2YJ4wP/bepA8PsmyYBi76Y4Uotc8wKCXT+q4Pq8vAEQhP+JHAbkhRapWF/fg+1hV5sK1dyzqTXMEpnD1X+mObZ8EWXpF3mMQH2Ykh/a3GuhSFnu4hRb31NgRRLCTsal/d0UPXCdRcT2/8+UQU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=kw5FAJxx; arc=none smtp.client-ip=209.85.221.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="kw5FAJxx" Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-43d76dd4ee8so1957422f8f.2 for ; Fri, 08 May 2026 09:56:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1778259382; x=1778864182; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=PNzAhALF+27Ww+LeezAsS+tgg1krlWNQ1vu17boWQ+w=; b=kw5FAJxxTrc/ZN+oumytsllYqCuKDzp0cr7/74A5hSIjqne1WPpfUFJ7/CQbX35Shs w9s1La+dtB5pjlmGAFS+LCpbtRjoq26Ku3yhHWii1dzIl7DDNwzZFa714B6lpUr81ebU O3tp4Gk2iPjJVn3OCYsBXoS4aB09+X/vG9+Vt6dgInzskQZ3Fm+5VJc5uCWLBiyILO0u /l+wPDMBcDAKGnKvuAgqYtqFMeWjh2gfmb4FODClPod6aqEEazJQGrPbQE3EAfgMzOKE 7U+ZZUI6Bn6jQ6Pil+DtlttiVqtoHnGpW5lbd3Ysfs5TLNzDmPgp2wbC2WriYsQ2sE9d 2wJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778259382; x=1778864182; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=PNzAhALF+27Ww+LeezAsS+tgg1krlWNQ1vu17boWQ+w=; b=oeSfe4BeTBfIaLYaTobrWDTPAZYsE37hlRMd7Xvr+JSx926Rm4zKC3IfPa7S2mVkjB 3rcAG8T2e5Ydny9f6qIEgjWrTpQbwlqw8mr5pe0FjlR2noP+dUNN+VQnq/QhB1EYaCYq rWl9F0GKr8wp8Rjcl7IdLlr15xYkHyjqLy5EOdSTqsSbwKQvyYGObK3z8AvJaybVbgHc C4rupjebzIAgJI4LdCx66lm3L7SfSKU8zquhTKyX6Vp5ONVeKgX5bGU+4UzAmMgAXA0I ikmQ46t7j0qqAeTDzQK04L1GlB8UsUIoop1aoygx1fNsPdyd/IOgdf/ltUUERjzouxQH R8DQ== X-Forwarded-Encrypted: i=1; AFNElJ/6j4flEiMJh6UuExZJWp+X7iKgIFWNLNGEhp6pJkD1Jha2vRkdHDPkajpB2dBYy+IVYFmb/s5XaNfyU5c=@vger.kernel.org X-Gm-Message-State: AOJu0YwZPcbKLTd9ZEi5aKJUZweiwOnX8HzL8FrmCTOnrBNxT7Uym2PR 3m0G/YccCz901zP8DZrmLQo+lzf8Nko3hfVeEFrdUxBazWOD3qMmZCr3 X-Gm-Gg: Acq92OHFUU8ErMC7Of6HEc8XM8DKcSBbjsG6ex6asjzrHHJZUj5IfdSGpc7DA+KPPsP GmSi/XKVGhq+RcspljUVLL1urKXAt+8aPJLV0defFSp+aFEFj2DxvwJEWn/Sdu0z34Uun32BB5u 337ocjWtccZZ0Dai/vdUi/jgncwO3Ynvg/GFjCBSSvTsKGwSiAETkAA41KzbQIAGobI/0oF3kp8 HBhpTv413Z9SgypeE1mKr3ZM8LbGGrMaKkJKAp/9cb6DSoxEEnG5Lb6pV5u7vbnZH93/OSQ7QsB UQs4kcuL+rbmMf0DRwaLLOgIEkBQyTpUFHWKvApO5htJI/LJ+/wEeJ7M2kigek3mQ26w4jRMH5a CDZ4EGx3VvVfizav0G1DHD6rdkuim5zd6Ia7K4DfLuVkQIVg7UwD3OAHl8UXmluft1YLFHGLoTu UE8cKEilGWBsedxu+FlUoCb5dUWKLKn43osGc= X-Received: by 2002:a05:6000:61e:b0:43d:7e6f:3816 with SMTP id ffacd0b85a97d-4515d9a05b5mr20486633f8f.40.1778259381497; Fri, 08 May 2026 09:56:21 -0700 (PDT) Received: from foxbook (bgt227.neoplus.adsl.tpnet.pl. [83.28.83.227]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4548ec6c221sm5430819f8f.13.2026.05.08.09.56.20 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Fri, 08 May 2026 09:56:21 -0700 (PDT) Date: Fri, 8 May 2026 18:56:17 +0200 From: Michal Pecio To: Nicola Lunghi Cc: mathias.nyman@intel.com, niklas.neronin@linux.intel.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers Message-ID: <20260508185617.6bf8a6eb.michal.pecio@gmail.com> In-Reply-To: <20260504233143.10242-3-nick83ola@gmail.com> References: <20260504233143.10242-2-nick83ola@gmail.com> <20260504233143.10242-3-nick83ola@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 5 May 2026 01:31:43 +0200, Nicola Lunghi wrote: > xhci_get_isoc_frame_id() silently rescheduled the first TRB to > start_frame_id+1 when the requested start frame was out of the valid > scheduling window or landed exactly on its boundary. This creates an > explicit one-frame hole in the isochronous stream. >=20 > Most USB audio devices tolerate a brief gap with a small glitch and > recover automatically. However, some devices assume that once > isochronous packets start streaming they flow continuously until the > stream is explicitly stopped. Any gap causes the device firmware to > permanently lose channel synchronization =E2=80=94 subsequent packets are > routed to the wrong output channels until the device is reset. That's not the only problem, we are basically violating documented usb_submit_urb() behavior, as Alan points out in Bugzilla. =20 > This was observed with the MOTU 1248 (USB ID 0x07fd:0x0005), where > after a gap the 24-channel output stream shifts by a fixed number of > channels, mapping audio intended for ch1/ch2 onto ch7/ch8 or other > channel pairs depending on timing. >=20 > Return -EINVAL instead so the caller falls back to TRB_SIA (Schedule > Immediately After) That's not what this acronym really means. > which lets the hardware place the TRB right after the previous one > without introducing a frame-aligned gap. IIRC, the meaning of "Start Isoch ASAP" is like Linux URB_ISO_ASAP: "as soon as possible *and not earlier* than possible". To guarantee no gaps, we should use SIA=3D0 and incremental Frame IDs on chips with CFC (without CFC it's a lost cause). The reason using SIA=3D1 helped you is because in reality: - the start_frame_id calculated here is pessimistic - I found that the IST reported by HCs is usually pessimistic too Therefore, even if this function believes that it's too late to execute some transfer (and currently tries to reschedule it for later), the HC may actually still execute it immediately without gaps if SIA=3D1. But the right thing to do is SIA=3D0 and correctly specified Frame ID. Setting SIA=3D1 opens the possibility of rescheduling for later (in HW) when it *really* is too late. And we don't want that, we want late submissions to result in Missed Service Error and EXDEV completion. Note that the comment in this function is bogus too. In reality: Software *shall* not schedule past end_frame_id. Software *should* not schedule before start_frame_id. The former is a requirement, the latter a recommendation. And a valid one, if we want the URB to execute. But here, we want it to fail :) Also, I'm not sure if ignoring submissions far into the future and turning them into SIA=3D1 is the right action. If a driver submits an untinterrupted stream (without URB_ISO_ASAP) going 895ms into the future, we should probably stop this madness and error out (TBD). > Link: https://bugzilla.kernel.org/show_bug.cgi?id=3D220748 > Assisted-by: Claude:claude-sonnet-4-6 sparse checkpatch > Signed-off-by: Nicola Lunghi > --- > drivers/usb/host/xhci-ring.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) >=20 > diff --git a/drivers/usb/host/xhci-ring.c > b/drivers/usb/host/xhci-ring.c index e47e644b296e..03e47db82092 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -4030,15 +4030,21 @@ static int xhci_get_isoc_frame_id(struct > xhci_hcd *xhci, ret =3D -EINVAL; > } > =20 > + /* > + * If the first TRB's start frame is out of the scheduling window or > + * lands exactly on its boundary, fall back to SIA (Schedule Immediately > + * After) rather than forcing start_frame_id+1. A forced +1 creates an > + * explicit one-frame hole that audio devices with strict continuity > + * requirements cannot recover from. The caller handles -EINVAL by > + * leaving sia_frame_id as TRB_SIA. > + */ > if (index =3D=3D 0) { > if (ret =3D=3D -EINVAL || start_frame =3D=3D start_frame_id) > { > - start_frame =3D start_frame_id + 1; > - if (urb->dev->speed =3D=3D USB_SPEED_LOW || > - urb->dev->speed =3D=3D USB_SPEED_FULL) > - urb->start_frame =3D start_frame; > - else > - urb->start_frame =3D start_frame << 3; > - ret =3D 0; > + xhci_dbg(xhci, "isoc: start frame %d %s window [%d, %d], using SIA\n", > + start_frame, > + ret =3D=3D -EINVAL ? "behind" : "at boundary of", > + start_frame_id, end_frame_id); I'm not very fond of debug prints which can show every microframe. Even better than a "debuggable" code is one which is right, well tested and known to work without worries. > + return -EINVAL; > } > } > =20 > --=20 > 2.51.0 >=20