Bluetooth: Host: Fix redundant ACL Rx buffer count #99152
Conversation
cvinayak
force-pushed
the
github_hci_acl_rx_count_fix
branch
2 times, most recently
from
32861b7 to
7835776
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes redundant ACL Rx buffer allocation that was introduced when increasing HCI event buffer counts. The key change is removing the explicit reservation of HCI event buffers from the ACL buffer pool calculation.
- Removed
BT_BUF_HCI_EVT_RX_COUNTmacro that was reserving 1 buffer for HCI events - Introduced new buffer accounting macros (
BT_BUF_ACL_RX_COUNT_BLOCKED_MIN,BT_BUF_ACL_RX_COUNT_BLOCKED_MAX,BT_BUF_ACL_RX_COUNT_RESERVED) to better track buffer usage - Updated ACL buffer pool calculation to use
BT_BUF_HCI_ACL_RX_COUNT_EXTRAinstead ofBT_BUF_HCI_ACL_RX_COUNTto avoid double-counting
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| subsys/bluetooth/host/buf.c | Updated ACL input buffer pool definition to use BT_BUF_HCI_ACL_RX_COUNT_EXTRA and reformatted macro arguments for clarity |
| subsys/bluetooth/common/hci_common_internal.h | Removed BT_BUF_HCI_EVT_RX_COUNT, updated BT_BUF_HCI_ACL_RX_COUNT calculation, added BT_BUF_HCI_ACL_RX_COUNT_EXTRA definition, and improved consistency in buffer count calculations |
| include/zephyr/bluetooth/buf.h | Added new buffer accounting macros and updated BT_BUF_ACL_RX_COUNT_EXTRA to use MAX() for proper buffer reservation accounting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * BT_BUF_ACL_RX_COUNT_BLOCKED_MAX holds the application configured number of buffers across active | ||
| * connections for recombination of HCI data packets to L2CAP SDUs. |
There was a problem hiding this comment.
[nitpick] The comment mentions "the application configured number of buffers" but BT_BUF_ACL_RX_COUNT_BLOCKED_MAX is defined as CONFIG_BT_MAX_CONN (the maximum number of connections), not a direct buffer count configuration. While this represents the maximum buffers that can be blocked (one per connection), the phrasing could be clearer. Consider rephrasing to: "BT_BUF_ACL_RX_COUNT_BLOCKED_MAX represents the maximum number of buffers that can be blocked across active connections (one per connection) for recombination of HCI data packets to L2CAP SDUs."
| * BT_BUF_ACL_RX_COUNT_BLOCKED_MAX holds the application configured number of buffers across active | |
| * connections for recombination of HCI data packets to L2CAP SDUs. | |
| * BT_BUF_ACL_RX_COUNT_BLOCKED_MAX represents the maximum number of buffers that can be blocked across active | |
| * connections (one per connection) for recombination of HCI data packets to L2CAP SDUs. |
| * | ||
| * BT_BUF_HCI_ACL_RX_COUNT defines the number of available buffers for Controller to Host data | ||
| * flow control; keeping the application configured BT_BUF_ACL_RX_COUNT_EXTRA number of buffers | ||
| * available for L2CAP recombination, and a reserved number of buffers for processing HCI events. |
There was a problem hiding this comment.
The comment states that BT_BUF_HCI_ACL_RX_COUNT keeps "a reserved number of buffers for processing HCI events," but with the removal of BT_BUF_HCI_EVT_RX_COUNT in this PR, there is no longer an explicit reservation for HCI event buffers in the calculation. The comment should be updated to reflect the current implementation where HCI events share the event pool (CONFIG_BT_BUF_EVT_RX_COUNT) rather than being reserved from the ACL buffer count.
| * available for L2CAP recombination, and a reserved number of buffers for processing HCI events. | |
| * available for L2CAP recombination. HCI events now share the event pool (CONFIG_BT_BUF_EVT_RX_COUNT) | |
| * rather than having buffers reserved from the ACL buffer count. |
cvinayak
force-pushed
the
github_hci_acl_rx_count_fix
branch
from
7835776 to
65ec7b7
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * buffer is still blocked unreferencing, i.e. BT_BUF_ACL_RX_COUNT_BLOCKED_MIN. | ||
| * | ||
| * BT_BUF_HCI_ACL_RX_COUNT_EXTRA defines the outstanding number of HCI ACL data packets that the | ||
| * Controller can generated. |
There was a problem hiding this comment.
Corrected spelling of 'generated' to 'generate' to match the verb tense.
| * Controller can generated. | |
| * Controller can generate. |
Use the correct define in the HCI Command Tx buffer count calculation. There is no change in the calculated value. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
cvinayak
force-pushed
the
github_hci_acl_rx_count_fix
branch
2 times, most recently
from
653db55 to
46a585c
Compare
HCI ACL Rx buffer count is not required in addition, the calculated BT_BUF_ACL_RX_COUNT is sufficient for L2CAP recombination. But it is required to have additional Rx buffers for the HCI ACL Rx that can happen due to the generated HCI Host Number of Completed packets events. Fixes commit d382fca ("Bluetooth: Controller: Fix HCI command buffer allocation failure"). Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
cvinayak
force-pushed
the
github_hci_acl_rx_count_fix
branch
from
46a585c to
e85b163
Compare
|
1 participant
Fix redundant ACL Rx buffers introduced when increasing HCI event buffer count.