| Message ID | 20250429-iso_ts-v1-1-e586f30de6cb@amlogic.com |
|---|---|
| State | New |
| Headers | show |
| Series | iso: add BT_ISO_TS optional to enable ISO timestamp | expand |
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
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,
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,
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, > > >
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, > > > > > >
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
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
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);