Bluetooth: Userchan: Parse multiple hci packets #61764

vChavezB · 2023-08-22T21:47:59Z

After conducting tests with a a virtual Bluetooth controller over TCP it was noticed that some HCI packets may arrive on the same buffer if sent over a short period of time.
This update ensures the hci packets are parsed correctly in the case multiple packets are in the same recieving buffer according to how HCI packets are decoded in the Bluetooth Spec v5.2 Part E.

This should fix #61762

Open for suggestions and discussion on how to address this the best way.

drivers/bluetooth/hci/userchan.c

jhedberg · 2023-08-28T14:35:46Z

drivers/bluetooth/hci/userchan.c

This doesn't seem correct. You have the packet type as the first byte and the packet-specific header comes after that. In all other cases you read &packet_buff[1] but here you read &packet_buff[0]. Actually, I don't see any reason why you need two parameters to this function. Just pass the original buffer and inside the function you can then do uint8_t type = buf[0];.

Yes, sorry mistake on my part while changing to the suggestions of the other reviewer.

jhedberg · 2023-08-28T14:38:25Z

drivers/bluetooth/hci/userchan.c

This generates unnecessarily complicated code. Just do size = sys_le16_to_cpu(hdr->len);. The structs are packed, so accessing the struct members generates automatically alignment-safe code (if needed by the CPU architecture).

Does the packed attribute deal with endianness ? Thats the reason I thought parts like these ones are needed in the current bluetooth stack

https://github.com/zephyrproject-rtos/zephyr/blob/2d65acca3a76b968657c2e796512558e93f2ed23/drivers/bluetooth/hci/h4.c#L127C3-L127C3

https://github.com/zephyrproject-rtos/zephyr/blob/2d65acca3a76b968657c2e796512558e93f2ed23/drivers/bluetooth/hci/h4.c#L140C2-L140C2

Edit: I read again your comment, ok are you only referring to changing the name of the variable from payload size to size ?

I changed the other review comments accordingly but this one is still is not clear to me.

Could you be more specific on your suggestion?

Just do size = sys_le16_to_cpu(hdr->len)

I understood either:

Change the name of the variable payload_len to size ?

Obtain the size of the payload via size = sys_le16_to_cpu(hdr->len); before the switch case? Its a trivial change and only the edge case for the struct bt_hci_cmd_hdr has to be taken into account (param_len).

Sorry for the misunderstanding on my part, looking forward for your comments.

Ok, I think I overthought the comment. Changed to using sys_le16_to_cpu instead of sys_get_le16 and the lenght member of the respective hci header struct instead of the address.

jhedberg · 2023-08-28T14:39:58Z

drivers/bluetooth/hci/userchan.c

