[PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Pauli Virtanen]

Use correct HCI ISO data packet header struct when the packet has
timestamp. The timestamp, when present, goes before the other fields
(Core v5.3 4E 5.4.5), so the structs are not compatible.

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    My hardware doesn't seem to produce timestamped packets, so this is not
    properly tested, except to the extent that it doesn't break the
    non-timestamped code path.
    
    Regardless, the current state of things looks wrong, so sending this to
    the list in any case.

 net/bluetooth/iso.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 2dabef488eaa..cb959e8eac18 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
 	__u16 pb, ts, len;
 
 	if (!conn)
@@ -1643,6 +1642,8 @@ 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) {
@@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 				goto drop;
 			}
 
+			len = __le16_to_cpu(hdr->slen);
 		} else {
+			struct hci_iso_data_hdr *hdr;
+
 			hdr = skb_pull_data(skb, HCI_ISO_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);
 		flags  = hci_iso_data_flags(len);
 		len    = hci_iso_data_len(len);
 
-- 
2.39.2

RE: Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.69 seconds
GitLint                       FAIL      0.56 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      31.92 seconds
CheckAllWarning               PASS      34.65 seconds
CheckSparse                   PASS      39.58 seconds
CheckSmatch                   PASS      107.42 seconds
BuildKernel32                 PASS      30.83 seconds
TestRunnerSetup               PASS      439.94 seconds
TestRunner_l2cap-tester       PASS      16.53 seconds
TestRunner_iso-tester         PASS      17.75 seconds
TestRunner_bnep-tester        PASS      5.75 seconds
TestRunner_mgmt-tester        PASS      112.93 seconds
TestRunner_rfcomm-tester      PASS      9.04 seconds
TestRunner_sco-tester         PASS      8.46 seconds
TestRunner_ioctl-tester       PASS      9.90 seconds
TestRunner_mesh-tester        PASS      7.18 seconds
TestRunner_smp-tester         PASS      8.27 seconds
TestRunner_userchan-tester    PASS      6.00 seconds
IncrementalBuild              PASS      29.22 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
14: B2 Line has trailing whitespace: "    "


---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Use correct HCI ISO data packet header struct when the packet has
> timestamp. The timestamp, when present, goes before the other fields
> (Core v5.3 4E 5.4.5), so the structs are not compatible.
>
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
>     My hardware doesn't seem to produce timestamped packets, so this is not
>     properly tested, except to the extent that it doesn't break the
>     non-timestamped code path.
>
>     Regardless, the current state of things looks wrong, so sending this to
>     the list in any case.

Perhaps we should first attempt to enable this in the emulator, Id use
BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
knowing the timestamp:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752

>  net/bluetooth/iso.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 2dabef488eaa..cb959e8eac18 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
>         __u16 pb, ts, len;
>
>         if (!conn)
> @@ -1643,6 +1642,8 @@ 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? */

Perhaps we should use skb_set_delivery_time to set the timestamp as
received by the controller? That said I don't know if the unit would
be compatible.

>                         hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>                         if (!hdr) {
> @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>                                 goto drop;
>                         }
>
> +                       len = __le16_to_cpu(hdr->slen);
>                 } else {
> +                       struct hci_iso_data_hdr *hdr;
> +
>                         hdr = skb_pull_data(skb, HCI_ISO_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);
>                 flags  = hci_iso_data_flags(len);
>                 len    = hci_iso_data_len(len);
>
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Pauli Virtanen]

Hi Luiz,

ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> > 
> > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > 
> > > > Use correct HCI ISO data packet header struct when the packet has
> > > > timestamp. The timestamp, when present, goes before the other fields
> > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > 
> > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > ---
> > > > 
> > > > Notes:
> > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > >     properly tested, except to the extent that it doesn't break the
> > > >     non-timestamped code path.
> > > > 
> > > >     Regardless, the current state of things looks wrong, so sending this to
> > > >     the list in any case.
> > 
> > Perhaps we should first attempt to enable this in the emulator, Id use
> > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > knowing the timestamp:
> > 
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752

Yes, it probably would be best, and probably not too complicated to do
at least if we are happy with not per spec timestamps.

