[RFC PATCH 0/2] Bluetooth: ISO: support setting send Time_Stamp and LE Read ISO TX Sync

[RFC PATCH 0/2] Bluetooth: ISO: support setting send Time_Stamp and LE Read ISO TX Sync

[Pauli Virtanen]

Add support for userspace setting BT_SCM_PKT_SEQNUM and
BT_SCM_PKT_ISO_TS as CMSG to provide timestamps and sequence numbers to
controller.

Also support obtaining sent packet timestamps via HCI LE Read ISO TX
Sync and transmitting those to user as HW timestamps.

User-provided BT_SCM_PKT_SEQNUM may make sense if userspace is not
sending SDUs every interval, and it is in the best position to determine
when it wanted things to be sent.  Behavior of course varies per
controller, so not sure how useful this is in practice, and if it's
premature to expose this in case there will be future clarifications in
the Core Specification (no idea about whether that's likely).

RFC since this is not satisfactorily tested on real hardware, and there
may be some design questions and ugly bits here. Sending anyway since
there seems to be interest in this.

I also send the series adding iso-tester + emulator tests.

Pauli Virtanen (2):
  Bluetooth: ISO: allow userspace to set seqnum and timestamp for
    sending
  Bluetooth: produce HW TX timestamps for ISO from LE Read ISO TX Sync

 include/net/bluetooth/bluetooth.h |  2 +
 include/net/bluetooth/hci.h       |  2 +-
 include/net/bluetooth/hci_core.h  |  5 +-
 net/bluetooth/hci_conn.c          | 53 +++++++++++++++++++--
 net/bluetooth/hci_core.c          |  8 ++--
 net/bluetooth/hci_event.c         | 79 ++++++++++++++++++++++++++++++-
 net/bluetooth/iso.c               | 70 +++++++++++++++++++++++----
 7 files changed, 199 insertions(+), 20 deletions(-)

-- 
2.53.0

[RFC PATCH 1/2] Bluetooth: ISO: allow userspace to set seqnum and timestamp for sending

[Pauli Virtanen]

Kernel currently leaves ISO packet scheduling to userspace.  To do this
properly, e.g. to ensure different CIS are synchronized, userspace needs
to be able to set packet timestamps. Core v6.2 Vol 6 Part G Sec 3.3
specifies that seqnum and timestamp shall correspond to each other, so
userspace must also be able to override the sequence number.

Add new CMSG BT_SCM_PKT_ISO_TS for the ISO timestamp.

Support BT_SCM_PKT_SEQNUM and BT_SCM_PKT_ISO_TS CMSG for sent packets.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    This is more or less the same patch as
    https://lore.kernel.org/linux-bluetooth/20260311081401.3832000-1-tailu.shi@samsung.com/
    but in slightly different words.
    
    Sending anyway now, since I had the test series written against this
    one.

 include/net/bluetooth/bluetooth.h |  2 +
 include/net/bluetooth/hci_core.h  |  2 +-
 net/bluetooth/hci_core.c          |  8 ++--
 net/bluetooth/iso.c               | 70 +++++++++++++++++++++++++++----
 4 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 69eed69f7f26..7f7e13a51de7 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -259,6 +259,8 @@ struct bt_codecs {
 
 #define BT_SCM_PKT_SEQNUM	0x05
 
+#define BT_SCM_PKT_ISO_TS	0x06
+
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
 __printf(1, 2)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a7bffb908c1e..c82cec45a9a1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -2336,7 +2336,7 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
 		 const void *param);
 void hci_send_acl(struct hci_chan *chan, struct sk_buff *skb, __u16 flags);
 void hci_send_sco(struct hci_conn *conn, struct sk_buff *skb);
-void hci_send_iso(struct hci_conn *conn, struct sk_buff *skb);
+void hci_send_iso(struct hci_conn *conn, struct sk_buff *skb, bool ts);
 
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
 void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index eb32802f6296..da2ac924aaf6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3322,7 +3322,7 @@ static void hci_add_iso_hdr(struct sk_buff *skb, __u16 handle, __u8 flags)
 }
 
 static void hci_queue_iso(struct hci_conn *conn, struct sk_buff_head *queue,
-			  struct sk_buff *skb)
+			  struct sk_buff *skb, u8 ts)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct sk_buff *list;
@@ -3335,7 +3335,7 @@ static void hci_queue_iso(struct hci_conn *conn, struct sk_buff_head *queue,
 
 	list = skb_shinfo(skb)->frag_list;
 
-	flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, 0x00);
+	flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, ts);
 	hci_add_iso_hdr(skb, conn->handle, flags);
 
 	if (!list) {
@@ -3368,13 +3368,13 @@ static void hci_queue_iso(struct hci_conn *conn, struct sk_buff_head *queue,
 	bt_dev_dbg(hdev, "hcon %p queued %d", conn, skb_queue_len(queue));
 }
 
-void hci_send_iso(struct hci_conn *conn, struct sk_buff *skb)
+void hci_send_iso(struct hci_conn *conn, struct sk_buff *skb, bool ts)
 {
 	struct hci_dev *hdev = conn->hdev;
 
 	BT_DBG("%s len %d", hdev->name, skb->len);
 
-	hci_queue_iso(conn, &conn->data_q, skb);
+	hci_queue_iso(conn, &conn->data_q, skb, ts ? 0x01 : 0x00);
 
 	queue_work(hdev->workqueue, &hdev->tx_work);
 }
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index a38d3774176d..260eb2d96390 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -537,11 +537,13 @@ static struct bt_iso_qos *iso_sock_get_qos(struct sock *sk)
 }
 
 static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
-			  const struct sockcm_cookie *sockc)
+			  const struct sockcm_cookie *sockc,
+			  u32 seqnum, u64 timestamp)
 {
 	struct iso_conn *conn = iso_pi(sk)->conn;
 	struct bt_iso_qos *qos = iso_sock_get_qos(sk);
-	struct hci_iso_data_hdr *hdr;
+	__le16 slen, sn;
+	bool ts;
 	int len = 0;
 
 	BT_DBG("sk %p len %d", sk, skb->len);
@@ -552,14 +554,29 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
 	len = skb->len;
 
 	/* Push ISO data header */
-	hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
-	hdr->sn = cpu_to_le16(conn->tx_sn++);
-	hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
-						      HCI_ISO_STATUS_VALID));
+	ts = (timestamp <= U32_MAX);
+	slen = cpu_to_le16(hci_iso_data_len_pack(len,
+						 HCI_ISO_STATUS_VALID));
+	sn = cpu_to_le16(seqnum <= U16_MAX ? seqnum : conn->tx_sn++);
+
+	if (ts) {
+		struct hci_iso_ts_data_hdr *hdr;
+
+		hdr = skb_push(skb, HCI_ISO_TS_DATA_HDR_SIZE);
+		hdr->ts = cpu_to_le32(timestamp);
+		hdr->sn = sn;
+		hdr->slen = slen;
+	} else {
+		struct hci_iso_data_hdr *hdr;
+
+		hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
+		hdr->sn = sn;
+		hdr->slen = slen;
+	}
 
 	if (sk->sk_state == BT_CONNECTED) {
 		hci_setup_tx_timestamp(skb, 1, sockc);
-		hci_send_iso(conn->hcon, skb);
+		hci_send_iso(conn->hcon, skb, ts);
 	} else {
 		len = -ENOTCONN;
 	}
