* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:14 ` [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet tailu.shi
@ 2026-03-11 8:54 ` Paul Menzel
2026-03-12 9:38 ` Tailu Shi
2026-03-11 16:24 ` Pauli Virtanen
2026-04-09 20:01 ` bluez.test.bot
2 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2026-03-11 8:54 UTC (permalink / raw)
To: Tailu Shi
Cc: Marcel Holtmann, Luiz Augusto von Dentz, Pauli Virtanen,
Johan Hedberg, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-bluetooth, netdev, linux-kernel
Dear Tailu,
Thank you for your patch. A formal request at the beginning to please
spell your name without the dot/period – maybe: Tailu Shi.
git config --global user.name "Tailu Shi"
git commit --amend -s --author="Tailu Shi <tailu.shi@samsung.com>"
Am 11.03.26 um 09:14 schrieb tailu.shi:
> The Bluetooth Core Specification defines the 'Time_Stamp' field of
> an HCI ISO packet as optional. However, it's mandatory to include
> the 'Time_Stamp' field when using HCI ISO packets per section 3.3
> 'HCI Feature Support Requirement' of TMAP specification.
> https://www.bluetooth.com/specifications/specs/html/?src=TMAP_v1.0.1/out/en/index-en.html#UUID-68138fa8-fd1f-1a1b-e552-ddcd8fede9ec
>
> To comply with TMAP, introduce a new socket option BT_PKT_ISO_TIMESTAMP
> that allows user application (e.g. PipeWire) to attach timestamp to the
> data it sends to the ISO socket within BT_SCM_PKT_ISO_TIMESTAMP CMSG.
>
> When the option is enabled, the kernel extracts the timestamp and copies
> it into the 'Time_Stamp' field of the outgoing HCI ISO packet.
>
> This also gives the controller the reference timing information
> required for ISO stream synchronization.
>
> A corresponding userspace change is required for full functionality.
> A reference implementation is available in PipeWire:
> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
Is the timestamp correctly visible in the btmon trace?
> Signed-off-by: tailu.shi <tailu.shi@samsung.com>
> ---
> include/net/bluetooth/bluetooth.h | 5 ++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/iso.c | 49 +++++++++++++++++++++++++++----
> 4 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 69eed69f7f26..f88c83d21c5e 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -259,6 +259,10 @@ struct bt_codecs {
>
> #define BT_SCM_PKT_SEQNUM 0x05
>
> +#define BT_PKT_ISO_TIMESTAMP 23
> +
> +#define BT_SCM_PKT_ISO_TIMESTAMP 0x06
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -409,6 +413,7 @@ enum {
> BT_SK_SUSPEND,
> BT_SK_PKT_STATUS,
> BT_SK_PKT_SEQNUM,
> + BT_SK_PKT_ISO_TIMESTAMP,
> };
>
> struct bt_sock_list {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a7bffb908c1e..e39d192f8ef6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -740,6 +740,7 @@ struct hci_conn {
> struct bt_iso_qos iso_qos;
> __u8 num_bis;
> __u8 bis[HCI_MAX_ISO_BIS];
> + bool iso_pkt_ts;
>
> unsigned long flags;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 31308c1de4ec..4264c0229dfb 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -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, conn->iso_pkt_ts ? 0x01 : 0x00);
> hci_add_iso_hdr(skb, conn->handle, flags);
>
> if (!list) {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index be145e2736b7..dd4d198bab4c 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -73,6 +73,9 @@ struct iso_pinfo {
> __u8 base_len;
> __u8 base[BASE_MAX_LENGTH];
> struct iso_conn *conn;
> + bool ts_flag;
> + __u32 ts;
> +
> };
>
> static struct bt_iso_qos default_qos;
> @@ -542,6 +545,7 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> struct iso_conn *conn = iso_pi(sk)->conn;
> struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> struct hci_iso_data_hdr *hdr;
> + struct hci_iso_ts_data_hdr *hdr_ts;
> int len = 0;
>
> BT_DBG("sk %p len %d", sk, skb->len);
> @@ -552,13 +556,20 @@ 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));
> + if (iso_pi(sk)->ts_flag) {
> + hdr_ts = skb_push(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> + hdr_ts->ts = cpu_to_le32(iso_pi(sk)->ts);
> + hdr_ts->sn = cpu_to_le16(conn->tx_sn++);
> + hdr_ts->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + } else {
> + 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));
> + }
>
> if (sk->sk_state == BT_CONNECTED) {
> hci_setup_tx_timestamp(skb, 1, sockc);
> + conn->hcon->iso_pkt_ts = iso_pi(sk)->ts_flag;
> hci_send_iso(conn->hcon, skb);
> } else {
> len = -ENOTCONN;
> @@ -1471,7 +1482,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sock *sk = sock->sk;
> struct sk_buff *skb, **frag;
> struct sockcm_cookie sockc;
> - size_t mtu;
> + struct cmsghdr *cm;
> + size_t mtu, hlen;
> int err;
>
> BT_DBG("sock %p, sk %p", sock, sk);
> @@ -1485,10 +1497,23 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> hci_sockcm_init(&sockc, sk);
>
> + iso_pi(sk)->ts_flag = false;
> if (msg->msg_controllen) {
> err = sock_cmsg_send(sk, msg, &sockc);
> if (err)
> return err;
> +
> + for (cm = CMSG_FIRSTHDR(msg); cm; cm = CMSG_NXTHDR(msg, cm)) {
> + if (cm->cmsg_level != SOL_BLUETOOTH)
> + continue;
> + if (test_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags) &&
> + cm->cmsg_type == BT_SCM_PKT_ISO_TIMESTAMP &&
> + cm->cmsg_len == CMSG_LEN(sizeof(u32))) {
> + iso_pi(sk)->ts_flag = true;
> + iso_pi(sk)->ts = *(u32 *)CMSG_DATA(cm);
> + break;
> + }
> + }
> }
>
> lock_sock(sk);
> @@ -1502,7 +1527,8 @@ 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);
> + hlen = iso_pi(sk)->ts_flag ? HCI_ISO_TS_DATA_HDR_SIZE : HCI_ISO_DATA_HDR_SIZE;
> + skb = bt_skb_sendmsg(sk, msg, len, mtu, hlen, 0);
> if (IS_ERR(skb))
> return PTR_ERR(skb);
>
> @@ -1840,6 +1866,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>
> break;
>
> + case BT_PKT_ISO_TIMESTAMP:
> + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> + if (err)
> + break;
> +
> + if (opt)
> + set_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + else
> + clear_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
The diff looks good.
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:54 ` Paul Menzel
@ 2026-03-12 9:38 ` Tailu Shi
0 siblings, 0 replies; 7+ messages in thread
From: Tailu Shi @ 2026-03-12 9:38 UTC (permalink / raw)
To: Tailu Shi, Paul Menzel
Cc: Marcel Holtmann, Luiz Augusto von Dentz, Pauli Virtanen,
Johan Hedberg, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-bluetooth, netdev, linux-kernel
Hi Paul,
Thank you for the review and the helpful suggestion about my name.
I have updated my Git configuration and amended the commit as you advised:
git config --global user.name "Tailu Shi"
git commit --amend -s --author="Tailu Shi <tailu.shi@samsung.com>"
On Wed, 11 Mar 2026 09:54:38 +0100 Paul Menzel wrote:
>> A corresponding userspace change is required for full functionality.
>> A reference implementation is available in PipeWire:
>> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
>
> Is the timestamp correctly visible in the btmon trace?
Yes, it can be seen in the btmon trace. The test was performed on the
kernel 7.0.0‑rc1+, using BlueZ 5.85 and PipeWire 1.5.84 with the
'bluez5.iso-tx-timestamp' configuration option enabled.
But seems the btmon v5.85 and v5.86 fails to parse it directly with
the command. However, the 'ts' field can be seen clearly with Wireshark.
The things are like below:
1) btmon v5.86
$ btmon -r dump.btsnoop -I |grep -A1 'ISO Data TX: Handle 2563'
< ISO Data TX: Handle 2563 [5/18] SN 2 #10999 [hci0] 128.571172
02 00 3c 00 00 00 00 00 00 00 00 00 00 00 00 00 ..<.............
--
2) Wireshark
Frame 19604: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
Bluetooth
Bluetooth Linux Monitor Transport
Bluetooth HCI ISO Packet
.... 1010 0000 0011 = Connection Handle: 0xa03
..10 .... .... .... = PB Flag: Complete SDU (0x2)
.1.. .... .... .... = Timestamp present: True
0... .... .... .... = Reserved: 0x0
..00 0000 0100 0100 = Data Length: 68
Data
[Connect in frame: 18836]
Bluetooth ISO Data
Timestamp: 805103660µs
Sequence Number: 84
.... 0000 0011 1100 = SDU Length: 60
SDU
Raw data:
0000 03 6a 44 00 2c e8 fc 2f 54 00 3c 00 00 00 00 00 .jD.,../T.<.....
0010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0040 38 24 f9 4a 0d 00 00 03 8$.J....
Thanks for your time again!
Best regards,
Tailu Shi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:14 ` [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet tailu.shi
2026-03-11 8:54 ` Paul Menzel
@ 2026-03-11 16:24 ` Pauli Virtanen
2026-03-13 12:01 ` Tailu Shi
2026-04-09 20:01 ` bluez.test.bot
2 siblings, 1 reply; 7+ messages in thread
From: Pauli Virtanen @ 2026-03-11 16:24 UTC (permalink / raw)
To: tailu.shi, Marcel Holtmann, Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi,
ke, 2026-03-11 kello 16:14 +0800, tailu.shi kirjoitti:
> The Bluetooth Core Specification defines the 'Time_Stamp' field of
> an HCI ISO packet as optional. However, it's mandatory to include
> the 'Time_Stamp' field when using HCI ISO packets per section 3.3
> 'HCI Feature Support Requirement' of TMAP specification.
> https://www.bluetooth.com/specifications/specs/html/?src=TMAP_v1.0.1/out/en/index-en.html#UUID-68138fa8-fd1f-1a1b-e552-ddcd8fede9ec
>
> To comply with TMAP, introduce a new socket option BT_PKT_ISO_TIMESTAMP
> that allows user application (e.g. PipeWire) to attach timestamp to the
> data it sends to the ISO socket within BT_SCM_PKT_ISO_TIMESTAMP CMSG.
>
> When the option is enabled, the kernel extracts the timestamp and copies
> it into the 'Time_Stamp' field of the outgoing HCI ISO packet.
>
> This also gives the controller the reference timing information
> required for ISO stream synchronization.
These timestamps are in controller clock domain, so generic host like
Linux appears to need to perform clock synchronization via
HCI_OP_LE_READ_ISO_TX_SYNC to be able to provide timestamps.
I had in the works a not fully finished patch series adding the same
thing + LE Read ISO TX Sync + tests. I sent these now to the list in
their current state, maybe they are useful if you are working on this
https://lore.kernel.org/linux-bluetooth/cover.1773245666.git.pav@iki.fi/T/
https://lore.kernel.org/linux-bluetooth/cover.1773245672.git.pav@iki.fi/T/
The tests pass, but not really tested to satisfaction on real
controllers.
From what I saw, it seems several controllers like Intel AX210 and some
Realtek ones don't implement LE Read ISO TX Sync correctly (returns 0
timestamp), so in those cases it AFAICS it's not possible to provide
host timestamps.
Getting the clock sync via LE Read ISO TX Sync to work properly across
controllers is maybe not so easy, as it appears the Core Specification
is somewhat unclear how it should work.
> A corresponding userspace change is required for full functionality.
> A reference implementation is available in PipeWire:
> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
This sends timestamps in host clock domain and has no clock sync, but I
guess this is just for testing.
> Signed-off-by: tailu.shi <tailu.shi@samsung.com>
> ---
> include/net/bluetooth/bluetooth.h | 5 ++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/iso.c | 49 +++++++++++++++++++++++++++----
> 4 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 69eed69f7f26..f88c83d21c5e 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -259,6 +259,10 @@ struct bt_codecs {
>
> #define BT_SCM_PKT_SEQNUM 0x05
>
> +#define BT_PKT_ISO_TIMESTAMP 23
> +
> +#define BT_SCM_PKT_ISO_TIMESTAMP 0x06
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -409,6 +413,7 @@ enum {
> BT_SK_SUSPEND,
> BT_SK_PKT_STATUS,
> BT_SK_PKT_SEQNUM,
> + BT_SK_PKT_ISO_TIMESTAMP,
> };
>
> struct bt_sock_list {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a7bffb908c1e..e39d192f8ef6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -740,6 +740,7 @@ struct hci_conn {
> struct bt_iso_qos iso_qos;
> __u8 num_bis;
> __u8 bis[HCI_MAX_ISO_BIS];
> + bool iso_pkt_ts;
>
> unsigned long flags;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 31308c1de4ec..4264c0229dfb 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -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, conn->iso_pkt_ts ? 0x01 : 0x00);
> hci_add_iso_hdr(skb, conn->handle, flags);
>
> if (!list) {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index be145e2736b7..dd4d198bab4c 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -73,6 +73,9 @@ struct iso_pinfo {
> __u8 base_len;
> __u8 base[BASE_MAX_LENGTH];
> struct iso_conn *conn;
> + bool ts_flag;
> + __u32 ts;
I think these are not needed, they can be passed as arguments to
iso_send_frame().
Adding them to iso_pinfo() probably causes a data race if two processes
sendmsg() at the same time.
> +
> };
>
> static struct bt_iso_qos default_qos;
> @@ -542,6 +545,7 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> struct iso_conn *conn = iso_pi(sk)->conn;
> struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> struct hci_iso_data_hdr *hdr;
> + struct hci_iso_ts_data_hdr *hdr_ts;
> int len = 0;
>
> BT_DBG("sk %p len %d", sk, skb->len);
> @@ -552,13 +556,20 @@ 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));
> + if (iso_pi(sk)->ts_flag) {
> + hdr_ts = skb_push(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> + hdr_ts->ts = cpu_to_le32(iso_pi(sk)->ts);
> + hdr_ts->sn = cpu_to_le16(conn->tx_sn++);
> + hdr_ts->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + } else {
> + 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));
> + }
>
> if (sk->sk_state == BT_CONNECTED) {
> hci_setup_tx_timestamp(skb, 1, sockc);
> + conn->hcon->iso_pkt_ts = iso_pi(sk)->ts_flag;
> hci_send_iso(conn->hcon, skb);
> } else {
> len = -ENOTCONN;
> @@ -1471,7 +1482,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sock *sk = sock->sk;
> struct sk_buff *skb, **frag;
> struct sockcm_cookie sockc;
> - size_t mtu;
> + struct cmsghdr *cm;
> + size_t mtu, hlen;
> int err;
>
> BT_DBG("sock %p, sk %p", sock, sk);
> @@ -1485,10 +1497,23 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> hci_sockcm_init(&sockc, sk);
>
> + iso_pi(sk)->ts_flag = false;
> if (msg->msg_controllen) {
> err = sock_cmsg_send(sk, msg, &sockc);
> if (err)
> return err;
> +
> + for (cm = CMSG_FIRSTHDR(msg); cm; cm = CMSG_NXTHDR(msg, cm)) {
> + if (cm->cmsg_level != SOL_BLUETOOTH)
> + continue;
> + if (test_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags) &&
> + cm->cmsg_type == BT_SCM_PKT_ISO_TIMESTAMP &&
> + cm->cmsg_len == CMSG_LEN(sizeof(u32))) {
> + iso_pi(sk)->ts_flag = true;
> + iso_pi(sk)->ts = *(u32 *)CMSG_DATA(cm);
> + break;
> + }
> + }
I would put this in a separate function.
> }
>
> lock_sock(sk);
> @@ -1502,7 +1527,8 @@ 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);
> + hlen = iso_pi(sk)->ts_flag ? HCI_ISO_TS_DATA_HDR_SIZE : HCI_ISO_DATA_HDR_SIZE;
> + skb = bt_skb_sendmsg(sk, msg, len, mtu, hlen, 0);
> if (IS_ERR(skb))
> return PTR_ERR(skb);
>
> @@ -1840,6 +1866,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>
> break;
>
> + case BT_PKT_ISO_TIMESTAMP:
> + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> + if (err)
> + break;
> +
> + if (opt)
> + set_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + else
> + clear_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 16:24 ` Pauli Virtanen
@ 2026-03-13 12:01 ` Tailu Shi
2026-03-13 20:50 ` Pauli Virtanen
0 siblings, 1 reply; 7+ messages in thread
From: Tailu Shi @ 2026-03-13 12:01 UTC (permalink / raw)
To: Pauli Virtanen
Cc: Tailu Shi, Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel,
linux-bluetooth, linux-kernel
Hi Pauli,
Thanks for the detailed review and for sharing your RFC patches.
On Wed, 11 Mar 2026 18:24:23 +0200 Pauli Virtanen wrote:
> These timestamps are in controller clock domain, so generic host like
> Linux appears to need to perform clock synchronization via
> HCI_OP_LE_READ_ISO_TX_SYNC to be able to provide timestamps.
I agree that, according to Core v6.2 Vol 6 Part G Section 3, the timestamp
must be derived from the Controller's clock. However, Section 3.3 also defines
a falllback where the controller decides the CIS/BIS event when the
host‑supplied timestamp is not based on the Controller's clock. It looks
contradictory.
Section 3.3 further states that the host may retrieve the timestamp via
HCI_LE_Read_ISO_TX_Sync command. The wording makes it optional. And also
as you have noted, many controller returns 0 for it, leaving the practical
challenges to make it work properly.
That's why we considered leaving it to the userspace application.
> I had in the works a not fully finished patch series adding the same
> thing + LE Read ISO TX Sync + tests. I sent these now to the list in
> their current state, maybe they are useful if you are working on this
>
> https://lore.kernel.org/linux-bluetooth/cover.1773245666.git.pav@iki.fi/T/
>
> https://lore.kernel.org/linux-bluetooth/cover.1773245672.git.pav@iki.fi/T/
>
> The tests pass, but not really tested to satisfaction on real
> controllers.
>
> From what I saw, it seems several controllers like Intel AX210 and some
> Realtek ones don't implement LE Read ISO TX Sync correctly (returns 0
> timestamp), so in those cases it AFAICS it's not possible to provide
> host timestamps.
>
> Getting the clock sync via LE Read ISO TX Sync to work properly across
> controllers is maybe not so easy, as it appears the Core Specification
> is somewhat unclear how it should work.
I have reviewed the series you posted and I think it both complete and
elegant. Since my earlier work is now covered by your commit (08692f3...),
I will withdraw my patch.
I've cherry-picked your series and kept the PipeWire logic that passes the
timestamp via BT_SCM_PKT_ISO_TIMESTAMP CMSG. The implementation works on my
platform (based on the Kernel 7.0.0-rc1, BlueZ 5.85 and PipeWire 1.5.84).
The timestamp can be observed in the btmon trace.
Partial HCI parsed by Wireshark:
Frame 32436: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
Bluetooth
Bluetooth Linux Monitor Transport
Bluetooth HCI ISO Packet
.... 1010 0000 0101 = Connection Handle: 0xa05
..10 .... .... .... = PB Flag: Complete SDU (0x2)
.1.. .... .... .... = Timestamp present: True
0... .... .... .... = Reserved: 0x0
..00 0000 0100 0100 = Data Length: 68
Data
[Connect in frame: 9436]
Bluetooth ISO Data
Timestamp: 469412333us
Sequence Number: 3595
.... 0000 0011 1100 = SDU Length: 60
SDU
Please feel free to add the following Tested-by line if it helps:
Tested-by: Tailu Shi <tailu.shi@samsung.com>
May I know if you have any plan to merge this series? And also please feel
free to let me know if there's anything I can help to move this forward.
> This sends timestamps in host clock domain and has no clock sync, but I
> guess this is just for testing.
I think it's better to have a complete implementation solution. So if possible,
I would like to improve it and push it forward. Please feel free to let me
know if you have any other opinions.
> I think these are not needed, they can be passed as arguments to
> iso_send_frame().
>
> Adding them to iso_pinfo() probably causes a data race if two processes
> sendmsg() at the same time.
Thank you for identifying this potential data race issue. It was not considered
in the current patch, and I will incorporate your suggestion in future development.
> I would put this in a separate function.
Agreed. It will make code clear. I will keep this code style in future development.
Thank you again for the detailed review!
Best regards!
Tailu
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-13 12:01 ` Tailu Shi
@ 2026-03-13 20:50 ` Pauli Virtanen
0 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2026-03-13 20:50 UTC (permalink / raw)
To: Tailu Shi; +Cc: Luiz Augusto von Dentz, linux-bluetooth
Hi,
pe, 2026-03-13 kello 20:01 +0800, Tailu Shi kirjoitti:
[clip]
> I agree that, according to Core v6.2 Vol 6 Part G Section 3, the
> timestamp must be derived from the Controller's clock. However,
> Section 3.3 also defines a falllback where the controller decides the
> CIS/BIS event when the host‑supplied timestamp is not based on the
> Controller's clock. It looks contradictory.
TMAP likely assumes the controller implements the specification so that
providing correct time stamps would be possible, but when they in
practice don't, spec compliance seems dubious and not setting known-
wrong Time_Stamp on packets sounds safer.
[clip]
> I've cherry-picked your series and kept the PipeWire logic that passes the
> timestamp via BT_SCM_PKT_ISO_TIMESTAMP CMSG. The implementation works on my
> platform (based on the Kernel 7.0.0-rc1, BlueZ 5.85 and PipeWire 1.5.84).
> The timestamp can be observed in the btmon trace.
>
> Partial HCI parsed by Wireshark:
> Frame 32436: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
> Bluetooth
> Bluetooth Linux Monitor Transport
> Bluetooth HCI ISO Packet
> .... 1010 0000 0101 = Connection Handle: 0xa05
> ..10 .... .... .... = PB Flag: Complete SDU (0x2)
> .1.. .... .... .... = Timestamp present: True
> 0... .... .... .... = Reserved: 0x0
> ..00 0000 0100 0100 = Data Length: 68
> Data
> [Connect in frame: 9436]
> Bluetooth ISO Data
> Timestamp: 469412333us
> Sequence Number: 3595
> .... 0000 0011 1100 = SDU Length: 60
> SDU
>
> Please feel free to add the following Tested-by line if it helps:
> Tested-by: Tailu Shi <tailu.shi@samsung.com>
>
> May I know if you have any plan to merge this series? And also please feel
> free to let me know if there's anything I can help to move this forward.
>
> > This sends timestamps in host clock domain and has no clock sync, but I
> > guess this is just for testing.
>
> I think it's better to have a complete implementation solution. So if possible,
> I would like to improve it and push it forward. Please feel free to let me
> know if you have any other opinions.
I think we will want LE Read ISO TX Sync support at least, as this will
solve the CIS/BIS synchronization problem, as user application can then
know what ISO packets controller considers are playing at the same time
(if controller actually implements this command properly).
It would be good to have information how different manufacturers have
interpreted the specification vs. LE Read ISO TX Sync. Right now, I can
only read what Zephyr is doing, and tested some other controllers
return zeros. If you have access to controllers that implement this
command, it would be very useful to know their precise interpretation.
@Luiz: thoughts on the below?
For setting Time_Stamp, I think there should be plan how userspace
application uses the new APIs to produce working sound on different
controllers, without having to maintain a quirk list for each
controller. I think quirk lists should be in the kernel driver.
Depending on what hardware is doing, we could add driver quirks along
the lines of
HCI_QUIRK_BROKEN_LE_READ_ISO_TX_SYNC, /* always zero / dummy value */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_HOST_PREV, /* most recent from host */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_NOCP, /* most recent completed */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_HOST_SEQ, /* uses host seqnum */
and should affect what skb is reported in hci_cc_le_read_iso_tx_sync(),
and if SOF_TIMESTAMPING_TX_HARDWARE capability is reported in ethtool.
(Eg. IIUC Zephyr HCI appears to interpret LE Read ISO TX Sync meaning
as follows:
- The sequence number is controller-assigned.
- The response corresponds to the ISO data packet that
the controller *most recently received from host*.
This interpretation makes sense, but is different from my patch and
iiuc also different from what Luiz had in mind.)
If no quirk is set but the command is supported, probably should emit
visible error in kernel log, or something along such lines, so the
driver authors can see something is not right.
But knowing what is the precise hardware behavior for each
chip/firmware seems a bit involved, probably should have some testing
tool for this.
***
What it seems userspace can do:
Userspace application would check via ethtool
SOF_TIMESTAMPING_TX_HARDWARE capability to know whether controller
handles Time_Stamp in a non-broken way. (For testing, there could be a
switch to force it to use timestamps regardless.)
It could then rely that the HW timestamp corresponds to specific packet
it sent earlier. It could assume that in its clock domain, the HW
timestamp is after SND timestamp, before COMPLETION timestamp, and
likely close to COMPLETION.
Since it doesn't know the exact correspondence, probably it would
always have to ensure a few packets are always queued in controller.
There also needs to be a way to recover from a situation where the
application is setting timestamps that are in the past in controller
clock domain.
The controller does not necessarily send them I think, but will report
them completed immediately (after older ones get flushed from its
buffers).
In this case LE Read ISO TX Sync timestamp would not advance, as no
packets get sent. Kernel would also not send HW timestamps to user
application in this case.
If the application when it doesn't receive HW timestamps from kernel,
just increments its send timestamp by SDU interval for each packet, it
eventually catches up the controller clock, and receives HW timestamp
and can resync.
Alternatively the user application may stop setting timestamps, in
which case controller shall send it in next SDU interval, and in this
case HW timestamp is received and user application can resync.
Or, alternatively, controller will decide the clock is out of sync and
send the SDU regardless, in which case user application will receive a
new HW timestamp and can again resync.
>
> > I think these are not needed, they can be passed as arguments to
> > iso_send_frame().
> >
> > Adding them to iso_pinfo() probably causes a data race if two processes
> > sendmsg() at the same time.
>
> Thank you for identifying this potential data race issue. It was not considered
> in the current patch, and I will incorporate your suggestion in future development.
>
> > I would put this in a separate function.
>
> Agreed. It will make code clear. I will keep this code style in future development.
>
>
> Thank you again for the detailed review!
>
> Best regards!
>
> Tailu
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:14 ` [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet tailu.shi
2026-03-11 8:54 ` Paul Menzel
2026-03-11 16:24 ` Pauli Virtanen
@ 2026-04-09 20:01 ` bluez.test.bot
2 siblings, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2026-04-09 20:01 UTC (permalink / raw)
To: linux-bluetooth, tailu.shi
[-- Attachment #1: Type: text/plain, Size: 7146 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=1064886
---Test result---
Test Summary:
CheckPatch PENDING 0.65 seconds
GitLint PENDING 0.53 seconds
SubjectPrefix PASS 0.06 seconds
BuildKernel PASS 26.70 seconds
CheckAllWarning PASS 28.86 seconds
CheckSparse PASS 27.93 seconds
BuildKernel32 PASS 25.43 seconds
TestRunnerSetup PASS 569.92 seconds
TestRunner_l2cap-tester FAIL 28.54 seconds
TestRunner_iso-tester PASS 46.25 seconds
TestRunner_bnep-tester PASS 6.33 seconds
TestRunner_mgmt-tester FAIL 113.45 seconds
TestRunner_rfcomm-tester PASS 10.83 seconds
TestRunner_sco-tester FAIL 14.42 seconds
TestRunner_ioctl-tester PASS 10.13 seconds
TestRunner_mesh-tester FAIL 12.62 seconds
TestRunner_smp-tester PASS 8.82 seconds
TestRunner_userchan-tester PASS 6.70 seconds
TestRunner_6lowpan-tester FAIL 8.85 seconds
IncrementalBuild PENDING 0.42 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Total: 96, Passed: 95 (99.0%), Failed: 1, Not Run: 0
Failed Test Cases
L2CAP BR/EDR Server - Set PHY 3M Failed 0.115 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.102 seconds
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
7.0.0-rc2-g9405d1bdb66a #1 Not tainted
------------------------------------------------------
kworker/u5:2/117 is trying to acquire lock:
ffff888001946240 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}, at: sco_connect_cfm+0x358/0x8d0
but task is already holding lock:
ffff888002141c20 (&conn->lock){+.+.}-{3:3}, at: sco_connect_cfm+0x22d/0x8d0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&conn->lock){+.+.}-{3:3}:
lock_acquire+0xf7/0x2c0
_raw_spin_lock+0x2a/0x40
sco_sock_connect+0x4d7/0x1280
__sys_connect+0x1a3/0x260
__x64_sys_connect+0x6e/0xb0
do_syscall_64+0xa0/0x570
entry_SYSCALL_64_after_hwframe+0x74/0x7c
-> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}:
check_prev_add+0xe9/0xc70
__lock_acquire+0x1457/0x1df0
lock_acquire+0xf7/0x2c0
lock_sock_nested+0x36/0xd0
sco_connect_cfm+0x358/0x8d0
hci_sync_conn_complete_evt+0x3d3/0x8e0
hci_event_packet+0x74f/0xb10
hci_rx_work+0x398/0xd00
process_scheduled_works+0xb16/0x1ac0
worker_thread+0x4ff/0xba0
kthread+0x368/0x490
ret_from_fork+0x498/0x7e0
ret_from_fork_asm+0x19/0x30
other info that might help us debug this:
...
BUG: sleeping function called from invalid context at net/core/sock.c:3782
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 117, name: kworker/u5:2
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
CPU: 0 UID: 0 PID: 117 Comm: kworker/u5:2 Not tainted 7.0.0-rc2-g9405d1bdb66a #1 PREEMPT(lazy)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Workqueue: hci0 hci_rx_work
Call Trace:
<TASK>
dump_stack_lvl+0x49/0x60
__might_resched+0x2ea/0x500
lock_sock_nested+0x47/0xd0
? sco_connect_cfm+0x358/0x8d0
sco_connect_cfm+0x358/0x8d0
? hci_debugfs_create_conn+0x190/0x210
? __pfx_sco_connect_cfm+0x10/0x10
hci_sync_conn_complete_evt+0x3d3/0x8e0
hci_event_packet+0x74f/0xb10
? __pfx_hci_sync_conn_complete_evt+0x10/0x10
? __pfx_hci_event_packet+0x10/0x10
? mark_held_locks+0x49/0x80
? lockdep_hardirqs_on_prepare+0xd4/0x180
? _raw_spin_unlock_irqrestore+0x2c/0x50
hci_rx_work+0x398/0xd00
process_scheduled_works+0xb16/0x1ac0
? __pfx_process_scheduled_works+0x10/0x10
? lock_acquire+0xf7/0x2c0
? lock_is_held_type+0x9b/0x110
? __pfx_hci_rx_work+0x10/0x10
worker_thread+0x4ff/0xba0
? _raw_spin_unlock_irqrestore+0x2c/0x50
? __pfx_worker_thread+0x10/0x10
kthread+0x368/0x490
? _raw_spin_unlock_irq+0x23/0x40
? __pfx_kthread+0x10/0x10
ret_from_fork+0x498/0x7e0
? __pfx_ret_from_fork+0x10/0x10
? __switch_to+0x9e4/0xe50
? __switch_to_asm+0x32/0x60
...
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.753 seconds
Mesh - Send cancel - 2 Timed out 1.993 seconds
##############################
Test: TestRunner_6lowpan-tester - FAIL
Desc: Run 6lowpan-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
7.0.0-rc2-g9405d1bdb66a #1 Not tainted
------------------------------------------------------
kworker/0:1/11 is trying to acquire lock:
ffff8880026e1140 ((wq_completion)hci0#2){+.+.}-{0:0}, at: touch_wq_lockdep_map+0x75/0x180
but task is already holding lock:
ffffffffb6a4d720 (rtnl_mutex){+.+.}-{4:4}, at: lowpan_unregister_netdev+0xd/0x30
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #4 (rtnl_mutex){+.+.}-{4:4}:
lock_acquire+0xf7/0x2c0
__mutex_lock+0x16b/0x1fc0
lowpan_register_netdev+0x11/0x30
chan_ready_cb+0x836/0xd00
l2cap_recv_frame+0x6a06/0x8920
l2cap_recv_acldata+0x790/0xdf0
hci_rx_work+0x500/0xd00
process_scheduled_works+0xb16/0x1ac0
worker_thread+0x4ff/0xba0
kthread+0x368/0x490
ret_from_fork+0x498/0x7e0
ret_from_fork_asm+0x19/0x30
-> #3 (&chan->lock#3/1){+.+.}-{4:4}:
lock_acquire+0xf7/0x2c0
__mutex_lock+0x16b/0x1fc0
l2cap_chan_connect+0x74e/0x1980
lowpan_control_write+0x523/0x660
full_proxy_write+0x10b/0x190
vfs_write+0x1c0/0xf60
ksys_write+0xf1/0x1d0
do_syscall_64+0xa0/0x570
entry_SYSCALL_64_after_hwframe+0x74/0x7c
-> #2 (&conn->lock){+.+.}-{4:4}:
...
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
https://github.com/bluez/bluetooth-next/pull/18/checks
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 7+ messages in thread