This seems like an unnecessary variable (doesn't add much to readability, and the way you've named it it's even longer thansizeof(packet_type).

I have changed it to now use it directly in the the return statement of the function.

jhedberg · 2023-08-28T14:41:15Z

drivers/bluetooth/hci/userchan.c

Call these len instead of size.

jhedberg · 2023-08-28T14:42:06Z

drivers/bluetooth/hci/userchan.c

I'd just call this function packet_len()

jhedberg · 2023-08-28T14:43:23Z

include/zephyr/bluetooth/hci_types.h

Please follow the same struct alignment style as elsewhere in the headers, so that the struct member names are aligned. That means two spaces after uint8_t but only one after uint16_t.

drivers/bluetooth/hci/userchan.c

include/zephyr/bluetooth/hci_types.h

drivers/bluetooth/hci/userchan.c

jhedberg

One more review round (thanks for your patience!). After that I think this is ready to be merged.

I think it'd also make sense to split the hci_types.h changes into its own commit (still in this same PR) before the userchan.c commit, since those changes are not strictly related to the userchan driver.

jhedberg · 2023-08-29T08:05:00Z

drivers/bluetooth/hci/userchan.c

Sorry if this is getting a bit overly pedantic, but I'd prefer if we stick to existing variable naming conventions and also avoid unnecessarily long names. This helps keep the code more readable (for someone who has read a lot of other Bluetooth subsystem code). and also avoids excessively long lines.

Just call the input parameter const uint8_t *buf

Oh no problem, I am aware that style and naming conventions are highly dependent on organisations policies. As I am still not that familiar with the codebase of Zephyr I know this could come up.

jhedberg · 2023-08-29T08:05:43Z

drivers/bluetooth/hci/userchan.c

const uint8_t type = buf[0]; const uint8_t hdr = &buf[sizeof(type)];

jhedberg · 2023-08-29T08:06:12Z

drivers/bluetooth/hci/userchan.c

const struct bt_hci_cmd_hdr *cmd = (const struct bt_hci_cmd_hdr *)hdr;

jhedberg · 2023-08-29T08:06:40Z

drivers/bluetooth/hci/userchan.c

const struct bt_hci_acl_hdr *acl = (const struct bt_hci_acl_hdr *)hdr;

jhedberg · 2023-08-29T08:07:00Z

drivers/bluetooth/hci/userchan.c

const struct bt_hci_sco_hdr *sco = (const struct bt_hci_sco_hdr *)hdr;

jhedberg · 2023-08-29T08:07:26Z

drivers/bluetooth/hci/userchan.c

const struct bt_hci_evt_hdr *evt = (const struct bt_hci_evt_hdr *)hdr;

jhedberg · 2023-08-29T08:07:49Z

drivers/bluetooth/hci/userchan.c

const struct bt_hci_iso_hdr *iso = (const struct bt_hci_iso_hdr *)hdr;

jhedberg · 2023-08-29T08:09:02Z

drivers/bluetooth/hci/userchan.c

Might make sense to add a LOG_WRN("Unknown packet type 0x%02x", type) before the return.

jhedberg · 2023-08-29T08:13:57Z

include/zephyr/bluetooth/hci_types.h

The other connection header structs (ACL & ISO) use simply handle here, so I'd stick to that for consistency (unless the HCI spec is actually fundamentally different in its naming conventions for these headers)

Ok, I just checked and the HCI spec uses Connection_Handle both for SCO and ISO, but just Handle for ACL. However, since the ISO struct already uses simply handle I'd follow that convention for SCO as well.

vChavezB · 2023-08-29T10:14:31Z

Added last suggestions. Pending separation of commits...

drivers/bluetooth/hci/userchan.c

Added HCI SCO header definition and included comments that refer to the Bluetooth core spec sections for the other HCI header types. Signed-off-by: Victor Chavez <[email protected]>

vChavezB · 2023-08-29T10:32:00Z

I have separated the commits.

Thalley · 2023-08-29T11:11:26Z

drivers/bluetooth/hci/userchan.c

Suggested change

payload_len = bt_iso_hdr_len(sys_le16_to_cpu(iso->len));

header_len = BT_HCI_ISO_HDR_SIZE;

}

payload_len = bt_iso_hdr_len(sys_le16_to_cpu(iso->len));

header_len = BT_HCI_ISO_HDR_SIZE;

break;

}

Thanks for noticing this, have been changing different parts that I must have deleted the break while refactoring.

Yeah, it existed earlier :D

After conducting tests with a a virtual Bluetooth controller over TCP it was noticed that some HCI packets may arrive on the same buffer if sent over a short period of time. This update ensures the hci packets are parsed correctly in the case multiple packets are in the same recieving buffer according to the Bluetooth Spec v5.4 Part E. Signed-off-by: Victor Chavez <[email protected]>

vChavezB force-pushed the bt_userchan_hci_packet_fix branch 9 times, most recently from 1afe8e4 to 40bd425 Compare August 22, 2023 22:28

vChavezB marked this pull request as ready for review August 26, 2023 08:34

vChavezB requested review from Vudentz, alwa-nordic and jhedberg as code owners August 26, 2023 08:34

zephyrbot added area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth labels Aug 26, 2023

zephyrbot requested review from Thalley, asbjornsabo, hermabe, jori-nordic, kruithofa, sjanc and theob-pro August 26, 2023 08:34

zephyrbot assigned jhedberg Aug 26, 2023

Thalley requested changes Aug 28, 2023

View reviewed changes

vChavezB force-pushed the bt_userchan_hci_packet_fix branch 6 times, most recently from e35b5ca to 35cc0d2 Compare August 28, 2023 13:25

vChavezB force-pushed the bt_userchan_hci_packet_fix branch 2 times, most recently from 49a7616 to e6f3dda Compare August 28, 2023 14:22

jhedberg requested changes Aug 28, 2023

View reviewed changes

vChavezB force-pushed the bt_userchan_hci_packet_fix branch 2 times, most recently from dd35ea2 to 57fdff7 Compare August 28, 2023 15:27

Thalley reviewed Aug 28, 2023

View reviewed changes

drivers/bluetooth/hci/userchan.c Outdated Show resolved Hide resolved

include/zephyr/bluetooth/hci_types.h Outdated Show resolved Hide resolved

vChavezB force-pushed the bt_userchan_hci_packet_fix branch 2 times, most recently from 4ff40fd to 0f51a22 Compare August 28, 2023 20:16

Thalley previously approved these changes Aug 28, 2023

View reviewed changes

drivers/bluetooth/hci/userchan.c Outdated Show resolved Hide resolved

vChavezB dismissed Thalley’s stale review via 3df2e5f August 28, 2023 20:57

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from 0f51a22 to 3df2e5f Compare August 28, 2023 20:57

Thalley previously approved these changes Aug 28, 2023

View reviewed changes

jhedberg requested changes Aug 29, 2023

View reviewed changes

vChavezB dismissed Thalley’s stale review via 758d0f8 August 29, 2023 10:12

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from 3df2e5f to 758d0f8 Compare August 29, 2023 10:12

Thalley reviewed Aug 29, 2023

View reviewed changes

drivers/bluetooth/hci/userchan.c Outdated Show resolved Hide resolved

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from 758d0f8 to 05d08cf Compare August 29, 2023 10:27

Bluetooth: controller: Added HCI SCO header definition

22b5012

Added HCI SCO header definition and included comments that refer to the Bluetooth core spec sections for the other HCI header types. Signed-off-by: Victor Chavez <[email protected]>

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from 05d08cf to 22b5012 Compare August 29, 2023 10:29

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from b5a6c51 to 5deee69 Compare August 29, 2023 11:09

Thalley requested changes Aug 29, 2023

View reviewed changes

vChavezB force-pushed the bt_userchan_hci_packet_fix branch from 5deee69 to fee9dc4 Compare August 29, 2023 11:15

Thalley approved these changes Aug 29, 2023

View reviewed changes

jhedberg approved these changes Aug 29, 2023

View reviewed changes

carlescufi merged commit 80d4c76 into zephyrproject-rtos:main Aug 30, 2023

patstew mentioned this pull request Mar 15, 2024

Bluetooth: userchan: Support TCP fragmentation of HCI packets #70322

Merged

vChavezB deleted the bt_userchan_hci_packet_fix branch August 8, 2024 21:44

-		payload_len =  bt_iso_hdr_len(sys_le16_to_cpu(iso->len));
-		header_len = BT_HCI_ISO_HDR_SIZE;
-	}
+		payload_len =  bt_iso_hdr_len(sys_le16_to_cpu(iso->len));
+		header_len = BT_HCI_ISO_HDR_SIZE;
+		break;
+	}

Bluetooth: Userchan: Parse multiple hci packets #61764

Bluetooth: Userchan: Parse multiple hci packets #61764

Uh oh!

Conversation

vChavezB commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vChavezB Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vChavezB Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vChavezB Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhedberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vChavezB commented Aug 22, 2023 •

edited

Loading

vChavezB Aug 28, 2023 •

edited

Loading

vChavezB Aug 28, 2023 •

edited

Loading

vChavezB Aug 28, 2023 •

edited

Loading