* [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
@ 2025-04-29 3:35 Yang Li via B4 Relay
2025-04-29 4:35 ` bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Yang Li via B4 Relay @ 2025-04-29 3:35 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman
Cc: linux-bluetooth, netdev, linux-kernel, Yang Li
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>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 3:35 [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp Yang Li via B4 Relay
@ 2025-04-29 4:35 ` bluez.test.bot
2025-04-29 14:26 ` [PATCH] " Pauli Virtanen
2025-05-07 6:19 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2025-04-29 4:35 UTC (permalink / raw)
To: linux-bluetooth, yang.li
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 3:35 [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp Yang Li via B4 Relay
2025-04-29 4:35 ` bluez.test.bot
@ 2025-04-29 14:26 ` Pauli Virtanen
2025-04-29 14:29 ` Pauli Virtanen
2025-04-29 14:30 ` Luiz Augusto von Dentz
2025-05-07 6:19 ` kernel test robot
2 siblings, 2 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-04-29 14:26 UTC (permalink / raw)
To: yang.li; +Cc: linux-bluetooth, netdev, Luiz Augusto von Dentz
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:26 ` [PATCH] " Pauli Virtanen
@ 2025-04-29 14:29 ` Pauli Virtanen
2025-04-29 14:31 ` Luiz Augusto von Dentz
2025-04-29 14:30 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 10+ messages in thread
From: Pauli Virtanen @ 2025-04-29 14:29 UTC (permalink / raw)
To: yang.li; +Cc: linux-bluetooth, netdev, Luiz Augusto von Dentz
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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:26 ` [PATCH] " Pauli Virtanen
2025-04-29 14:29 ` Pauli Virtanen
@ 2025-04-29 14:30 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-29 14:30 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: yang.li, linux-bluetooth, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:29 ` Pauli Virtanen
@ 2025-04-29 14:31 ` Luiz Augusto von Dentz
2025-04-29 14:35 ` Pauli Virtanen
0 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-29 14:31 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: yang.li, linux-bluetooth, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:31 ` Luiz Augusto von Dentz
@ 2025-04-29 14:35 ` Pauli Virtanen
2025-04-29 14:38 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 10+ messages in thread
From: Pauli Virtanen @ 2025-04-29 14:35 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: yang.li, linux-bluetooth, netdev
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,
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:35 ` Pauli Virtanen
@ 2025-04-29 14:38 ` Luiz Augusto von Dentz
2025-04-30 7:45 ` Yang Li
0 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-29 14:38 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: yang.li, linux-bluetooth, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 14:38 ` Luiz Augusto von Dentz
@ 2025-04-30 7:45 ` Yang Li
0 siblings, 0 replies; 10+ messages in thread
From: Yang Li @ 2025-04-30 7:45 UTC (permalink / raw)
To: Luiz Augusto von Dentz, Pauli Virtanen; +Cc: linux-bluetooth, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
2025-04-29 3:35 [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp Yang Li via B4 Relay
2025-04-29 4:35 ` bluez.test.bot
2025-04-29 14:26 ` [PATCH] " Pauli Virtanen
@ 2025-05-07 6:19 ` kernel test robot
2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-05-07 6:19 UTC (permalink / raw)
To: Yang Li via B4 Relay, Marcel Holtmann, Johan Hedberg,
Luiz Augusto von Dentz, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman
Cc: oe-kbuild-all, netdev, linux-bluetooth, linux-kernel, Yang Li
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-05-07 6:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-29 3:35 [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp Yang Li via B4 Relay
2025-04-29 4:35 ` bluez.test.bot
2025-04-29 14:26 ` [PATCH] " Pauli Virtanen
2025-04-29 14:29 ` Pauli Virtanen
2025-04-29 14:31 ` Luiz Augusto von Dentz
2025-04-29 14:35 ` Pauli Virtanen
2025-04-29 14:38 ` Luiz Augusto von Dentz
2025-04-30 7:45 ` Yang Li
2025-04-29 14:30 ` Luiz Augusto von Dentz
2025-05-07 6:19 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox