fix(third_party/bluetooth): fix bugs in USB host BT HCI drivers#411
fix(third_party/bluetooth): fix bugs in USB host BT HCI drivers#411sakumisu merged 1 commit intocherry-embedded:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdjusted USB-host Bluetooth HCI transport code in NimBLE and Zephyr trees: forwarding raw HCI buffers to H4 state machine, refactoring command/ACL handling to typed structures and consolidated buffers, re-enabled ISO payload-length validation, improved error returns, and added guards/casts to avoid warnings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@sakumisu This CI failure is not related to my PR (c3203ee). It's a pre-existing issue in port/hpmicro/usb_dc_hpm.c: #error "Please use SDK version 1.12.0 or later because of USB api modification" The HPM SDK version used in the CI environment is older than 1.12.0, so usb_dcd_set_sutw() and usb_dcd_get_sutw() are unavailable. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
third_party/zephyr_bluetooth-2.7.5/ble_hci_usbh.c (1)
314-332:⚠️ Potential issue | 🟠 MajorThread creation is still unguarded on reconnect.
s_registeredguards onlybt_hci_driver_register, but theusb_osal_thread_createcalls below run every timeusbh_bluetooth_runis invoked. On a USB reconnect you'll spawn additionalble_rx/ble_evt/ble_aclthreads — a resource leak that will cause duplicate RX dispatch.The threads do have exit paths (they detect
USB_ERR_SHUTDOWNfrom the USB core and callusb_osal_thread_delete), so they won't accumulate indefinitely. However, there is still a window where multiple thread instances coexist, particularly if the device reconnects faster than the old threads exit.Extend the
s_registeredguard to cover thread creation:🛡️ Minimal fix: extend the one-shot guard to cover thread creation
void usbh_bluetooth_run(struct usbh_bluetooth *bluetooth_class) { static bool s_registered; (void)bluetooth_class; if (!s_registered) { bt_hci_driver_register(&usbh_drv); - s_registered = true; - } `#ifdef` CONFIG_USBHOST_BLUETOOTH_HCI_H4 - usb_osal_thread_create("ble_rx", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_rx_thread, NULL); + usb_osal_thread_create("ble_rx", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_rx_thread, NULL); `#else` - usb_osal_thread_create("ble_evt", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_evt_rx_thread, NULL); - usb_osal_thread_create("ble_acl", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_acl_rx_thread, NULL); + usb_osal_thread_create("ble_evt", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_evt_rx_thread, NULL); + usb_osal_thread_create("ble_acl", 2048, CONFIG_USBHOST_PSC_PRIO + 1, usbh_bluetooth_hci_acl_rx_thread, NULL); `#endif` + s_registered = true; + } usbh_bluetooth_run_callback(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@third_party/zephyr_bluetooth-2.7.5/ble_hci_usbh.c` around lines 314 - 332, usbh_bluetooth_run currently only guards bt_hci_driver_register with the static s_registered flag, but still calls usb_osal_thread_create unconditionally; move/extend the s_registered check to also surround the usb_osal_thread_create calls so threads (usb_osal_thread_create for usbh_bluetooth_hci_rx_thread, usbh_bluetooth_hci_evt_rx_thread and usbh_bluetooth_hci_acl_rx_thread) are created only once per driver registration, leaving usbh_bluetooth_run_callback() outside if you need it always called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@third_party/nimble-1.6.0/ble_hci_usbh.c`:
- Around line 58-68: The function ble_transport_to_ll_acl_impl currently copies
pkt_len bytes into a stack buffer without validating pkt_len; add a bounds check
that pkt_len <= sizeof(buf) before calling os_mbuf_copydata and return a
suitable error (e.g., BLE_ERR_MEM_CAPACITY) if it is too large; additionally,
replace the large stack allocation with a heap or static buffer (or a fixed
bounded buffer) when MYNEWT_VAL(BLE_TRANSPORT_ACL_SIZE) can be large to avoid
stack overflow—update references in ble_transport_to_ll_acl_impl and ensure
os_mbuf_free_chain(om) still runs on error paths.
---
Outside diff comments:
In `@third_party/zephyr_bluetooth-2.7.5/ble_hci_usbh.c`:
- Around line 314-332: usbh_bluetooth_run currently only guards
bt_hci_driver_register with the static s_registered flag, but still calls
usb_osal_thread_create unconditionally; move/extend the s_registered check to
also surround the usb_osal_thread_create calls so threads
(usb_osal_thread_create for usbh_bluetooth_hci_rx_thread,
usbh_bluetooth_hci_evt_rx_thread and usbh_bluetooth_hci_acl_rx_thread) are
created only once per driver registration, leaving usbh_bluetooth_run_callback()
outside if you need it always called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9602e4a5-4f16-4f1a-91f0-a9dcbfbe8f6a
📒 Files selected for processing (2)
third_party/nimble-1.6.0/ble_hci_usbh.cthird_party/zephyr_bluetooth-2.7.5/ble_hci_usbh.c
NimBLE (nimble-1.6.0/ble_hci_usbh.c): - fix hci_read_callback: pass full buffer to hci_h4_sm_rx in one call instead of splitting type byte and payload into two calls - fix ble_transport_to_ll_cmd_impl: use struct ble_hci_cmd for type-safe length calculation instead of raw byte indexing - fix ble_transport_to_ll_acl_impl: gather mbuf with os_mbuf_copydata and issue single usbh_bluetooth_hci_write; the old per-segment loop violated USB Bluetooth class bulk transfer requirements - fix unused-parameter warnings in run/stop callbacks Zephyr (zephyr_bluetooth-2.7.5/ble_hci_usbh.c): - remove unreachable fallthrough comment after return statement - restore ISO payload length check (was commented out); use inline mask (& 0x3FFF) per BT Core Spec v5.x Vol 4 Part E Section 5.4.5 - fix bt_usbh_send error code: 255 -> -EIO - guard bt_hci_driver_register with static bool to prevent duplicate registration on USB reconnect - fix unused-parameter warnings in run/stop callbacks
2 participants
NimBLE (nimble-1.6.0/ble_hci_usbh.c):
Zephyr (zephyr_bluetooth-2.7.5/ble_hci_usbh.c):
Summary by CodeRabbit
Bug Fixes
Performance