[PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Yang Li via B4 Relay]

From: Yang Li <yang.li@amlogic.com>

Application layer programs (like pipewire) need to use
iso timestamp information for audio synchronization.

Signed-off-by: Yang Li <yang.li@amlogic.com>
---
 include/net/bluetooth/bluetooth.h |  4 ++-
 net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index bbefde319f95..a102bd76647c 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -242,6 +242,7 @@ struct bt_codecs {
 #define BT_CODEC_MSBC		0x05
 
 #define BT_ISO_BASE		20
+#define BT_ISO_TS		21
 
 __printf(1, 2)
 void bt_info(const char *fmt, ...);
@@ -390,7 +391,8 @@ struct bt_sock {
 enum {
 	BT_SK_DEFER_SETUP,
 	BT_SK_SUSPEND,
-	BT_SK_PKT_STATUS
+	BT_SK_PKT_STATUS,
+	BT_SK_ISO_TS
 };
 
 struct bt_sock_list {
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 2f348f48e99d..2c1fdea4b8c1 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
 		iso_pi(sk)->base_len = optlen;
 
 		break;
+	case BT_ISO_TS:
+		if (optlen != sizeof(opt)) {
+			err = -EINVAL;
+			break;
+		}
 
+		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
+		if (err)
+			break;
+
+		if (opt)
+			set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
+		else
+			clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
+		break;
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
 			err = -EFAULT;
 
 		break;
+	case BT_ISO_TS:
+		if (len < sizeof(u32)) {
+			err = -EINVAL;
+			break;
+		}
 
+		if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
+			    (u32 __user *)optval))
+			err = -EFAULT;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
 void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 {
 	struct iso_conn *conn = hcon->iso_data;
+	struct sock *sk;
 	__u16 pb, ts, len;
 
 	if (!conn)
 		goto drop;
 
-	pb     = hci_iso_flags_pb(flags);
-	ts     = hci_iso_flags_ts(flags);
+	iso_conn_lock(conn);
+	sk = conn->sk;
+	iso_conn_unlock(conn);
+
+	if (!sk)
+		goto drop;
+
+	pb = hci_iso_flags_pb(flags);
+	ts = hci_iso_flags_ts(flags);
 
 	BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
 
@@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 		if (ts) {
 			struct hci_iso_ts_data_hdr *hdr;
 
-			/* TODO: add timestamp to the packet? */
-			hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
-			if (!hdr) {
-				BT_ERR("Frame is too short (len %d)", skb->len);
-				goto drop;
-			}
+			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
+				hdr = (struct hci_iso_ts_data_hdr *)skb->data;
+				len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
+			} else {
+				hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
+				if (!hdr) {
+					BT_ERR("Frame is too short (len %d)", skb->len);
+					goto drop;
+				}
 
-			len = __le16_to_cpu(hdr->slen);
+				len = __le16_to_cpu(hdr->slen);
+			}
 		} else {
 			struct hci_iso_data_hdr *hdr;
 
+			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
+				BT_ERR("Invalid option BT_SK_ISO_TS");
+				clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
+			}
+
 			hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
 			if (!hdr) {
 				BT_ERR("Frame is too short (len %d)", skb->len);

---
base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
change-id: 20250421-iso_ts-c82a300ae784

Best regards,
-- 
Yang Li <yang.li@amlogic.com>

RE: iso: add BT_ISO_TS optional to enable ISO timestamp

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2863 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=957927

---Test result---

Test Summary:
CheckPatch                    PENDING   0.35 seconds
GitLint                       PENDING   0.28 seconds
SubjectPrefix                 FAIL      0.41 seconds
BuildKernel                   PASS      24.31 seconds
CheckAllWarning               PASS      26.61 seconds
CheckSparse                   WARNING   30.08 seconds
BuildKernel32                 PASS      24.09 seconds
TestRunnerSetup               PASS      456.46 seconds
TestRunner_l2cap-tester       PASS      22.11 seconds
TestRunner_iso-tester         FAIL      35.46 seconds
TestRunner_bnep-tester        PASS      4.65 seconds
TestRunner_mgmt-tester        PASS      118.65 seconds
TestRunner_rfcomm-tester      PASS      7.84 seconds
TestRunner_sco-tester         PASS      12.86 seconds
TestRunner_ioctl-tester       PASS      8.19 seconds
TestRunner_mesh-tester        FAIL      6.07 seconds
TestRunner_smp-tester         PASS      7.27 seconds
TestRunner_userchan-tester    PASS      4.90 seconds
IncrementalBuild              PENDING   0.55 seconds

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

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

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/iso.c:2330:42: warning: restricted __le16 degrades to integer
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 127, Passed: 124 (97.6%), Failed: 2, Not Run: 1

Failed Test Cases
ISO Send - TX Timestamping                           Failed       0.192 seconds
ISO Send - TX CMSG Timestamping                      Failed       0.185 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76f/0x7d0
WARNING: CPU: 0 PID: 36 at kernel/workqueue.c:2257 __queue_work+0x93e/0xba0
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Failed       0.120 seconds
Mesh - Send cancel - 2                               Failed       0.141 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Pauli Virtanen]

Hi,

ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> From: Yang Li <yang.li@amlogic.com>
> 
> Application layer programs (like pipewire) need to use
> iso timestamp information for audio synchronization.

I think the timestamp should be put into CMSG, same ways as packet
status is. The packet body should then always contain only the payload.

> 
> Signed-off-by: Yang Li <yang.li@amlogic.com>
> ---
>  include/net/bluetooth/bluetooth.h |  4 ++-
>  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index bbefde319f95..a102bd76647c 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -242,6 +242,7 @@ struct bt_codecs {
>  #define BT_CODEC_MSBC		0x05
>  
>  #define BT_ISO_BASE		20
> +#define BT_ISO_TS		21
>  
>  __printf(1, 2)
>  void bt_info(const char *fmt, ...);
> @@ -390,7 +391,8 @@ struct bt_sock {
>  enum {
>  	BT_SK_DEFER_SETUP,
>  	BT_SK_SUSPEND,
> -	BT_SK_PKT_STATUS
> +	BT_SK_PKT_STATUS,
> +	BT_SK_ISO_TS
>  };
>  
>  struct bt_sock_list {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 2f348f48e99d..2c1fdea4b8c1 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>  		iso_pi(sk)->base_len = optlen;
>  
>  		break;
> +	case BT_ISO_TS:
> +		if (optlen != sizeof(opt)) {
> +			err = -EINVAL;
> +			break;
> +		}
>  
> +		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> +		if (err)
> +			break;
> +
> +		if (opt)
> +			set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> +		else
> +			clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> +		break;
>  	default:
>  		err = -ENOPROTOOPT;
>  		break;
> @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
>  			err = -EFAULT;
>  
>  		break;
> +	case BT_ISO_TS:
> +		if (len < sizeof(u32)) {
> +			err = -EINVAL;
> +			break;
> +		}
>  
> +		if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> +			    (u32 __user *)optval))
> +			err = -EFAULT;
> +		break;
>  	default:
>  		err = -ENOPROTOOPT;
>  		break;
> @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
>  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>  {
>  	struct iso_conn *conn = hcon->iso_data;
> +	struct sock *sk;
>  	__u16 pb, ts, len;
>  
>  	if (!conn)
>  		goto drop;
>  
> -	pb     = hci_iso_flags_pb(flags);
> -	ts     = hci_iso_flags_ts(flags);
> +	iso_conn_lock(conn);
> +	sk = conn->sk;
> +	iso_conn_unlock(conn);
> +
> +	if (!sk)
> +		goto drop;
> +
> +	pb = hci_iso_flags_pb(flags);
> +	ts = hci_iso_flags_ts(flags);
>  
>  	BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
>  
> @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>  		if (ts) {
>  			struct hci_iso_ts_data_hdr *hdr;
>  
> -			/* TODO: add timestamp to the packet? */
> -			hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> -			if (!hdr) {
> -				BT_ERR("Frame is too short (len %d)", skb->len);
> -				goto drop;
> -			}
> +			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> +				hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> +				len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> +			} else {
> +				hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> +				if (!hdr) {
> +					BT_ERR("Frame is too short (len %d)", skb->len);
> +					goto drop;
> +				}
>  
> -			len = __le16_to_cpu(hdr->slen);
> +				len = __le16_to_cpu(hdr->slen);
> +			}
>  		} else {
>  			struct hci_iso_data_hdr *hdr;
>  
> +			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> +				BT_ERR("Invalid option BT_SK_ISO_TS");
> +				clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> +			}
> +
>  			hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
>  			if (!hdr) {
>  				BT_ERR("Frame is too short (len %d)", skb->len);
> 
> ---
> base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> change-id: 20250421-iso_ts-c82a300ae784
> 
> Best regards,

-- 
Pauli Virtanen

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Pauli Virtanen]

ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
> Hi,
> 
> ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> > From: Yang Li <yang.li@amlogic.com>
> > 
> > Application layer programs (like pipewire) need to use
> > iso timestamp information for audio synchronization.
> 
> I think the timestamp should be put into CMSG, same ways as packet
> status is. The packet body should then always contain only the payload.

Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
mechanism, which would avoid adding a new API.

> > 
> > Signed-off-by: Yang Li <yang.li@amlogic.com>
> > ---
> >  include/net/bluetooth/bluetooth.h |  4 ++-
> >  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
> >  2 files changed, 52 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index bbefde319f95..a102bd76647c 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -242,6 +242,7 @@ struct bt_codecs {
> >  #define BT_CODEC_MSBC		0x05
> >  
> >  #define BT_ISO_BASE		20
> > +#define BT_ISO_TS		21
> >  
> >  __printf(1, 2)
> >  void bt_info(const char *fmt, ...);
> > @@ -390,7 +391,8 @@ struct bt_sock {
> >  enum {
> >  	BT_SK_DEFER_SETUP,
> >  	BT_SK_SUSPEND,
> > -	BT_SK_PKT_STATUS
> > +	BT_SK_PKT_STATUS,
> > +	BT_SK_ISO_TS
> >  };
> >  
> >  struct bt_sock_list {
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 2f348f48e99d..2c1fdea4b8c1 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> >  		iso_pi(sk)->base_len = optlen;
> >  
> >  		break;
> > +	case BT_ISO_TS:
> > +		if (optlen != sizeof(opt)) {
> > +			err = -EINVAL;
> > +			break;
> > +		}
> >  
> > +		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> > +		if (err)
> > +			break;
> > +
> > +		if (opt)
> > +			set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +		else
> > +			clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +		break;
> >  	default:
> >  		err = -ENOPROTOOPT;
> >  		break;
> > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> >  			err = -EFAULT;
> >  
> >  		break;
> > +	case BT_ISO_TS:
> > +		if (len < sizeof(u32)) {
> > +			err = -EINVAL;
> > +			break;
> > +		}
> >  
> > +		if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> > +			    (u32 __user *)optval))
> > +			err = -EFAULT;
> > +		break;
> >  	default:
> >  		err = -ENOPROTOOPT;
> >  		break;
> > @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
> >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >  {
> >  	struct iso_conn *conn = hcon->iso_data;
> > +	struct sock *sk;
> >  	__u16 pb, ts, len;
> >  
> >  	if (!conn)
> >  		goto drop;
> >  
> > -	pb     = hci_iso_flags_pb(flags);
> > -	ts     = hci_iso_flags_ts(flags);
> > +	iso_conn_lock(conn);
> > +	sk = conn->sk;
> > +	iso_conn_unlock(conn);
> > +
> > +	if (!sk)
> > +		goto drop;
> > +
> > +	pb = hci_iso_flags_pb(flags);
> > +	ts = hci_iso_flags_ts(flags);
> >  
> >  	BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
> >  
> > @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >  		if (ts) {
> >  			struct hci_iso_ts_data_hdr *hdr;
> >  
> > -			/* TODO: add timestamp to the packet? */
> > -			hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > -			if (!hdr) {
> > -				BT_ERR("Frame is too short (len %d)", skb->len);
> > -				goto drop;
> > -			}
> > +			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > +				hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> > +				len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> > +			} else {
> > +				hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > +				if (!hdr) {
> > +					BT_ERR("Frame is too short (len %d)", skb->len);
> > +					goto drop;
> > +				}
> >  
> > -			len = __le16_to_cpu(hdr->slen);
> > +				len = __le16_to_cpu(hdr->slen);
> > +			}
> >  		} else {
> >  			struct hci_iso_data_hdr *hdr;
> >  
> > +			if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > +				BT_ERR("Invalid option BT_SK_ISO_TS");
> > +				clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +			}
> > +
> >  			hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
> >  			if (!hdr) {
> >  				BT_ERR("Frame is too short (len %d)", skb->len);
> > 
> > ---
> > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> > change-id: 20250421-iso_ts-c82a300ae784
> > 
> > Best regards,

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Apr 29, 2025 at 10:29 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
> > Hi,
> >
> > ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> > > From: Yang Li <yang.li@amlogic.com>
> > >
> > > Application layer programs (like pipewire) need to use
> > > iso timestamp information for audio synchronization.
> >
> > I think the timestamp should be put into CMSG, same ways as packet
> > status is. The packet body should then always contain only the payload.
>
> Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
> mechanism, which would avoid adding a new API.

Either that or we use BT_PKT_STATUS, does SOF_TIMESTAMPING_RX_HARDWARE
requires the use of errqueue?

> > >
> > > Signed-off-by: Yang Li <yang.li@amlogic.com>
> > > ---
> > >  include/net/bluetooth/bluetooth.h |  4 ++-
> > >  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
> > >  2 files changed, 52 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > > index bbefde319f95..a102bd76647c 100644
> > > --- a/include/net/bluetooth/bluetooth.h
> > > +++ b/include/net/bluetooth/bluetooth.h
> > > @@ -242,6 +242,7 @@ struct bt_codecs {
> > >  #define BT_CODEC_MSBC              0x05
> > >
> > >  #define BT_ISO_BASE                20
> > > +#define BT_ISO_TS          21
> > >
> > >  __printf(1, 2)
> > >  void bt_info(const char *fmt, ...);
> > > @@ -390,7 +391,8 @@ struct bt_sock {
> > >  enum {
> > >     BT_SK_DEFER_SETUP,
> > >     BT_SK_SUSPEND,
> > > -   BT_SK_PKT_STATUS
> > > +   BT_SK_PKT_STATUS,
> > > +   BT_SK_ISO_TS
> > >  };
> > >
> > >  struct bt_sock_list {
> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > index 2f348f48e99d..2c1fdea4b8c1 100644
> > > --- a/net/bluetooth/iso.c
> > > +++ b/net/bluetooth/iso.c
> > > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> > >             iso_pi(sk)->base_len = optlen;
> > >
> > >             break;
> > > +   case BT_ISO_TS:
> > > +           if (optlen != sizeof(opt)) {
> > > +                   err = -EINVAL;
> > > +                   break;
> > > +           }
> > >
> > > +           err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> > > +           if (err)
> > > +                   break;
> > > +
> > > +           if (opt)
> > > +                   set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > > +           else
> > > +                   clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > > +           break;
> > >     default:
> > >             err = -ENOPROTOOPT;
> > >             break;
> > > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> > >                     err = -EFAULT;
> > >
> > >             break;
> > > +   case BT_ISO_TS:
> > > +           if (len < sizeof(u32)) {
> > > +                   err = -EINVAL;
> > > +                   break;
> > > +           }
> > >
> > > +           if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> > > +                       (u32 __user *)optval))
> > > +                   err = -EFAULT;
> > > +           break;
> > >     default:
> > >             err = -ENOPROTOOPT;
> > >             break;
> > > @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
> > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> > >  {
> > >     struct iso_conn *conn = hcon->iso_data;
> > > +   struct sock *sk;
> > >     __u16 pb, ts, len;
> > >
> > >     if (!conn)
> > >             goto drop;
> > >
> > > -   pb     = hci_iso_flags_pb(flags);
> > > -   ts     = hci_iso_flags_ts(flags);
> > > +   iso_conn_lock(conn);
> > > +   sk = conn->sk;
> > > +   iso_conn_unlock(conn);
> > > +
> > > +   if (!sk)
> > > +           goto drop;
> > > +
> > > +   pb = hci_iso_flags_pb(flags);
> > > +   ts = hci_iso_flags_ts(flags);
> > >
> > >     BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
> > >
> > > @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> > >             if (ts) {
> > >                     struct hci_iso_ts_data_hdr *hdr;
> > >
> > > -                   /* TODO: add timestamp to the packet? */
> > > -                   hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > > -                   if (!hdr) {
> > > -                           BT_ERR("Frame is too short (len %d)", skb->len);
> > > -                           goto drop;
> > > -                   }
> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > > +                           hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> > > +                           len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> > > +                   } else {
> > > +                           hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > > +                           if (!hdr) {
> > > +                                   BT_ERR("Frame is too short (len %d)", skb->len);
> > > +                                   goto drop;
> > > +                           }
> > >
> > > -                   len = __le16_to_cpu(hdr->slen);
> > > +                           len = __le16_to_cpu(hdr->slen);
> > > +                   }
> > >             } else {
> > >                     struct hci_iso_data_hdr *hdr;
> > >
> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > > +                           BT_ERR("Invalid option BT_SK_ISO_TS");
> > > +                           clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > > +                   }
> > > +
> > >                     hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
> > >                     if (!hdr) {
> > >                             BT_ERR("Frame is too short (len %d)", skb->len);
> > >
> > > ---
> > > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> > > change-id: 20250421-iso_ts-c82a300ae784
> > >
> > > Best regards,



-- 
Luiz Augusto von Dentz

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Pauli Virtanen]

Hi,

29. huhtikuuta 2025 17.31.25 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com> kirjoitti:
>Hi Pauli,
>
>On Tue, Apr 29, 2025 at 10:29 AM Pauli Virtanen <pav@iki.fi> wrote:
>>
>> ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
>> > Hi,
>> >
>> > ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
>> > > From: Yang Li <yang.li@amlogic.com>
>> > >
>> > > Application layer programs (like pipewire) need to use
>> > > iso timestamp information for audio synchronization.
>> >
>> > I think the timestamp should be put into CMSG, same ways as packet
>> > status is. The packet body should then always contain only the payload.
>>
>> Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
>> mechanism, which would avoid adding a new API.
>
>Either that or we use BT_PKT_STATUS, does SOF_TIMESTAMPING_RX_HARDWARE
>requires the use of errqueue?

No, it just adds a CMSG, similar to the RX software tstamp.

>
>> > >
>> > > Signed-off-by: Yang Li <yang.li@amlogic.com>
>> > > ---
>> > >  include/net/bluetooth/bluetooth.h |  4 ++-
>> > >  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
>> > >  2 files changed, 52 insertions(+), 10 deletions(-)
>> > >
>> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> > > index bbefde319f95..a102bd76647c 100644
>> > > --- a/include/net/bluetooth/bluetooth.h
>> > > +++ b/include/net/bluetooth/bluetooth.h
>> > > @@ -242,6 +242,7 @@ struct bt_codecs {
>> > >  #define BT_CODEC_MSBC              0x05
>> > >
>> > >  #define BT_ISO_BASE                20
>> > > +#define BT_ISO_TS          21
>> > >
>> > >  __printf(1, 2)
>> > >  void bt_info(const char *fmt, ...);
>> > > @@ -390,7 +391,8 @@ struct bt_sock {
>> > >  enum {
>> > >     BT_SK_DEFER_SETUP,
>> > >     BT_SK_SUSPEND,
>> > > -   BT_SK_PKT_STATUS
>> > > +   BT_SK_PKT_STATUS,
>> > > +   BT_SK_ISO_TS
>> > >  };
>> > >
>> > >  struct bt_sock_list {
>> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>> > > index 2f348f48e99d..2c1fdea4b8c1 100644
>> > > --- a/net/bluetooth/iso.c
>> > > +++ b/net/bluetooth/iso.c
>> > > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>> > >             iso_pi(sk)->base_len = optlen;
>> > >
>> > >             break;
>> > > +   case BT_ISO_TS:
>> > > +           if (optlen != sizeof(opt)) {
>> > > +                   err = -EINVAL;
>> > > +                   break;
>> > > +           }
>> > >
>> > > +           err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
>> > > +           if (err)
>> > > +                   break;
>> > > +
>> > > +           if (opt)
>> > > +                   set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>> > > +           else
>> > > +                   clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>> > > +           break;
>> > >     default:
>> > >             err = -ENOPROTOOPT;
>> > >             break;
>> > > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
>> > >                     err = -EFAULT;
>> > >
>> > >             break;
>> > > +   case BT_ISO_TS:
>> > > +           if (len < sizeof(u32)) {
>> > > +                   err = -EINVAL;
>> > > +                   break;
>> > > +           }
>> > >
>> > > +           if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
>> > > +                       (u32 __user *)optval))
>> > > +                   err = -EFAULT;
>> > > +           break;
>> > >     default:
>> > >             err = -ENOPROTOOPT;
>> > >             break;
>> > > @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
>> > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>> > >  {
>> > >     struct iso_conn *conn = hcon->iso_data;
>> > > +   struct sock *sk;
>> > >     __u16 pb, ts, len;
>> > >
>> > >     if (!conn)
>> > >             goto drop;
>> > >
>> > > -   pb     = hci_iso_flags_pb(flags);
>> > > -   ts     = hci_iso_flags_ts(flags);
>> > > +   iso_conn_lock(conn);
>> > > +   sk = conn->sk;
>> > > +   iso_conn_unlock(conn);
>> > > +
>> > > +   if (!sk)
>> > > +           goto drop;
>> > > +
>> > > +   pb = hci_iso_flags_pb(flags);
>> > > +   ts = hci_iso_flags_ts(flags);
>> > >
>> > >     BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
>> > >
>> > > @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>> > >             if (ts) {
>> > >                     struct hci_iso_ts_data_hdr *hdr;
>> > >
>> > > -                   /* TODO: add timestamp to the packet? */
>> > > -                   hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>> > > -                   if (!hdr) {
>> > > -                           BT_ERR("Frame is too short (len %d)", skb->len);
>> > > -                           goto drop;
>> > > -                   }
>> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
>> > > +                           hdr = (struct hci_iso_ts_data_hdr *)skb->data;
>> > > +                           len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
>> > > +                   } else {
>> > > +                           hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>> > > +                           if (!hdr) {
>> > > +                                   BT_ERR("Frame is too short (len %d)", skb->len);
>> > > +                                   goto drop;
>> > > +                           }
>> > >
>> > > -                   len = __le16_to_cpu(hdr->slen);
>> > > +                           len = __le16_to_cpu(hdr->slen);
>> > > +                   }
>> > >             } else {
>> > >                     struct hci_iso_data_hdr *hdr;
>> > >
>> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
>> > > +                           BT_ERR("Invalid option BT_SK_ISO_TS");
>> > > +                           clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>> > > +                   }
>> > > +
>> > >                     hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
>> > >                     if (!hdr) {
>> > >                             BT_ERR("Frame is too short (len %d)", skb->len);
>> > >
>> > > ---
>> > > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
>> > > change-id: 20250421-iso_ts-c82a300ae784
>> > >
>> > > Best regards,
>
>
>

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Apr 29, 2025 at 10:35 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> 29. huhtikuuta 2025 17.31.25 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com> kirjoitti:
> >Hi Pauli,
> >
> >On Tue, Apr 29, 2025 at 10:29 AM Pauli Virtanen <pav@iki.fi> wrote:
> >>
> >> ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
> >> > Hi,
> >> >
> >> > ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> >> > > From: Yang Li <yang.li@amlogic.com>
> >> > >
> >> > > Application layer programs (like pipewire) need to use
> >> > > iso timestamp information for audio synchronization.
> >> >
> >> > I think the timestamp should be put into CMSG, same ways as packet
> >> > status is. The packet body should then always contain only the payload.
> >>
> >> Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
> >> mechanism, which would avoid adding a new API.
> >
> >Either that or we use BT_PKT_STATUS, does SOF_TIMESTAMPING_RX_HARDWARE
> >requires the use of errqueue?
>
> No, it just adds a CMSG, similar to the RX software tstamp.

Perfect, then there should be no problem going with that, we might
want to introduce some tests for it and perhaps have the emulator
adding timestamps headers so we can test this with the likes of
iso-tester.

> >
> >> > >
> >> > > Signed-off-by: Yang Li <yang.li@amlogic.com>
> >> > > ---
> >> > >  include/net/bluetooth/bluetooth.h |  4 ++-
> >> > >  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
> >> > >  2 files changed, 52 insertions(+), 10 deletions(-)
> >> > >
> >> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >> > > index bbefde319f95..a102bd76647c 100644
> >> > > --- a/include/net/bluetooth/bluetooth.h
> >> > > +++ b/include/net/bluetooth/bluetooth.h
> >> > > @@ -242,6 +242,7 @@ struct bt_codecs {
> >> > >  #define BT_CODEC_MSBC              0x05
> >> > >
> >> > >  #define BT_ISO_BASE                20
> >> > > +#define BT_ISO_TS          21
> >> > >
> >> > >  __printf(1, 2)
> >> > >  void bt_info(const char *fmt, ...);
> >> > > @@ -390,7 +391,8 @@ struct bt_sock {
> >> > >  enum {
> >> > >     BT_SK_DEFER_SETUP,
> >> > >     BT_SK_SUSPEND,
> >> > > -   BT_SK_PKT_STATUS
> >> > > +   BT_SK_PKT_STATUS,
> >> > > +   BT_SK_ISO_TS
> >> > >  };
> >> > >
> >> > >  struct bt_sock_list {
> >> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> >> > > index 2f348f48e99d..2c1fdea4b8c1 100644
> >> > > --- a/net/bluetooth/iso.c
> >> > > +++ b/net/bluetooth/iso.c
> >> > > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> >> > >             iso_pi(sk)->base_len = optlen;
> >> > >
> >> > >             break;
> >> > > +   case BT_ISO_TS:
> >> > > +           if (optlen != sizeof(opt)) {
> >> > > +                   err = -EINVAL;
> >> > > +                   break;
> >> > > +           }
> >> > >
> >> > > +           err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> >> > > +           if (err)
> >> > > +                   break;
> >> > > +
> >> > > +           if (opt)
> >> > > +                   set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > +           else
> >> > > +                   clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > +           break;
> >> > >     default:
> >> > >             err = -ENOPROTOOPT;
> >> > >             break;
> >> > > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> >> > >                     err = -EFAULT;
> >> > >
> >> > >             break;
> >> > > +   case BT_ISO_TS:
> >> > > +           if (len < sizeof(u32)) {
> >> > > +                   err = -EINVAL;
> >> > > +                   break;
> >> > > +           }
> >> > >
> >> > > +           if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> >> > > +                       (u32 __user *)optval))
> >> > > +                   err = -EFAULT;
> >> > > +           break;
> >> > >     default:
> >> > >             err = -ENOPROTOOPT;
> >> > >             break;
> >> > > @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
> >> > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >> > >  {
> >> > >     struct iso_conn *conn = hcon->iso_data;
> >> > > +   struct sock *sk;
> >> > >     __u16 pb, ts, len;
> >> > >
> >> > >     if (!conn)
> >> > >             goto drop;
> >> > >
> >> > > -   pb     = hci_iso_flags_pb(flags);
> >> > > -   ts     = hci_iso_flags_ts(flags);
> >> > > +   iso_conn_lock(conn);
> >> > > +   sk = conn->sk;
> >> > > +   iso_conn_unlock(conn);
> >> > > +
> >> > > +   if (!sk)
> >> > > +           goto drop;
> >> > > +
> >> > > +   pb = hci_iso_flags_pb(flags);
> >> > > +   ts = hci_iso_flags_ts(flags);
> >> > >
> >> > >     BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
> >> > >
> >> > > @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >> > >             if (ts) {
> >> > >                     struct hci_iso_ts_data_hdr *hdr;
> >> > >
> >> > > -                   /* TODO: add timestamp to the packet? */
> >> > > -                   hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> >> > > -                   if (!hdr) {
> >> > > -                           BT_ERR("Frame is too short (len %d)", skb->len);
> >> > > -                           goto drop;
> >> > > -                   }
> >> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> >> > > +                           hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> >> > > +                           len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> >> > > +                   } else {
> >> > > +                           hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> >> > > +                           if (!hdr) {
> >> > > +                                   BT_ERR("Frame is too short (len %d)", skb->len);
> >> > > +                                   goto drop;
> >> > > +                           }
> >> > >
> >> > > -                   len = __le16_to_cpu(hdr->slen);
> >> > > +                           len = __le16_to_cpu(hdr->slen);
> >> > > +                   }
> >> > >             } else {
> >> > >                     struct hci_iso_data_hdr *hdr;
> >> > >
> >> > > +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> >> > > +                           BT_ERR("Invalid option BT_SK_ISO_TS");
> >> > > +                           clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > +                   }
> >> > > +
> >> > >                     hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
> >> > >                     if (!hdr) {
> >> > >                             BT_ERR("Frame is too short (len %d)", skb->len);
> >> > >
> >> > > ---
> >> > > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> >> > > change-id: 20250421-iso_ts-c82a300ae784
> >> > >
> >> > > Best regards,
> >
> >
> >



-- 
Luiz Augusto von Dentz

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Yang Li]

Hi Luiz, Pauli
>
> Hi Pauli,
>
> On Tue, Apr 29, 2025 at 10:35 AM Pauli Virtanen <pav@iki.fi> wrote:
>> Hi,
>>
>> 29. huhtikuuta 2025 17.31.25 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com> kirjoitti:
>>> Hi Pauli,
>>>
>>> On Tue, Apr 29, 2025 at 10:29 AM Pauli Virtanen <pav@iki.fi> wrote:
>>>> ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
>>>>> Hi,
>>>>>
>>>>> ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
>>>>>> From: Yang Li <yang.li@amlogic.com>
>>>>>>
>>>>>> Application layer programs (like pipewire) need to use
>>>>>> iso timestamp information for audio synchronization.
>>>>> I think the timestamp should be put into CMSG, same ways as packet
>>>>> status is. The packet body should then always contain only the payload.
>>>> Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
>>>> mechanism, which would avoid adding a new API.
>>> Either that or we use BT_PKT_STATUS, does SOF_TIMESTAMPING_RX_HARDWARE
>>> requires the use of errqueue?
>> No, it just adds a CMSG, similar to the RX software tstamp.
> Perfect, then there should be no problem going with that, we might
> want to introduce some tests for it and perhaps have the emulator
> adding timestamps headers so we can test this with the likes of
> iso-tester.
Okay, let me try.
>
>>>>>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>>>>>> ---
>>>>>>   include/net/bluetooth/bluetooth.h |  4 ++-
>>>>>>   net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
>>>>>>   2 files changed, 52 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>>>>>> index bbefde319f95..a102bd76647c 100644
>>>>>> --- a/include/net/bluetooth/bluetooth.h
>>>>>> +++ b/include/net/bluetooth/bluetooth.h
>>>>>> @@ -242,6 +242,7 @@ struct bt_codecs {
>>>>>>   #define BT_CODEC_MSBC              0x05
>>>>>>
>>>>>>   #define BT_ISO_BASE                20
>>>>>> +#define BT_ISO_TS          21
>>>>>>
>>>>>>   __printf(1, 2)
>>>>>>   void bt_info(const char *fmt, ...);
>>>>>> @@ -390,7 +391,8 @@ struct bt_sock {
>>>>>>   enum {
>>>>>>      BT_SK_DEFER_SETUP,
>>>>>>      BT_SK_SUSPEND,
>>>>>> -   BT_SK_PKT_STATUS
>>>>>> +   BT_SK_PKT_STATUS,
>>>>>> +   BT_SK_ISO_TS
>>>>>>   };
>>>>>>
>>>>>>   struct bt_sock_list {
>>>>>> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>>>>>> index 2f348f48e99d..2c1fdea4b8c1 100644
>>>>>> --- a/net/bluetooth/iso.c
>>>>>> +++ b/net/bluetooth/iso.c
>>>>>> @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>>>>>>              iso_pi(sk)->base_len = optlen;
>>>>>>
>>>>>>              break;
>>>>>> +   case BT_ISO_TS:
>>>>>> +           if (optlen != sizeof(opt)) {
>>>>>> +                   err = -EINVAL;
>>>>>> +                   break;
>>>>>> +           }
>>>>>>
>>>>>> +           err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
>>>>>> +           if (err)
>>>>>> +                   break;
>>>>>> +
>>>>>> +           if (opt)
>>>>>> +                   set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>>>>>> +           else
>>>>>> +                   clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>>>>>> +           break;
>>>>>>      default:
>>>>>>              err = -ENOPROTOOPT;
>>>>>>              break;
>>>>>> @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
>>>>>>                      err = -EFAULT;
>>>>>>
>>>>>>              break;
>>>>>> +   case BT_ISO_TS:
>>>>>> +           if (len < sizeof(u32)) {
>>>>>> +                   err = -EINVAL;
>>>>>> +                   break;
>>>>>> +           }
>>>>>>
>>>>>> +           if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
>>>>>> +                       (u32 __user *)optval))
>>>>>> +                   err = -EFAULT;
>>>>>> +           break;
>>>>>>      default:
>>>>>>              err = -ENOPROTOOPT;
>>>>>>              break;
>>>>>> @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
>>>>>>   void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>>>>>>   {
>>>>>>      struct iso_conn *conn = hcon->iso_data;
>>>>>> +   struct sock *sk;
>>>>>>      __u16 pb, ts, len;
>>>>>>
>>>>>>      if (!conn)
>>>>>>              goto drop;
>>>>>>
>>>>>> -   pb     = hci_iso_flags_pb(flags);
>>>>>> -   ts     = hci_iso_flags_ts(flags);
>>>>>> +   iso_conn_lock(conn);
>>>>>> +   sk = conn->sk;
>>>>>> +   iso_conn_unlock(conn);
>>>>>> +
>>>>>> +   if (!sk)
>>>>>> +           goto drop;
>>>>>> +
>>>>>> +   pb = hci_iso_flags_pb(flags);
>>>>>> +   ts = hci_iso_flags_ts(flags);
>>>>>>
>>>>>>      BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
>>>>>>
>>>>>> @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>>>>>>              if (ts) {
>>>>>>                      struct hci_iso_ts_data_hdr *hdr;
>>>>>>
>>>>>> -                   /* TODO: add timestamp to the packet? */
>>>>>> -                   hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>>>>>> -                   if (!hdr) {
>>>>>> -                           BT_ERR("Frame is too short (len %d)", skb->len);
>>>>>> -                           goto drop;
>>>>>> -                   }
>>>>>> +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
>>>>>> +                           hdr = (struct hci_iso_ts_data_hdr *)skb->data;
>>>>>> +                           len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
>>>>>> +                   } else {
>>>>>> +                           hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>>>>>> +                           if (!hdr) {
>>>>>> +                                   BT_ERR("Frame is too short (len %d)", skb->len);
>>>>>> +                                   goto drop;
>>>>>> +                           }
>>>>>>
>>>>>> -                   len = __le16_to_cpu(hdr->slen);
>>>>>> +                           len = __le16_to_cpu(hdr->slen);
>>>>>> +                   }
>>>>>>              } else {
>>>>>>                      struct hci_iso_data_hdr *hdr;
>>>>>>
>>>>>> +                   if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
>>>>>> +                           BT_ERR("Invalid option BT_SK_ISO_TS");
>>>>>> +                           clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
>>>>>> +                   }
>>>>>> +
>>>>>>                      hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
>>>>>>                      if (!hdr) {
>>>>>>                              BT_ERR("Frame is too short (len %d)", skb->len);
>>>>>>
>>>>>> ---
>>>>>> base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
>>>>>> change-id: 20250421-iso_ts-c82a300ae784
>>>>>>
>>>>>> Best regards,
>>>
>>>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Apr 29, 2025 at 10:26 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> > From: Yang Li <yang.li@amlogic.com>
> >
> > Application layer programs (like pipewire) need to use
> > iso timestamp information for audio synchronization.
>
> I think the timestamp should be put into CMSG, same ways as packet
> status is. The packet body should then always contain only the payload.

Yes, we shall not have headers being sent to the application as part
of the payload.

> >
> > Signed-off-by: Yang Li <yang.li@amlogic.com>
> > ---
> >  include/net/bluetooth/bluetooth.h |  4 ++-
> >  net/bluetooth/iso.c               | 58 +++++++++++++++++++++++++++++++++------
> >  2 files changed, 52 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index bbefde319f95..a102bd76647c 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -242,6 +242,7 @@ struct bt_codecs {
> >  #define BT_CODEC_MSBC                0x05
> >
> >  #define BT_ISO_BASE          20
> > +#define BT_ISO_TS            21
> >
> >  __printf(1, 2)
> >  void bt_info(const char *fmt, ...);
> > @@ -390,7 +391,8 @@ struct bt_sock {
> >  enum {
> >       BT_SK_DEFER_SETUP,
> >       BT_SK_SUSPEND,
> > -     BT_SK_PKT_STATUS
> > +     BT_SK_PKT_STATUS,
> > +     BT_SK_ISO_TS
> >  };
> >
> >  struct bt_sock_list {
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 2f348f48e99d..2c1fdea4b8c1 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> >               iso_pi(sk)->base_len = optlen;
> >
> >               break;
> > +     case BT_ISO_TS:
> > +             if (optlen != sizeof(opt)) {
> > +                     err = -EINVAL;
> > +                     break;
> > +             }
> >
> > +             err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> > +             if (err)
> > +                     break;
> > +
> > +             if (opt)
> > +                     set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +             else
> > +                     clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +             break;
> >       default:
> >               err = -ENOPROTOOPT;
> >               break;
> > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> >                       err = -EFAULT;
> >
> >               break;
> > +     case BT_ISO_TS:
> > +             if (len < sizeof(u32)) {
> > +                     err = -EINVAL;
> > +                     break;
> > +             }
> >
> > +             if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> > +                         (u32 __user *)optval))
> > +                     err = -EFAULT;
> > +             break;
> >       default:
> >               err = -ENOPROTOOPT;
> >               break;
> > @@ -2271,13 +2294,21 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
> >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >  {
> >       struct iso_conn *conn = hcon->iso_data;
> > +     struct sock *sk;
> >       __u16 pb, ts, len;
> >
> >       if (!conn)
> >               goto drop;
> >
> > -     pb     = hci_iso_flags_pb(flags);
> > -     ts     = hci_iso_flags_ts(flags);
> > +     iso_conn_lock(conn);
> > +     sk = conn->sk;
> > +     iso_conn_unlock(conn);
> > +
> > +     if (!sk)
> > +             goto drop;
> > +
> > +     pb = hci_iso_flags_pb(flags);
> > +     ts = hci_iso_flags_ts(flags);
> >
> >       BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
> >
> > @@ -2294,17 +2325,26 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >               if (ts) {
> >                       struct hci_iso_ts_data_hdr *hdr;
> >
> > -                     /* TODO: add timestamp to the packet? */
> > -                     hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > -                     if (!hdr) {
> > -                             BT_ERR("Frame is too short (len %d)", skb->len);
> > -                             goto drop;
> > -                     }
> > +                     if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > +                             hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> > +                             len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> > +                     } else {
> > +                             hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> > +                             if (!hdr) {
> > +                                     BT_ERR("Frame is too short (len %d)", skb->len);
> > +                                     goto drop;
> > +                             }
> >
> > -                     len = __le16_to_cpu(hdr->slen);
> > +                             len = __le16_to_cpu(hdr->slen);
> > +                     }
> >               } else {
> >                       struct hci_iso_data_hdr *hdr;
> >
> > +                     if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> > +                             BT_ERR("Invalid option BT_SK_ISO_TS");
> > +                             clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> > +                     }
> > +
> >                       hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
> >                       if (!hdr) {
> >                               BT_ERR("Frame is too short (len %d)", skb->len);
> >
> > ---
> > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> > change-id: 20250421-iso_ts-c82a300ae784
> >
> > Best regards,
>
> --
> Pauli Virtanen



-- 
Luiz Augusto von Dentz

Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp

[kernel test robot]

Hi Yang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 16b4f97defefd93cfaea017a7c3e8849322f7dde]

url:    https://github.com/intel-lab-lkp/linux/commits/Yang-Li-via-B4-Relay/iso-add-BT_ISO_TS-optional-to-enable-ISO-timestamp/20250429-113716
base:   16b4f97defefd93cfaea017a7c3e8849322f7dde
patch link:    https://lore.kernel.org/r/20250429-iso_ts-v1-1-e586f30de6cb%40amlogic.com
patch subject: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
config: x86_64-randconfig-123-20250502 (https://download.01.org/0day-ci/archive/20250507/202505071336.tWe4dbxi-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250507/202505071336.tWe4dbxi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505071336.tWe4dbxi-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> net/bluetooth/iso.c:2330:42: sparse: sparse: restricted __le16 degrades to integer

vim +2330 net/bluetooth/iso.c

  2293	
  2294	void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
  2295	{
  2296		struct iso_conn *conn = hcon->iso_data;
  2297		struct sock *sk;
  2298		__u16 pb, ts, len;
  2299	
  2300		if (!conn)
  2301			goto drop;
  2302	
  2303		iso_conn_lock(conn);
  2304		sk = conn->sk;
  2305		iso_conn_unlock(conn);
  2306	
  2307		if (!sk)
  2308			goto drop;
  2309	
  2310		pb = hci_iso_flags_pb(flags);
  2311		ts = hci_iso_flags_ts(flags);
  2312	
  2313		BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
  2314	
  2315		switch (pb) {
  2316		case ISO_START:
  2317		case ISO_SINGLE:
  2318			if (conn->rx_len) {
  2319				BT_ERR("Unexpected start frame (len %d)", skb->len);
  2320				kfree_skb(conn->rx_skb);
  2321				conn->rx_skb = NULL;
  2322				conn->rx_len = 0;
  2323			}
  2324	
  2325			if (ts) {
  2326				struct hci_iso_ts_data_hdr *hdr;
  2327	
  2328				if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
  2329					hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> 2330					len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
  2331				} else {
  2332					hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
  2333					if (!hdr) {
  2334						BT_ERR("Frame is too short (len %d)", skb->len);
  2335						goto drop;
  2336					}
  2337	
  2338					len = __le16_to_cpu(hdr->slen);
  2339				}
  2340			} else {
  2341				struct hci_iso_data_hdr *hdr;
  2342	
  2343				if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
  2344					BT_ERR("Invalid option BT_SK_ISO_TS");
  2345					clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
  2346				}
  2347	
  2348				hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
  2349				if (!hdr) {
  2350					BT_ERR("Frame is too short (len %d)", skb->len);
  2351					goto drop;
  2352				}
  2353	
  2354				len = __le16_to_cpu(hdr->slen);
  2355			}
  2356	
  2357			flags  = hci_iso_data_flags(len);
  2358			len    = hci_iso_data_len(len);
  2359	
  2360			BT_DBG("Start: total len %d, frag len %d flags 0x%4.4x", len,
  2361			       skb->len, flags);
  2362	
  2363			if (len == skb->len) {
  2364				/* Complete frame received */
  2365				hci_skb_pkt_status(skb) = flags & 0x03;
  2366				iso_recv_frame(conn, skb);
  2367				return;
  2368			}
  2369	
  2370			if (pb == ISO_SINGLE) {
  2371				BT_ERR("Frame malformed (len %d, expected len %d)",
  2372				       skb->len, len);
  2373				goto drop;
  2374			}
  2375	
  2376			if (skb->len > len) {
  2377				BT_ERR("Frame is too long (len %d, expected len %d)",
  2378				       skb->len, len);
  2379				goto drop;
  2380			}
  2381	
  2382			/* Allocate skb for the complete frame (with header) */
  2383			conn->rx_skb = bt_skb_alloc(len, GFP_KERNEL);
  2384			if (!conn->rx_skb)
  2385				goto drop;
  2386	
  2387			hci_skb_pkt_status(conn->rx_skb) = flags & 0x03;
  2388			skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
  2389						  skb->len);
  2390			conn->rx_len = len - skb->len;
  2391			break;
  2392	
  2393		case ISO_CONT:
  2394			BT_DBG("Cont: frag len %d (expecting %d)", skb->len,
  2395			       conn->rx_len);
  2396	
  2397			if (!conn->rx_len) {
  2398				BT_ERR("Unexpected continuation frame (len %d)",
  2399				       skb->len);
  2400				goto drop;
  2401			}
  2402	
  2403			if (skb->len > conn->rx_len) {
  2404				BT_ERR("Fragment is too long (len %d, expected %d)",
  2405				       skb->len, conn->rx_len);
  2406				kfree_skb(conn->rx_skb);
  2407				conn->rx_skb = NULL;
  2408				conn->rx_len = 0;
  2409				goto drop;
  2410			}
  2411	
  2412			skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
  2413						  skb->len);
  2414			conn->rx_len -= skb->len;
  2415			return;
  2416	
  2417		case ISO_END:
  2418			skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
  2419						  skb->len);
  2420			conn->rx_len -= skb->len;
  2421	
  2422			if (!conn->rx_len) {
  2423				struct sk_buff *rx_skb = conn->rx_skb;
  2424	
  2425				/* Complete frame received. iso_recv_frame
  2426				 * takes ownership of the skb so set the global
  2427				 * rx_skb pointer to NULL first.
  2428				 */
  2429				conn->rx_skb = NULL;
  2430				iso_recv_frame(conn, rx_skb);
  2431			}
  2432			break;
  2433		}
  2434	
  2435	drop:
  2436		kfree_skb(skb);
  2437	}
  2438	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki