Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bluetooth: Host: Fix L2CAP EINPROGRESS refs #76835

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Bluetooth: Host: Fix L2CAP EINPROGRESS refs #76835

wants to merge 5 commits into from
+390 −3

Conversation

alwa-nordic
Copy link
Collaborator

Fix discrepancy in reference management between calls to bt_l2cap_chan_ops.recv when the application returns -EINPROGRESS.

There are two call sites, l2cap_chan_le_recv_sdu and l2cap_chan_le_recv, that were inconsistent.

l2cap_chan_le_recv_sdu moves the reference, and this patch updates l2cap_chan_le_recv_sdu to do the same.

This behavior is also now documented.

This bug has existed since the introduction of this feature in 3151d26.

Signed-off-by: Aleksander Wasaznik [email protected]

@alwa-nordic alwa-nordic marked this pull request as ready for review August 8, 2024 15:30
@alwa-nordic alwa-nordic added the bug The issue is a bug, or the PR is fixing a bug label Aug 8, 2024
jori-nordic
jori-nordic reviewed Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@jori-nordic jori-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also clean-up and submit the test that found the bug.

@jhedberg
Copy link
Member

jhedberg commented Aug 9, 2024

GitHub doesn't allow me to do inline review of commit messages, so I'll do it here:

l2cap_chan_le_recv_sdu moves the reference, and this patch updates
l2cap_chan_le_recv_sdu to do the same.

The second function reference there should be l2cap_chan_le_recv, shouldn't it?

@zephyrbot zephyrbot added the platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim label Aug 9, 2024
@zephyrbot zephyrbot requested a review from aescolar August 9, 2024 12:26
@alwa-nordic
Copy link
Collaborator Author

Added test and fixed the commented-on issues.

@alwa-nordic
Copy link
Collaborator Author

I'll delete the tester code, and rewrite the test. Basing it on a different test was a mistake.

jori-nordic
jori-nordic previously approved these changes Sep 13, 2024
View reviewed changes
Thalley
Thalley previously approved these changes Sep 13, 2024
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers from me

include/zephyr/bluetooth/l2cap.h Outdated Show resolved Hide resolved
tests/bsim/bluetooth/host/l2cap/einprogress/compile.sh Show resolved Hide resolved
jhedberg
jhedberg reviewed Sep 13, 2024
View reviewed changes
subsys/bluetooth/host/hci_core.c
@@ -262,13 +263,23 @@ void bt_send_one_host_num_completed_packets(uint16_t handle)
BT_ASSERT_MSG(err == 0, "Unable to send Host NCP (err %d)", err);
}

#if defined(CONFIG_BT_TESTING)
__weak void bt_testing_trace_event_acl_pool_destroy(struct net_buf *buf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we might need a include/zephyr/bluetooth/testing.h which would contain the various prototypes of testing-related functions and also properly document them. I noticed we have some existing cases that could use it, like here:

zephyr/tests/bsim/bluetooth/host/misc/disconnect/dut/src/main.c

Lines 32 to 33 in 1471c2f

/* Defined in hci_core.c */
extern k_tid_t bt_testing_tx_tid_get(void);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to expose test functions in a public header file?

Honestly not sure what the best practices is for testing in Zephyr/C when it comes to things like this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like that concern should be solvable with proper documentation. These APIs either way require explicitly enabling CONFIG_BT_TESTING before they can be used. We could additionally output a build warning (if we don't already) that the configuration should not be used in production.

@alwa-nordic alwa-nordic dismissed stale reviews from Thalley and jori-nordic via dae83b6 October 14, 2024 12:04
@alwa-nordic alwa-nordic force-pushed the fix-ref-l2cap-einprogress branch 2 times, most recently from dae83b6 to 3ebe4e8 Compare October 14, 2024 12:09
@alwa-nordic
Bluetooth: Host: Fix bt_l2cap_chan_ops.recv -EINPROGRESS
21d1bc7 
Fix discrepancy in reference management between calls to
`bt_l2cap_chan_ops.recv` when the application returns `-EINPROGRESS`.

There are two call sites, `l2cap_chan_le_recv_sdu` and
`l2cap_chan_le_recv`, that were inconsistent.

`l2cap_chan_le_recv_sdu` moves the reference, and this patch updates
`l2cap_chan_le_recv` to do the same.

This behavior is also now documented.

This bug has existed since the introduction of this feature in
3151d26.

Signed-off-by: Aleksander Wasaznik <[email protected]>
@alwa-nordic
Bluetooth: Host: Upgrade log severity for L2CAP user error
185c5f2 
For ease of development, we should log the event as an error.

Signed-off-by: Aleksander Wasaznik <[email protected]>
@alwa-nordic
Bluetooth: Host: Add BT_TESTING trace event for ACL pool destroy
c8495a1 
This is needed for a test to catch a double-free.

Signed-off-by: Aleksander Wasaznik <[email protected]>
@alwa-nordic
Bluetooth: testlib: Add BT_TESTLIB_ADDR_LE_RANDOM_C0_00_00_00_00_
2c17ad1 
This is shorthand for random static addresses. It's similar to
`bt_addr_le_from_str`, but is a macro that results in an object literal,
making it more versatile and less verbose.

This macro only gives access to the first 255 random static addresses,
but this ought to be enough addresses for testing.

Signed-off-by: Aleksander Wasaznik <[email protected]>
@alwa-nordic
Bluetooth: Host: Test L2CAP -EINPROGRESS feature
83cebe8 
The test implementation is based on a copy of the HFC multilink test.
The test verifies that the stack respects the reference counting of SDU
buffers when the L2CAP -EINPROGRESS feature is used.

Signed-off-by: Aleksander Wasaznik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhedberg jhedberg jhedberg left review comments

@Thalley Thalley Thalley left review comments

@jori-nordic jori-nordic jori-nordic left review comments

@hermabe hermabe Awaiting requested review from hermabe

@sjanc sjanc Awaiting requested review from sjanc

@theob-pro theob-pro Awaiting requested review from theob-pro

@aescolar aescolar Awaiting requested review from aescolar

At least 2 approving reviews are required to merge this pull request.

Assignees

@jhedberg jhedberg

@jori-nordic jori-nordic

Labels
area: Bluetooth Host area: Bluetooth bug The issue is a bug, or the PR is fixing a bug platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alwa-nordic @jhedberg @Thalley @jori-nordic @zephyrbot