> > > >  net/bluetooth/iso.c | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > --- a/net/bluetooth/iso.c
> > > > +++ b/net/bluetooth/iso.c
> > > > @@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
> > > >         __u16 pb, ts, len;
> > > > 
> > > >         if (!conn)
> > > > @@ -1643,6 +1642,8 @@ 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?
> > > > */
> > 
> > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > received by the controller? That said I don't know if the unit
> > would
> > be compatible.

The userspace I think is interested also in the sequence number and
packet status flags (if controller knows data is corrupted, that should
be indicated to userspace, similarly for missed packets). Would e.g.
putting all these into some custom cmsg struct be better here?

This timestamp is also based on controller clock. Not sure if
delivery_time is supposed to be in system clock not some unrelated
hardware clock.

Userspace may also want to set the packet sequence numbers and
timestamps itself when sending, to indicate in which interval it wanted
things to come out.

On systems where synchronization to Controller ISO clock is not
available (it IIUC cannot really be done via HCI and so this is likely
the situation on desktop/laptop machines), userspace needs some other
tool to keep its SDU timing in sync with the controller, and for e.g.
keeping a fixed number of packets queued to have some buffer for missed
deadlines, and making sure there is no timing offset between eg. left
and right earpieces of TWS headset.

This probably means things like information on the timing of the latest
Number of Completed Packets event for the handle, the number of
currently queued ISO packets on the controller, and the sequence number
of the latest packet queued on controller (to know how much is still in
socket buffer). Probably also LE Read TX Sync information to know
controller timestamp on a sent packet...

Does this go via some new ioctls? Or send empty packets with some cmsg
(so userspace can just poll until it needs to do something)?

It should also be decided how much the kernel is going to be involved
in getting the SDU timing, sequence numbers, etc. right, or whether
it's just left to the userspace.

Before trying to propose something here, I think I'd need to first make
some prototypes so that it becomes clearer how it would best work.


> > > >                         hdr = skb_pull_data(skb,
> > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > >                         if (!hdr) {
> > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > struct sk_buff *skb, u16 flags)
> > > >                                 goto drop;
> > > >                         }
> > > > 
> > > > +                       len = __le16_to_cpu(hdr->slen);
> > > >                 } else {
> > > > +                       struct hci_iso_data_hdr *hdr;
> > > > +
> > > >                         hdr = skb_pull_data(skb,
> > > > HCI_ISO_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);
> > > >                 flags  = hci_iso_data_flags(len);
> > > >                 len    = hci_iso_data_len(len);
> > > > 
> > > > --
> > > > 2.39.2
> > > > 
> > 
> > 

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Luiz Augusto von Dentz]

Hi Pauli,

On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > Hi Pauli,
> > >
> > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > >
> > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > >
> > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > >     properly tested, except to the extent that it doesn't break the
> > > > >     non-timestamped code path.
> > > > >
> > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > >     the list in any case.
> > >
> > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > knowing the timestamp:
> > >
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
>
> Yes, it probably would be best, and probably not too complicated to do
> at least if we are happy with not per spec timestamps.
>
> > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > --- a/net/bluetooth/iso.c
> > > > > +++ b/net/bluetooth/iso.c
> > > > > @@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
> > > > >         __u16 pb, ts, len;
> > > > >
> > > > >         if (!conn)
> > > > > @@ -1643,6 +1642,8 @@ 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?
> > > > > */
> > >
> > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > received by the controller? That said I don't know if the unit
> > > would
> > > be compatible.
>
> The userspace I think is interested also in the sequence number and
> packet status flags (if controller knows data is corrupted, that should
> be indicated to userspace, similarly for missed packets). Would e.g.
> putting all these into some custom cmsg struct be better here?
>
> This timestamp is also based on controller clock. Not sure if
> delivery_time is supposed to be in system clock not some unrelated
> hardware clock.
>
> Userspace may also want to set the packet sequence numbers and
> timestamps itself when sending, to indicate in which interval it wanted
> things to come out.
>
> On systems where synchronization to Controller ISO clock is not
> available (it IIUC cannot really be done via HCI and so this is likely
> the situation on desktop/laptop machines), userspace needs some other
> tool to keep its SDU timing in sync with the controller, and for e.g.
> keeping a fixed number of packets queued to have some buffer for missed
> deadlines, and making sure there is no timing offset between eg. left
> and right earpieces of TWS headset.

Yeah, btw I will be posting the patches for CSIP soon and I wonder if
you could check with this headset of yours, in fact we probably need
some changes to the likes of pipewire to detect the presence of
DeviceSet object and split the left and right streams.

> This probably means things like information on the timing of the latest
> Number of Completed Packets event for the handle, the number of
> currently queued ISO packets on the controller, and the sequence number
> of the latest packet queued on controller (to know how much is still in
> socket buffer). Probably also LE Read TX Sync information to know
> controller timestamp on a sent packet...
>
> Does this go via some new ioctls? Or send empty packets with some cmsg
> (so userspace can just poll until it needs to do something)?
>
> It should also be decided how much the kernel is going to be involved
> in getting the SDU timing, sequence numbers, etc. right, or whether
> it's just left to the userspace.
>
> Before trying to propose something here, I think I'd need to first make
> some prototypes so that it becomes clearer how it would best work.

I think we might want to use BT_PKT_STATUS, like is done for SCO, to
carry information such as controller timestamp, etc, have a look at
it:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450

>
> > > > >                         hdr = skb_pull_data(skb,
> > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > >                         if (!hdr) {
> > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > struct sk_buff *skb, u16 flags)
> > > > >                                 goto drop;
> > > > >                         }
> > > > >
> > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > >                 } else {
> > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > +
> > > > >                         hdr = skb_pull_data(skb,
> > > > > HCI_ISO_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);
> > > > >                 flags  = hci_iso_data_flags(len);
> > > > >                 len    = hci_iso_data_len(len);
> > > > >
> > > > > --
> > > > > 2.39.2
> > > > >
> > >
> > >
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Pauli Virtanen]

Hi Luiz,

pe, 2023-02-24 kello 12:41 -0800, Luiz Augusto von Dentz kirjoitti:
> On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > > Hi Pauli,
> > > > 
> > > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > > 
> > > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > > > 
> > > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > > ---
> > > > > > 
> > > > > > Notes:
> > > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > > >     properly tested, except to the extent that it doesn't break the
> > > > > >     non-timestamped code path.
> > > > > > 
> > > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > > >     the list in any case.
> > > > 
> > > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > > knowing the timestamp:
> > > > 
> > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
> > 
> > Yes, it probably would be best, and probably not too complicated to do
> > at least if we are happy with not per spec timestamps.
> > 
> > > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > > --- a/net/bluetooth/iso.c
> > > > > > +++ b/net/bluetooth/iso.c
> > > > > > @@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
> > > > > >         __u16 pb, ts, len;
> > > > > > 
> > > > > >         if (!conn)
> > > > > > @@ -1643,6 +1642,8 @@ 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?
> > > > > > */
> > > > 
> > > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > > received by the controller? That said I don't know if the unit
> > > > would
> > > > be compatible.
> > 
> > The userspace I think is interested also in the sequence number and
> > packet status flags (if controller knows data is corrupted, that should
> > be indicated to userspace, similarly for missed packets). Would e.g.
> > putting all these into some custom cmsg struct be better here?
> > 
> > This timestamp is also based on controller clock. Not sure if
> > delivery_time is supposed to be in system clock not some unrelated
> > hardware clock.
> > 
> > Userspace may also want to set the packet sequence numbers and
> > timestamps itself when sending, to indicate in which interval it wanted
> > things to come out.
> > 
> > On systems where synchronization to Controller ISO clock is not
> > available (it IIUC cannot really be done via HCI and so this is likely
> > the situation on desktop/laptop machines), userspace needs some other
> > tool to keep its SDU timing in sync with the controller, and for e.g.
> > keeping a fixed number of packets queued to have some buffer for missed
> > deadlines, and making sure there is no timing offset between eg. left
> > and right earpieces of TWS headset.
> 
> Yeah, btw I will be posting the patches for CSIP soon and I wonder if
> you could check with this headset of yours, in fact we probably need
> some changes to the likes of pipewire to detect the presence of
> DeviceSet object and split the left and right streams.

I'll test the CSIP patchset with the Samsung TWS headset I have. I'm
also going to make the necessary changes to Pipewire, to see how it
works from our side.

FWIW, right now, there are issues in getting reliable BAP audio on this
TWS headset device (with one earbud only currently) --- I'm sending
unframed LC3 packets to the ISO socket with sub-ms timing accuracy (err
~ 0.2ms max, 50us std) and there are still frequent pops in sine wave.
Stuffing extra 2-4 packets to the socket at the start of playback seems
to reduce the problem, so maybe the tx sync issue is suspect.

However, no such problem with nrf5430 or ax210+bluez+pipewire as
receiver, so not really yet clear.

> > This probably means things like information on the timing of the latest
> > Number of Completed Packets event for the handle, the number of
> > currently queued ISO packets on the controller, and the sequence number
> > of the latest packet queued on controller (to know how much is still in
> > socket buffer). Probably also LE Read TX Sync information to know
> > controller timestamp on a sent packet...
> > 
> > Does this go via some new ioctls? Or send empty packets with some cmsg
> > (so userspace can just poll until it needs to do something)?
> > 
> > It should also be decided how much the kernel is going to be involved
> > in getting the SDU timing, sequence numbers, etc. right, or whether
> > it's just left to the userspace.
> > 
> > Before trying to propose something here, I think I'd need to first make
> > some prototypes so that it becomes clearer how it would best work.
> 
> I think we might want to use BT_PKT_STATUS, like is done for SCO, to
> carry information such as controller timestamp, etc, have a look at
> it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450


Yes, for RX that's probably what it should be.

