| Message ID | 1fd2d4523c139deda93aab2c31f1508d79c32472.1676921889.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e16f2ec5cf3a500c5c082d4cad5bd8a234c86361 |
| Headers | show |
| Series | Bluetooth: ISO: fix timestamped HCI ISO data packet parsing | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | 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: " " |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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
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 >
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 > > > > > > > >
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 > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > >
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!
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
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);
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(-)