@@ -1465,12 +1482,43 @@ static int iso_sock_getname(struct socket *sock, struct sockaddr *addr,
 	return len;
 }
 
+static int iso_cmsg_send(struct sock *sk, struct msghdr *msg,
+			 u32 *seqnum, u64 *timestamp)
+{
+	struct cmsghdr *cmsg;
+
+	for_each_cmsghdr(cmsg, msg) {
+		if (!CMSG_OK(msg, cmsg))
+			return -EINVAL;
+		if (cmsg->cmsg_level != SOL_BLUETOOTH)
+			continue;
+
+		switch (cmsg->cmsg_type) {
+		case BT_SCM_PKT_SEQNUM:
+			if (cmsg->cmsg_len != CMSG_LEN(sizeof(u16)))
+				return -EINVAL;
+			*seqnum = *(u16 *)CMSG_DATA(cmsg);
+			break;
+		case BT_SCM_PKT_ISO_TS:
+			if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
+				return -EINVAL;
+			*timestamp = *(u32 *)CMSG_DATA(cmsg);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 			    size_t len)
 {
 	struct sock *sk = sock->sk;
 	struct sk_buff *skb, **frag;
 	struct sockcm_cookie sockc;
+	u32 seqnum = U32_MAX;
+	u64 timestamp = U64_MAX;
 	size_t mtu;
 	int err;
 
@@ -1486,6 +1534,10 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	hci_sockcm_init(&sockc, sk);
 
 	if (msg->msg_controllen) {
+		err = iso_cmsg_send(sk, msg, &seqnum, &timestamp);
+		if (err)
+			return err;
+
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (err)
 			return err;
@@ -1502,7 +1554,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	release_sock(sk);
 
-	skb = bt_skb_sendmsg(sk, msg, len, mtu, HCI_ISO_DATA_HDR_SIZE, 0);
+	skb = bt_skb_sendmsg(sk, msg, len, mtu, HCI_ISO_TS_DATA_HDR_SIZE, 0);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
@@ -1536,7 +1588,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	lock_sock(sk);
 
 	if (sk->sk_state == BT_CONNECTED)
-		err = iso_send_frame(sk, skb, &sockc);
+		err = iso_send_frame(sk, skb, &sockc, seqnum, timestamp);
 	else
 		err = -ENOTCONN;
 
-- 
2.53.0

RE: Bluetooth: ISO: support setting send Time_Stamp and LE Read ISO TX Sync

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3468 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1065140

---Test result---

Test Summary:
CheckPatch                    PENDING   0.33 seconds
GitLint                       PENDING   0.26 seconds
SubjectPrefix                 PASS      0.25 seconds
BuildKernel                   PASS      26.20 seconds
CheckAllWarning               PASS      28.57 seconds
CheckSparse                   WARNING   32.83 seconds
BuildKernel32                 PASS      26.03 seconds
TestRunnerSetup               PASS      574.85 seconds
TestRunner_l2cap-tester       PASS      29.62 seconds
TestRunner_iso-tester         FAIL      59.66 seconds
TestRunner_bnep-tester        PASS      6.62 seconds
TestRunner_mgmt-tester        FAIL      127.39 seconds
TestRunner_rfcomm-tester      PASS      9.87 seconds
TestRunner_sco-tester         FAIL      14.85 seconds
TestRunner_ioctl-tester       PASS      10.71 seconds
TestRunner_mesh-tester        FAIL      12.96 seconds
TestRunner_smp-tester         PASS      8.92 seconds
TestRunner_userchan-tester    PASS      6.89 seconds
IncrementalBuild              PENDING   0.63 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_core.c:85:9: warning: context imbalance in '__hci_dev_get' - different lock contexts for basic blocknet/bluetooth/hci_core.c: note: in included file (through include/linux/notifier.h, include/linux/memory_hotplug.h, include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, include/linux/radix-tree.h, ...):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in le_read_features_complete+0x7e/0x2b0
Total: 141, Passed: 140 (99.3%), Failed: 1, Not Run: 0

Failed Test Cases
ISO Ethtool Get Ts Info - Success                    Failed       0.071 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.114 seconds
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
BUG: sleeping function called from invalid context at net/core/sock.c:3782
Total: 30, Passed: 30 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.478 seconds
Mesh - Send cancel - 2                               Timed out    1.995 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

[RFC PATCH 2/2] Bluetooth: produce HW TX timestamps for ISO from LE Read ISO TX Sync

[Pauli Virtanen]

For ISO packets with HW TX timestamping enabled, retrieve ISO send
timestamps via LE Read ISO TX Sync. Specification allows retrieving the
timestamp only for the "latest" sent packet.

Report SDU Reference time as the timestamp, as this is the value
relevant for userspace send synchronization.

Due to overall poor description of the timing features in ISO-over-HCI
transport in Core Specification v6.2, it is ambiguous what packet the
command actually reports, so timestamps may not be assigned to the
exactly correct packets, user applications have to deal with it.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci.h      |  2 +-
 include/net/bluetooth/hci_core.h |  3 +-
 net/bluetooth/hci_conn.c         | 53 +++++++++++++++++++--
 net/bluetooth/hci_event.c        | 79 +++++++++++++++++++++++++++++++-
 4 files changed, 131 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 89ad9470fa71..b536587a909f 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -2138,7 +2138,7 @@ struct hci_rp_le_read_iso_tx_sync {
 	__u8    status;
 	__le16  handle;
 	__le16  seq;
-	__le32  imestamp;
+	__le32  timestamp;
 	__u8    offset[3];
 } __packed;
 
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c82cec45a9a1..5d530349c2a4 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -272,6 +272,7 @@ struct adv_info {
 
 struct tx_queue {
 	struct sk_buff_head queue;
+	struct sk_buff *iso_last_tx;
 	unsigned int extra;
 	unsigned int tracked;
 };
@@ -1630,7 +1631,7 @@ void hci_conn_failed(struct hci_conn *conn, u8 status);
 u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
 
 void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb);
-void hci_conn_tx_dequeue(struct hci_conn *conn);
+void hci_conn_tx_dequeue(struct hci_conn *conn, bool last);
 void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
 			    const struct sockcm_cookie *sockc);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index a47f5daffdbf..342747a7c225 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1232,6 +1232,9 @@ void hci_conn_del(struct hci_conn *conn)
 	skb_queue_purge(&conn->data_q);
 	skb_queue_purge(&conn->tx_q.queue);
 
+	kfree_skb(conn->tx_q.iso_last_tx);
+	conn->tx_q.iso_last_tx = NULL;
+
 	/* Remove the connection from the list and cleanup its remaining
 	 * state. This is a separate function since for some cases like
 	 * BT_CONNECT_SCAN we *only* want the cleanup part without the
@@ -2388,6 +2391,20 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
 	return cis;
 }
 
+static int hci_conn_le_read_iso_tx_sync(struct hci_conn *conn)
+{
+	struct hci_cp_le_read_iso_tx_sync cp;
+
+	if (!(conn->hdev->commands[41] & 0x40))
+		return -EOPNOTSUPP;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.handle = cpu_to_le16(conn->handle);
+
+	return hci_send_cmd(conn->hdev, HCI_OP_LE_READ_ISO_TX_SYNC,
+			    sizeof(cp), &cp);
+}
+
 /* Check link security requirement */
 int hci_conn_check_link_mode(struct hci_conn *conn)
 {
@@ -3156,6 +3173,18 @@ void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
 	struct tx_queue *comp = &conn->tx_q;
 	bool track = false;
 
+	/* Check for HW timestamping */
+	switch (conn->type) {
+	case CIS_LINK:
+	case BIS_LINK:
+		if (skb->sk &&
+		    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NOBPF)) {
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+			track = true;
+		}
+		break;
+	}
+
 	/* Emit SND now, ie. just before sending to driver */
 	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
 		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
@@ -3214,7 +3243,7 @@ void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
 	skb_queue_purge(&comp->queue);
 }
 
-void hci_conn_tx_dequeue(struct hci_conn *conn)
+void hci_conn_tx_dequeue(struct hci_conn *conn, bool last)
 {
 	struct tx_queue *comp = &conn->tx_q;
 	struct sk_buff *skb;
@@ -3234,8 +3263,24 @@ void hci_conn_tx_dequeue(struct hci_conn *conn)
 
 	if (skb->sk) {
 		comp->tracked--;
-		__skb_tstamp_tx(skb, NULL, NULL, skb->sk,
-				SCM_TSTAMP_COMPLETION);
+
+		if (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP)
+			__skb_tstamp_tx(skb, NULL, NULL, skb->sk,
+					SCM_TSTAMP_COMPLETION);
+
+		switch (conn->type) {
+		case CIS_LINK:
+		case BIS_LINK:
+			if (!last)
+				break;
+			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NOBPF) {
+				kfree_skb(conn->tx_q.iso_last_tx);
+				conn->tx_q.iso_last_tx = skb;
+				hci_conn_le_read_iso_tx_sync(conn);
+				return;
+			}
+			break;
+		}
 	}
 
 	kfree_skb(skb);
@@ -3283,6 +3328,8 @@ int hci_ethtool_ts_info(unsigned int index, int sk_proto,
 
 	switch (sk_proto) {
 	case BTPROTO_ISO:
+		info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE;
+		fallthrough;
 	case BTPROTO_L2CAP:
 		info->so_timestamping |= SOF_TIMESTAMPING_TX_SOFTWARE;
 		info->so_timestamping |= SOF_TIMESTAMPING_TX_COMPLETION;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 286529d2e554..f2aade36a39e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3777,6 +3777,81 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
 	rcu_read_unlock();
 }
 
+static u8 hci_cc_le_read_iso_tx_sync(struct hci_dev *hdev, void *data,
+				     struct sk_buff *skb)
+{
+	struct hci_rp_le_read_iso_tx_sync *rp = data;
+	struct skb_shared_hwtstamps ssh;
+	struct hci_conn *conn;
+	u32 offset;
+
+	bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
+
+	if (rp->status)
+		return rp->status;
+
+	offset = rp->offset[0] | (rp->offset[1] << 8) | (rp->offset[2] << 16);
+	bt_dev_dbg(hdev, "ts %u to %u", __le32_to_cpu(rp->timestamp), offset);
+
+	hci_dev_lock(hdev);
+
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
+	if (!conn)
+		goto unlock;
+
+	if (!conn->tx_q.iso_last_tx)
+		goto unlock;
+
+	/* Report SDU synchronization reference as the timestamp. This value is
+	 * most useful for userspace, as it is the Time_Stamp field in sent
+	 * packets. See Fig. 3.1 and 3.2 in Core v6.2 Vol 6 Part G Sec. 3.
+	 *
+	 * Unfortunately, the ISO-over-HCI transport is poorly described in the
+	 * specification.
+	 *
+	 * The HCI specification contradicts itself which packet timestamp is
+	 * reported by Le Read ISO TX Sync: "scheduled for transmission" in Core
+	 * v6.2 Vol 4 Part E Sec 4.7 vs. "transmitted" in Sec. 7.8.96.
+	 * The specification is also unclear on what these words mean.
+	 *
+	 * We assume here it means "transmitted", and additionally that
+	 * "transmitted" means the last packet reported in Number of Completed
+	 * Packets.
+	 *
+	 * The specification is also unclear what sequence number is returned by
+	 * this command. We assume it is a sequence number assigned by the
+	 * controller. These are equivalent to the controller clock, so they are
+	 * useless for generic Host (which we are) with no direct access to
+	 * controller clock.
+	 *
+	 * Userspace application that handle these timestamps must then be
+	 * prepared that the ISO timestamp is reported on the wrong packet
+	 * depending on controller. Actual packet TX must rely on backpressure
+	 * (monitoring packet completion via COMPLETION tstamp), and the
+	 * timestamps are useful only for aligning the playback of multiple ISO
+	 * streams.
+	 */
+
+	memset(&ssh, 0, sizeof(ssh));
+
+	/* SDU Reference = BIG/CIG Reference - Time_Offset */
+	ssh.hwtstamp = us_to_ktime(__le32_to_cpu(rp->timestamp) - offset);
+
+	/* The value may be 0, but skb_tstamp_tx() does not allow reporting zero
+	 * HW timestamp. Offset by 1 ns as a workaround.
+	 */
+	ssh.hwtstamp += 1;
+
+	skb_tstamp_tx(conn->tx_q.iso_last_tx, &ssh);
+
+	kfree_skb(conn->tx_q.iso_last_tx);
+	conn->tx_q.iso_last_tx = NULL;
+
+unlock:
+	hci_dev_unlock(hdev);
+	return rp->status;
+}
+
 static u8 hci_cc_le_read_buffer_size_v2(struct hci_dev *hdev, void *data,
 					struct sk_buff *skb)
 {
@@ -4224,6 +4299,8 @@ static const struct hci_cc {
 	HCI_CC(HCI_OP_LE_READ_TRANSMIT_POWER, hci_cc_le_read_transmit_power,
 	       sizeof(struct hci_rp_le_read_transmit_power)),
 	HCI_CC_STATUS(HCI_OP_LE_SET_PRIVACY_MODE, hci_cc_le_set_privacy_mode),
+	HCI_CC(HCI_OP_LE_READ_ISO_TX_SYNC, hci_cc_le_read_iso_tx_sync,
+	       sizeof(struct hci_rp_le_read_iso_tx_sync)),
 	HCI_CC(HCI_OP_LE_READ_BUFFER_SIZE_V2, hci_cc_le_read_buffer_size_v2,
 	       sizeof(struct hci_rp_le_read_buffer_size_v2)),
 	HCI_CC_VL(HCI_OP_LE_SET_CIG_PARAMS, hci_cc_le_set_cig_params,
@@ -4509,7 +4586,7 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
 		}
 
 		for (i = 0; i < count; ++i)
-			hci_conn_tx_dequeue(conn);
+			hci_conn_tx_dequeue(conn, (i == count - 1));
 
 		switch (conn->type) {
 		case ACL_LINK:
-- 
2.53.0

Re: [RFC PATCH 2/2] Bluetooth: produce HW TX timestamps for ISO from LE Read ISO TX Sync

[Luiz Augusto von Dentz]

Hi Pauli,

On Wed, Mar 11, 2026 at 12:18 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> For ISO packets with HW TX timestamping enabled, retrieve ISO send
> timestamps via LE Read ISO TX Sync. Specification allows retrieving the
> timestamp only for the "latest" sent packet.
>
> Report SDU Reference time as the timestamp, as this is the value
> relevant for userspace send synchronization.
>
> Due to overall poor description of the timing features in ISO-over-HCI
> transport in Core Specification v6.2, it is ambiguous what packet the
> command actually reports, so timestamps may not be assigned to the
> exactly correct packets, user applications have to deal with it.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  include/net/bluetooth/hci.h      |  2 +-
>  include/net/bluetooth/hci_core.h |  3 +-
>  net/bluetooth/hci_conn.c         | 53 +++++++++++++++++++--
>  net/bluetooth/hci_event.c        | 79 +++++++++++++++++++++++++++++++-
>  4 files changed, 131 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 89ad9470fa71..b536587a909f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -2138,7 +2138,7 @@ struct hci_rp_le_read_iso_tx_sync {
>         __u8    status;
>         __le16  handle;
>         __le16  seq;
> -       __le32  imestamp;
> +       __le32  timestamp;
>         __u8    offset[3];
>  } __packed;
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index c82cec45a9a1..5d530349c2a4 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -272,6 +272,7 @@ struct adv_info {
>
>  struct tx_queue {
>         struct sk_buff_head queue;
> +       struct sk_buff *iso_last_tx;
>         unsigned int extra;
>         unsigned int tracked;
>  };
> @@ -1630,7 +1631,7 @@ void hci_conn_failed(struct hci_conn *conn, u8 status);
>  u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
>
>  void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb);
> -void hci_conn_tx_dequeue(struct hci_conn *conn);
> +void hci_conn_tx_dequeue(struct hci_conn *conn, bool last);
>  void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
>                             const struct sockcm_cookie *sockc);
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index a47f5daffdbf..342747a7c225 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1232,6 +1232,9 @@ void hci_conn_del(struct hci_conn *conn)
>         skb_queue_purge(&conn->data_q);
>         skb_queue_purge(&conn->tx_q.queue);
>
> +       kfree_skb(conn->tx_q.iso_last_tx);
> +       conn->tx_q.iso_last_tx = NULL;
> +
>         /* Remove the connection from the list and cleanup its remaining
>          * state. This is a separate function since for some cases like
>          * BT_CONNECT_SCAN we *only* want the cleanup part without the
> @@ -2388,6 +2391,20 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
>         return cis;
>  }
>
> +static int hci_conn_le_read_iso_tx_sync(struct hci_conn *conn)
> +{
> +       struct hci_cp_le_read_iso_tx_sync cp;
> +
> +       if (!(conn->hdev->commands[41] & 0x40))
> +               return -EOPNOTSUPP;
> +
> +       memset(&cp, 0, sizeof(cp));
> +       cp.handle = cpu_to_le16(conn->handle);
> +
> +       return hci_send_cmd(conn->hdev, HCI_OP_LE_READ_ISO_TX_SYNC,
> +                           sizeof(cp), &cp);
> +}
> +
>  /* Check link security requirement */
>  int hci_conn_check_link_mode(struct hci_conn *conn)
>  {
> @@ -3156,6 +3173,18 @@ void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
>         struct tx_queue *comp = &conn->tx_q;
>         bool track = false;
>
> +       /* Check for HW timestamping */
> +       switch (conn->type) {
> +       case CIS_LINK:
> +       case BIS_LINK:
> +               if (skb->sk &&
> +                   (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NOBPF)) {
> +                       skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +                       track = true;
> +               }
> +               break;
> +       }
> +
>         /* Emit SND now, ie. just before sending to driver */
>         if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
>                 __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
> @@ -3214,7 +3243,7 @@ void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
>         skb_queue_purge(&comp->queue);
>  }
>
> -void hci_conn_tx_dequeue(struct hci_conn *conn)
> +void hci_conn_tx_dequeue(struct hci_conn *conn, bool last)
>  {
>         struct tx_queue *comp = &conn->tx_q;
>         struct sk_buff *skb;
> @@ -3234,8 +3263,24 @@ void hci_conn_tx_dequeue(struct hci_conn *conn)
>
>         if (skb->sk) {
>                 comp->tracked--;
> -               __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> -                               SCM_TSTAMP_COMPLETION);
> +
> +               if (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP)
> +                       __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> +                                       SCM_TSTAMP_COMPLETION);
> +
> +               switch (conn->type) {
> +               case CIS_LINK:
> +               case BIS_LINK:
> +                       if (!last)
> +                               break;
> +                       if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NOBPF) {
> +                               kfree_skb(conn->tx_q.iso_last_tx);
> +                               conn->tx_q.iso_last_tx = skb;
> +                               hci_conn_le_read_iso_tx_sync(conn);
> +                               return;
> +                       }
> +                       break;
> +               }
>         }
>
>         kfree_skb(skb);
> @@ -3283,6 +3328,8 @@ int hci_ethtool_ts_info(unsigned int index, int sk_proto,
>
>         switch (sk_proto) {
>         case BTPROTO_ISO:
> +               info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE;
> +               fallthrough;
>         case BTPROTO_L2CAP:
>                 info->so_timestamping |= SOF_TIMESTAMPING_TX_SOFTWARE;
>                 info->so_timestamping |= SOF_TIMESTAMPING_TX_COMPLETION;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 286529d2e554..f2aade36a39e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3777,6 +3777,81 @@ static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
>         rcu_read_unlock();
>  }
>
> +static u8 hci_cc_le_read_iso_tx_sync(struct hci_dev *hdev, void *data,
> +                                    struct sk_buff *skb)
> +{
> +       struct hci_rp_le_read_iso_tx_sync *rp = data;
> +       struct skb_shared_hwtstamps ssh;
> +       struct hci_conn *conn;
> +       u32 offset;
> +
> +       bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
> +
> +       if (rp->status)
> +               return rp->status;
> +
> +       offset = rp->offset[0] | (rp->offset[1] << 8) | (rp->offset[2] << 16);
> +       bt_dev_dbg(hdev, "ts %u to %u", __le32_to_cpu(rp->timestamp), offset);
> +
> +       hci_dev_lock(hdev);
> +
> +       conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
> +       if (!conn)
> +               goto unlock;
> +
> +       if (!conn->tx_q.iso_last_tx)
> +               goto unlock;
> +
> +       /* Report SDU synchronization reference as the timestamp. This value is
> +        * most useful for userspace, as it is the Time_Stamp field in sent
> +        * packets. See Fig. 3.1 and 3.2 in Core v6.2 Vol 6 Part G Sec. 3.
> +        *
> +        * Unfortunately, the ISO-over-HCI transport is poorly described in the
> +        * specification.
> +        *
> +        * The HCI specification contradicts itself which packet timestamp is
> +        * reported by Le Read ISO TX Sync: "scheduled for transmission" in Core
> +        * v6.2 Vol 4 Part E Sec 4.7 vs. "transmitted" in Sec. 7.8.96.
> +        * The specification is also unclear on what these words mean.
> +        *
> +        * We assume here it means "transmitted", and additionally that
> +        * "transmitted" means the last packet reported in Number of Completed
> +        * Packets.
> +        *
> +        * The specification is also unclear what sequence number is returned by
> +        * this command. We assume it is a sequence number assigned by the
> +        * controller. These are equivalent to the controller clock, so they are
> +        * useless for generic Host (which we are) with no direct access to
> +        * controller clock.

Interesting analysis. I thought the sequence number for NOCP must
match the sequence number set by the host when sending the packet, and
its sole purpose was to be able to fetch the hardware time. Did you
observe something different? If the sequence number had to synchronize
with a clock, the host would need to generate empty packets even when
there is nothing to transmit. This would consume power and memory
solely to keep the sequence number in sync with the clock. I don't
actually understand why NOCP was even used with ISO if the intention
was to use sequence numbers like above, at very least NOCP should have
been updated to carry the hardware clock and other timing information
directly rather than using another command that consumes even more
system power and memory.

> +        *
> +        * Userspace application that handle these timestamps must then be
> +        * prepared that the ISO timestamp is reported on the wrong packet
> +        * depending on controller. Actual packet TX must rely on backpressure
> +        * (monitoring packet completion via COMPLETION tstamp), and the
> +        * timestamps are useful only for aligning the playback of multiple ISO
> +        * streams.
> +        */
> +
> +       memset(&ssh, 0, sizeof(ssh));
> +
> +       /* SDU Reference = BIG/CIG Reference - Time_Offset */
> +       ssh.hwtstamp = us_to_ktime(__le32_to_cpu(rp->timestamp) - offset);
> +
> +       /* The value may be 0, but skb_tstamp_tx() does not allow reporting zero
> +        * HW timestamp. Offset by 1 ns as a workaround.
> +        */
> +       ssh.hwtstamp += 1;
> +
> +       skb_tstamp_tx(conn->tx_q.iso_last_tx, &ssh);
> +
> +       kfree_skb(conn->tx_q.iso_last_tx);
> +       conn->tx_q.iso_last_tx = NULL;
> +
> +unlock:
> +       hci_dev_unlock(hdev);
> +       return rp->status;
> +}
> +
>  static u8 hci_cc_le_read_buffer_size_v2(struct hci_dev *hdev, void *data,
>                                         struct sk_buff *skb)
>  {
> @@ -4224,6 +4299,8 @@ static const struct hci_cc {
>         HCI_CC(HCI_OP_LE_READ_TRANSMIT_POWER, hci_cc_le_read_transmit_power,
>                sizeof(struct hci_rp_le_read_transmit_power)),
>         HCI_CC_STATUS(HCI_OP_LE_SET_PRIVACY_MODE, hci_cc_le_set_privacy_mode),
> +       HCI_CC(HCI_OP_LE_READ_ISO_TX_SYNC, hci_cc_le_read_iso_tx_sync,
> +              sizeof(struct hci_rp_le_read_iso_tx_sync)),
>         HCI_CC(HCI_OP_LE_READ_BUFFER_SIZE_V2, hci_cc_le_read_buffer_size_v2,
>                sizeof(struct hci_rp_le_read_buffer_size_v2)),
>         HCI_CC_VL(HCI_OP_LE_SET_CIG_PARAMS, hci_cc_le_set_cig_params,
> @@ -4509,7 +4586,7 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
>                 }
>
>                 for (i = 0; i < count; ++i)
> -                       hci_conn_tx_dequeue(conn);
> +                       hci_conn_tx_dequeue(conn, (i == count - 1));
>
>                 switch (conn->type) {
>                 case ACL_LINK:
> --
> 2.53.0
>
>


-- 
Luiz Augusto von Dentz

Re: [RFC PATCH 2/2] Bluetooth: produce HW TX timestamps for ISO from LE Read ISO TX Sync

[Pauli Virtanen]

Hi Luiz,

ke, 2026-03-11 kello 16:30 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Wed, Mar 11, 2026 at 12:18 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > For ISO packets with HW TX timestamping enabled, retrieve ISO send
> > timestamps via LE Read ISO TX Sync. Specification allows retrieving
> > the
> > timestamp only for the "latest" sent packet.
> > 
> > Report SDU Reference time as the timestamp, as this is the value
> > relevant for userspace send synchronization.
> > 
> > Due to overall poor description of the timing features in ISO-over-
> > HCI
> > transport in Core Specification v6.2, it is ambiguous what packet
> > the
> > command actually reports, so timestamps may not be assigned to the
> > exactly correct packets, user applications have to deal with it.
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  include/net/bluetooth/hci.h      |  2 +-
> >  include/net/bluetooth/hci_core.h |  3 +-
> >  net/bluetooth/hci_conn.c         | 53 +++++++++++++++++++--
> >  net/bluetooth/hci_event.c        | 79
> > +++++++++++++++++++++++++++++++-
> >  4 files changed, 131 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci.h
> > b/include/net/bluetooth/hci.h
> > index 89ad9470fa71..b536587a909f 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -2138,7 +2138,7 @@ struct hci_rp_le_read_iso_tx_sync {
> >         __u8    status;
> >         __le16  handle;
> >         __le16  seq;
> > -       __le32  imestamp;
> > +       __le32  timestamp;
> >         __u8    offset[3];
> >  } __packed;
> > 
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index c82cec45a9a1..5d530349c2a4 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -272,6 +272,7 @@ struct adv_info {
> > 
> >  struct tx_queue {
> >         struct sk_buff_head queue;
> > +       struct sk_buff *iso_last_tx;
> >         unsigned int extra;
> >         unsigned int tracked;
> >  };
> > @@ -1630,7 +1631,7 @@ void hci_conn_failed(struct hci_conn *conn,
> > u8 status);
> >  u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
> > 
> >  void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff
> > *skb);
> > -void hci_conn_tx_dequeue(struct hci_conn *conn);
> > +void hci_conn_tx_dequeue(struct hci_conn *conn, bool last);
> >  void hci_setup_tx_timestamp(struct sk_buff *skb, size_t
> > key_offset,
> >                             const struct sockcm_cookie *sockc);
> > 
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index a47f5daffdbf..342747a7c225 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1232,6 +1232,9 @@ void hci_conn_del(struct hci_conn *conn)
> >         skb_queue_purge(&conn->data_q);
> >         skb_queue_purge(&conn->tx_q.queue);
> > 
> > +       kfree_skb(conn->tx_q.iso_last_tx);
> > +       conn->tx_q.iso_last_tx = NULL;
> > +
> >         /* Remove the connection from the list and cleanup its
> > remaining
> >          * state. This is a separate function since for some cases
> > like
> >          * BT_CONNECT_SCAN we *only* want the cleanup part without
> > the
> > @@ -2388,6 +2391,20 @@ struct hci_conn *hci_connect_cis(struct
> > hci_dev *hdev, bdaddr_t *dst,
> >         return cis;
> >  }
> > 
> > +static int hci_conn_le_read_iso_tx_sync(struct hci_conn *conn)
> > +{
> > +       struct hci_cp_le_read_iso_tx_sync cp;
> > +
> > +       if (!(conn->hdev->commands[41] & 0x40))
> > +               return -EOPNOTSUPP;
> > +
> > +       memset(&cp, 0, sizeof(cp));
> > +       cp.handle = cpu_to_le16(conn->handle);
> > +
> > +       return hci_send_cmd(conn->hdev, HCI_OP_LE_READ_ISO_TX_SYNC,
> > +                           sizeof(cp), &cp);
> > +}
> > +
> >  /* Check link security requirement */
> >  int hci_conn_check_link_mode(struct hci_conn *conn)
> >  {
> > @@ -3156,6 +3173,18 @@ void hci_conn_tx_queue(struct hci_conn
> > *conn, struct sk_buff *skb)
> >         struct tx_queue *comp = &conn->tx_q;
> >         bool track = false;
> > 
> > +       /* Check for HW timestamping */
> > +       switch (conn->type) {
> > +       case CIS_LINK:
> > +       case BIS_LINK:
> > +               if (skb->sk &&
> > +                   (skb_shinfo(skb)->tx_flags &
> > SKBTX_HW_TSTAMP_NOBPF)) {
> > +                       skb_shinfo(skb)->tx_flags |=
> > SKBTX_IN_PROGRESS;
> > +                       track = true;
> > +               }
> > +               break;
> > +       }
> > +
> >         /* Emit SND now, ie. just before sending to driver */
> >         if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
> >                 __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> > SCM_TSTAMP_SND);
> > @@ -3214,7 +3243,7 @@ void hci_conn_tx_queue(struct hci_conn *conn,
> > struct sk_buff *skb)
> >         skb_queue_purge(&comp->queue);
> >  }
> > 
> > -void hci_conn_tx_dequeue(struct hci_conn *conn)
> > +void hci_conn_tx_dequeue(struct hci_conn *conn, bool last)
> >  {
> >         struct tx_queue *comp = &conn->tx_q;
> >         struct sk_buff *skb;
> > @@ -3234,8 +3263,24 @@ void hci_conn_tx_dequeue(struct hci_conn
> > *conn)
> > 
> >         if (skb->sk) {
> >                 comp->tracked--;
> > -               __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> > -                               SCM_TSTAMP_COMPLETION);
> > +
> > +               if (skb_shinfo(skb)->tx_flags &
> > SKBTX_COMPLETION_TSTAMP)
> > +                       __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> > +                                       SCM_TSTAMP_COMPLETION);
> > +
> > +               switch (conn->type) {
> > +               case CIS_LINK:
> > +               case BIS_LINK:
> > +                       if (!last)
> > +                               break;
> > +                       if (skb_shinfo(skb)->tx_flags &
> > SKBTX_HW_TSTAMP_NOBPF) {
> > +                               kfree_skb(conn->tx_q.iso_last_tx);
> > +                               conn->tx_q.iso_last_tx = skb;
> > +                               hci_conn_le_read_iso_tx_sync(conn);
> > +                               return;
> > +                       }
> > +                       break;
> > +               }
> >         }
> > 
> >         kfree_skb(skb);
> > @@ -3283,6 +3328,8 @@ int hci_ethtool_ts_info(unsigned int index,
> > int sk_proto,
> > 
> >         switch (sk_proto) {
> >         case BTPROTO_ISO:
> > +               info->so_timestamping |=
> > SOF_TIMESTAMPING_TX_HARDWARE;
> > +               fallthrough;
> >         case BTPROTO_L2CAP:
> >                 info->so_timestamping |=
> > SOF_TIMESTAMPING_TX_SOFTWARE;
> >                 info->so_timestamping |=
> > SOF_TIMESTAMPING_TX_COMPLETION;
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 286529d2e554..f2aade36a39e 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3777,6 +3777,81 @@ static inline void
> > handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
> >         rcu_read_unlock();
> >  }
> > 
> > +static u8 hci_cc_le_read_iso_tx_sync(struct hci_dev *hdev, void
> > *data,
> > +                                    struct sk_buff *skb)
> > +{
> > +       struct hci_rp_le_read_iso_tx_sync *rp = data;
> > +       struct skb_shared_hwtstamps ssh;
> > +       struct hci_conn *conn;
> > +       u32 offset;
> > +
> > +       bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
> > +
> > +       if (rp->status)
> > +               return rp->status;
> > +
> > +       offset = rp->offset[0] | (rp->offset[1] << 8) | (rp-
> > >offset[2] << 16);
> > +       bt_dev_dbg(hdev, "ts %u to %u", __le32_to_cpu(rp-
> > >timestamp), offset);
> > +
> > +       hci_dev_lock(hdev);
> > +
> > +       conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp-
> > >handle));
> > +       if (!conn)
> > +               goto unlock;
> > +
> > +       if (!conn->tx_q.iso_last_tx)
> > +               goto unlock;
> > +
> > +       /* Report SDU synchronization reference as the timestamp.
> > This value is
> > +        * most useful for userspace, as it is the Time_Stamp field
> > in sent
> > +        * packets. See Fig. 3.1 and 3.2 in Core v6.2 Vol 6 Part G
> > Sec. 3.
> > +        *
> > +        * Unfortunately, the ISO-over-HCI transport is poorly
> > described in the
> > +        * specification.
> > +        *
> > +        * The HCI specification contradicts itself which packet
> > timestamp is
> > +        * reported by Le Read ISO TX Sync: "scheduled for
> > transmission" in Core
> > +        * v6.2 Vol 4 Part E Sec 4.7 vs. "transmitted" in Sec.
> > 7.8.96.
> > +        * The specification is also unclear on what these words
> > mean.
> > +        *
> > +        * We assume here it means "transmitted", and additionally
> > that
> > +        * "transmitted" means the last packet reported in Number
> > of Completed
> > +        * Packets.
> > +        *
> > +        * The specification is also unclear what sequence number
> > is returned by
> > +        * this command. We assume it is a sequence number assigned
> > by the
> > +        * controller. These are equivalent to the controller
> > clock, so they are
> > +        * useless for generic Host (which we are) with no direct
> > access to
> > +        * controller clock.
> 
> Interesting analysis. I thought the sequence number for NOCP must
> match the sequence number set by the host when sending the packet,
> and
> its sole purpose was to be able to fetch the hardware time. Did you
> observe something different? If the sequence number had to
> synchronize
> with a clock, the host would need to generate empty packets even when
> there is nothing to transmit. This would consume power and memory
> solely to keep the sequence number in sync with the clock. I don't
> actually understand why NOCP was even used with ISO if the intention
> was to use sequence numbers like above, at very least NOCP should
> have
> been updated to carry the hardware clock and other timing information
> directly rather than using another command that consumes even more
> system power and memory.

There's no relationship to NOCP specified in Core Spec, it's pretty
vague. However, I don't see any other way for host to get clock sync
for TX than via NOCP (at least for <1 packet accuracy).

It would indeed make most sense that sequence number is what the host
sent in the ISO packet, but it is not specified. Core Specification
v6.2 Vol 6 Part G Sec 2 also says:

"""
SDUs sent to the upper layer by the ISOAL shall be given a sequence
number which is initialized to 0 when the CIS or BIS is created. SDUs
received by the ISOAL from the upper layer shall be given a sequence
number which is initialized to 0 when the CIS or BIS is created. The
upper layer may synchronize its sequence number with the sequence
number in the ISOAL once the Datapath is configured and the link is
established.
"""

so one gets the impression controller shall assign new sequence numbers
and use those, and host shall "synchronize" to it in some unspecified
way.

I saw some related discussions by the Zephyr people, I think the
outcome was along the lines that the spec is so vague that these
features are unusable.

Some clarifications would be useful here, as I don't get what was the
way the spec intends that these are used.

***

IIRC, in the controllers I tested (Intel AX210, Realtek 8761CU) the
returned timestamp and sequence number are always zero, so they haven't
actually implemented the command. I'd have to check what Mediatek
MT7925 does, it was at least setting RX timestamps properly.

In Zephyr IIRC controller-assigned sequence number is used, but I'd
have to double check this.


> > +        *
> > +        * Userspace application that handle these timestamps must
> > then be
> > +        * prepared that the ISO timestamp is reported on the wrong
> > packet
> > +        * depending on controller. Actual packet TX must rely on
> > backpressure
> > +        * (monitoring packet completion via COMPLETION tstamp),
> > and the
> > +        * timestamps are useful only for aligning the playback of
> > multiple ISO
> > +        * streams.
> > +        */
> > +
> > +       memset(&ssh, 0, sizeof(ssh));
> > +
> > +       /* SDU Reference = BIG/CIG Reference - Time_Offset */
> > +       ssh.hwtstamp = us_to_ktime(__le32_to_cpu(rp->timestamp) -
> > offset);
> > +
> > +       /* The value may be 0, but skb_tstamp_tx() does not allow
> > reporting zero
> > +        * HW timestamp. Offset by 1 ns as a workaround.
> > +        */
> > +       ssh.hwtstamp += 1;
> > +
> > +       skb_tstamp_tx(conn->tx_q.iso_last_tx, &ssh);
> > +
> > +       kfree_skb(conn->tx_q.iso_last_tx);
> > +       conn->tx_q.iso_last_tx = NULL;
> > +
> > +unlock:
> > +       hci_dev_unlock(hdev);
> > +       return rp->status;
> > +}
> > +
> >  static u8 hci_cc_le_read_buffer_size_v2(struct hci_dev *hdev, void
> > *data,
> >                                         struct sk_buff *skb)
> >  {
> > @@ -4224,6 +4299,8 @@ static const struct hci_cc {
> >         HCI_CC(HCI_OP_LE_READ_TRANSMIT_POWER,
> > hci_cc_le_read_transmit_power,
> >                sizeof(struct hci_rp_le_read_transmit_power)),
> >         HCI_CC_STATUS(HCI_OP_LE_SET_PRIVACY_MODE,
> > hci_cc_le_set_privacy_mode),
> > +       HCI_CC(HCI_OP_LE_READ_ISO_TX_SYNC,
> > hci_cc_le_read_iso_tx_sync,
> > +              sizeof(struct hci_rp_le_read_iso_tx_sync)),
> >         HCI_CC(HCI_OP_LE_READ_BUFFER_SIZE_V2,
> > hci_cc_le_read_buffer_size_v2,
> >                sizeof(struct hci_rp_le_read_buffer_size_v2)),
> >         HCI_CC_VL(HCI_OP_LE_SET_CIG_PARAMS,
> > hci_cc_le_set_cig_params,
> > @@ -4509,7 +4586,7 @@ static void hci_num_comp_pkts_evt(struct
> > hci_dev *hdev, void *data,
> >                 }
> > 
> >                 for (i = 0; i < count; ++i)
> > -                       hci_conn_tx_dequeue(conn);
> > +                       hci_conn_tx_dequeue(conn, (i == count -
> > 1));
> > 
> >                 switch (conn->type) {
> >                 case ACL_LINK:
> > --
> > 2.53.0
> > 
> > 
> 

Re: [RFC PATCH 2/2] Bluetooth: produce HW TX timestamps for ISO from LE Read ISO TX Sync

[Pauli Virtanen]

ke, 2026-03-11 kello 23:19 +0200, Pauli Virtanen kirjoitti:
> Hi Luiz,
[clip]
> > Interesting analysis. I thought the sequence number for NOCP must
> > match the sequence number set by the host when sending the packet,
> > and
> > its sole purpose was to be able to fetch the hardware time. Did you
> > observe something different? If the sequence number had to
> > synchronize
> > with a clock, the host would need to generate empty packets even when
> > there is nothing to transmit. This would consume power and memory
> > solely to keep the sequence number in sync with the clock. I don't
> > actually understand why NOCP was even used with ISO if the intention
> > was to use sequence numbers like above, at very least NOCP should
> > have
> > been updated to carry the hardware clock and other timing information
> > directly rather than using another command that consumes even more
> > system power and memory.
> 
> There's no relationship to NOCP specified in Core Spec, it's pretty
> vague. However, I don't see any other way for host to get clock sync
> for TX than via NOCP (at least for <1 packet accuracy).
> 
> It would indeed make most sense that sequence number is what the host
> sent in the ISO packet, but it is not specified. Core Specification
> v6.2 Vol 6 Part G Sec 2 also says:
> 
> """
> SDUs sent to the upper layer by the ISOAL shall be given a sequence
> number which is initialized to 0 when the CIS or BIS is created. SDUs
> received by the ISOAL from the upper layer shall be given a sequence
> number which is initialized to 0 when the CIS or BIS is created. The
> upper layer may synchronize its sequence number with the sequence
> number in the ISOAL once the Datapath is configured and the link is
> established.
> """
> 
> so one gets the impression controller shall assign new sequence numbers
> and use those, and host shall "synchronize" to it in some unspecified
> way.
> 
> I saw some related discussions by the Zephyr people, I think the
> outcome was along the lines that the spec is so vague that these
> features are unusable.
> 
> Some clarifications would be useful here, as I don't get what was the
> way the spec intends that these are used.
> 
> ***
> 
> IIRC, in the controllers I tested (Intel AX210, Realtek 8761CU) the
> returned timestamp and sequence number are always zero, so they haven't
> actually implemented the command. I'd have to check what Mediatek
> MT7925 does, it was at least setting RX timestamps properly.
> 
> In Zephyr IIRC controller-assigned sequence number is used, but I'd
> have to double check this.

Retest:

MT7925 firmware produces LE Read ISO TX Sync responses with sequence
numbers from host ISO packets, but timestamp is always 0.

RTL8176CU firmware also produces responses with sequence numbers from
host ISO packets, but timestamp is always 0.

Intel AX210 produces responses with both sequence number and timestamp
0.

Zephyr produces responses with controller's own sequence numbering,
host ISO packet seqnum is only used for determining skipped packets.
Provides timestamps.


MT/RTL/Intel seem to have same interpretation although none of them
actually implements the command.


> > > +        *
> > > +        * Userspace application that handle these timestamps must
> > > then be
> > > +        * prepared that the ISO timestamp is reported on the wrong
> > > packet
> > > +        * depending on controller. Actual packet TX must rely on
> > > backpressure
> > > +        * (monitoring packet completion via COMPLETION tstamp),
> > > and the
> > > +        * timestamps are useful only for aligning the playback of
> > > multiple ISO
> > > +        * streams.
> > > +        */
> > > +
> > > +       memset(&ssh, 0, sizeof(ssh));
> > > +
> > > +       /* SDU Reference = BIG/CIG Reference - Time_Offset */
> > > +       ssh.hwtstamp = us_to_ktime(__le32_to_cpu(rp->timestamp) -
> > > offset);
> > > +
> > > +       /* The value may be 0, but skb_tstamp_tx() does not allow
> > > reporting zero
> > > +        * HW timestamp. Offset by 1 ns as a workaround.
> > > +        */
> > > +       ssh.hwtstamp += 1;
> > > +
> > > +       skb_tstamp_tx(conn->tx_q.iso_last_tx, &ssh);
> > > +
> > > +       kfree_skb(conn->tx_q.iso_last_tx);
> > > +       conn->tx_q.iso_last_tx = NULL;
> > > +
> > > +unlock:
> > > +       hci_dev_unlock(hdev);
> > > +       return rp->status;
> > > +}
> > > +
> > >  static u8 hci_cc_le_read_buffer_size_v2(struct hci_dev *hdev, void
> > > *data,
> > >                                         struct sk_buff *skb)
> > >  {
> > > @@ -4224,6 +4299,8 @@ static const struct hci_cc {
> > >         HCI_CC(HCI_OP_LE_READ_TRANSMIT_POWER,
> > > hci_cc_le_read_transmit_power,
> > >                sizeof(struct hci_rp_le_read_transmit_power)),
> > >         HCI_CC_STATUS(HCI_OP_LE_SET_PRIVACY_MODE,
> > > hci_cc_le_set_privacy_mode),
> > > +       HCI_CC(HCI_OP_LE_READ_ISO_TX_SYNC,
> > > hci_cc_le_read_iso_tx_sync,
> > > +              sizeof(struct hci_rp_le_read_iso_tx_sync)),
> > >         HCI_CC(HCI_OP_LE_READ_BUFFER_SIZE_V2,
> > > hci_cc_le_read_buffer_size_v2,
> > >                sizeof(struct hci_rp_le_read_buffer_size_v2)),
> > >         HCI_CC_VL(HCI_OP_LE_SET_CIG_PARAMS,
> > > hci_cc_le_set_cig_params,
> > > @@ -4509,7 +4586,7 @@ static void hci_num_comp_pkts_evt(struct
> > > hci_dev *hdev, void *data,
> > >                 }
> > > 
> > >                 for (i = 0; i < count; ++i)
> > > -                       hci_conn_tx_dequeue(conn);
> > > +                       hci_conn_tx_dequeue(conn, (i == count -
> > > 1));
> > > 
> > >                 switch (conn->type) {
> > >                 case ACL_LINK:
> > > --
> > > 2.53.0
> > > 
> > > 
> > 

-- 
Pauli Virtanen