> > 
> > > > > >                         hdr = skb_pull_data(skb,
> > > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > > >                         if (!hdr) {
> > > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > struct sk_buff *skb, u16 flags)
> > > > > >                                 goto drop;
> > > > > >                         }
> > > > > > 
> > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > >                 } else {
> > > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > > +
> > > > > >                         hdr = skb_pull_data(skb,
> > > > > > HCI_ISO_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);
> > > > > >                 flags  = hci_iso_data_flags(len);
> > > > > >                 len    = hci_iso_data_len(len);
> > > > > > 
> > > > > > --
> > > > > > 2.39.2
> > > > > > 
> > > > 
> > > > 
> > 
> > 
> 
> 

-- 
Pauli Virtanen

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[Luiz Augusto von Dentz]

Hi Pauli,

On Fri, Feb 24, 2023 at 2:55 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> pe, 2023-02-24 kello 12:41 -0800, Luiz Augusto von Dentz kirjoitti:
> > On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > > > Hi Pauli,
> > > > >
> > > > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > > >
> > > > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > > > >
> > > > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > > > >     properly tested, except to the extent that it doesn't break the
> > > > > > >     non-timestamped code path.
> > > > > > >
> > > > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > > > >     the list in any case.
> > > > >
> > > > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > > > knowing the timestamp:
> > > > >
> > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
> > >
> > > Yes, it probably would be best, and probably not too complicated to do
> > > at least if we are happy with not per spec timestamps.
> > >
> > > > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > > > --- a/net/bluetooth/iso.c
> > > > > > > +++ b/net/bluetooth/iso.c
> > > > > > > @@ -1621,7 +1621,6 @@ 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 hci_iso_data_hdr *hdr;
> > > > > > >         __u16 pb, ts, len;
> > > > > > >
> > > > > > >         if (!conn)
> > > > > > > @@ -1643,6 +1642,8 @@ 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?
> > > > > > > */
> > > > >
> > > > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > > > received by the controller? That said I don't know if the unit
> > > > > would
> > > > > be compatible.
> > >
> > > The userspace I think is interested also in the sequence number and
> > > packet status flags (if controller knows data is corrupted, that should
> > > be indicated to userspace, similarly for missed packets). Would e.g.
> > > putting all these into some custom cmsg struct be better here?
> > >
> > > This timestamp is also based on controller clock. Not sure if
> > > delivery_time is supposed to be in system clock not some unrelated
> > > hardware clock.
> > >
> > > Userspace may also want to set the packet sequence numbers and
> > > timestamps itself when sending, to indicate in which interval it wanted
> > > things to come out.
> > >
> > > On systems where synchronization to Controller ISO clock is not
> > > available (it IIUC cannot really be done via HCI and so this is likely
> > > the situation on desktop/laptop machines), userspace needs some other
> > > tool to keep its SDU timing in sync with the controller, and for e.g.
> > > keeping a fixed number of packets queued to have some buffer for missed
> > > deadlines, and making sure there is no timing offset between eg. left
> > > and right earpieces of TWS headset.
> >
> > Yeah, btw I will be posting the patches for CSIP soon and I wonder if
> > you could check with this headset of yours, in fact we probably need
> > some changes to the likes of pipewire to detect the presence of
> > DeviceSet object and split the left and right streams.
>
> I'll test the CSIP patchset with the Samsung TWS headset I have. I'm
> also going to make the necessary changes to Pipewire, to see how it
> works from our side.

I just posted an RFC set:

https://patchwork.kernel.org/project/bluetooth/list/?series=726293

Make sure you also have the set bellow, apparently this was the cause
of transport not showing up since bap_ready callback was called to
early before we had subscribed for notifications, etc:

https://patchwork.kernel.org/project/bluetooth/list/?series=726232

> FWIW, right now, there are issues in getting reliable BAP audio on this
> TWS headset device (with one earbud only currently) --- I'm sending
> unframed LC3 packets to the ISO socket with sub-ms timing accuracy (err
> ~ 0.2ms max, 50us std) and there are still frequent pops in sine wave.
> Stuffing extra 2-4 packets to the socket at the start of playback seems
> to reduce the problem, so maybe the tx sync issue is suspect.

Yeah, Im afraid the headset might be trying to minimize using a jitter
buffer so if we miss any internal it will cause such problem, the
other problem is that the controller may not be reporting the Number
of Packets Complete one by one, which cause us to send packets in
batches which end up removing the timing accuracy of the socket since
we have to wait the controller to acknowledge the packets previously
sent, perhaps we could try detecting this behavior and disable Number
of Packets Complete flow control, so we send based on the configured
interval no matter if we think there is a buffer available in the
controller or not.

> However, no such problem with nrf5430 or ax210+bluez+pipewire as
> receiver, so not really yet clear.
>
> > > This probably means things like information on the timing of the latest
> > > Number of Completed Packets event for the handle, the number of
> > > currently queued ISO packets on the controller, and the sequence number
> > > of the latest packet queued on controller (to know how much is still in
> > > socket buffer). Probably also LE Read TX Sync information to know
> > > controller timestamp on a sent packet...
> > >
> > > Does this go via some new ioctls? Or send empty packets with some cmsg
> > > (so userspace can just poll until it needs to do something)?
> > >
> > > It should also be decided how much the kernel is going to be involved
> > > in getting the SDU timing, sequence numbers, etc. right, or whether
> > > it's just left to the userspace.
> > >
> > > Before trying to propose something here, I think I'd need to first make
> > > some prototypes so that it becomes clearer how it would best work.
> >
> > I think we might want to use BT_PKT_STATUS, like is done for SCO, to
> > carry information such as controller timestamp, etc, have a look at
> > it:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450
>
>
> Yes, for RX that's probably what it should be.
>
> > >
> > > > > > >                         hdr = skb_pull_data(skb,
> > > > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > > > >                         if (!hdr) {
> > > > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > > struct sk_buff *skb, u16 flags)
> > > > > > >                                 goto drop;
> > > > > > >                         }
> > > > > > >
> > > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > > >                 } else {
> > > > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > > > +
> > > > > > >                         hdr = skb_pull_data(skb,
> > > > > > > HCI_ISO_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);
> > > > > > >                 flags  = hci_iso_data_flags(len);
> > > > > > >                 len    = hci_iso_data_len(len);
> > > > > > >
> > > > > > > --
> > > > > > > 2.39.2
> > > > > > >
> > > > >
> > > > >
> > >
> > >
> >
> >
>
> --
> Pauli Virtanen



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 20 Feb 2023 19:38:24 +0000 you wrote:
> Use correct HCI ISO data packet header struct when the packet has
> timestamp. The timestamp, when present, goes before the other fields
> (Core v5.3 4E 5.4.5), so the structs are not compatible.
> 
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> 
> [...]

Here is the summary with links:
  - Bluetooth: ISO: fix timestamped HCI ISO data packet parsing
    https://git.kernel.org/bluetooth/bluetooth-next/c/e16f2ec5cf3a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html