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.

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

Bluetooth: Host: Ensure conn_ready access is thread safe #83700

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

Yagoor
Copy link

@Yagoor Yagoor commented Jan 8, 2025

  • The bt_dev.le.conn_ready list is accessed by the tx_processor
    which runs in a workqueue, but bt_conn_data_ready can be called
    from different threads so we need to make sure that nothing will
    trigger a context switch while we are manipulating the list since
    sys_slist_*() functions are not thread safe.
  • This only happens if call to bt_conn_data_ready is performed
    from a preemptive task which can happen depending on the
    application.

* The `bt_dev.le.conn_ready` list is accessed by the tx_processor
which runs in a workqueue, but `bt_conn_data_ready` can be called
from different threads so we need to make sure that nothing will
trigger a context switch while we are manipulating the list since
sys_slist_*() functions are not thread safe.
* This only happens if call to `bt_conn_data_ready` is performed
from a preemptive task which can happen depending on the
application.

Signed-off-by: Yago Fontoura do Rosario <[email protected]>
@Yagoor Yagoor marked this pull request as ready for review January 9, 2025 08:19
@zephyrbot zephyrbot added area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Jan 9, 2025
* which runs in a workqueue, but `bt_conn_data_ready` can be called
* from different threads so we need to make sure that nothing will
* trigger a thread switch while we are manipulating the list since
* sys_slist_*() functions are not thread safe.
Copy link
Collaborator

Choose a reason for hiding this comment

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

get_conn_ready also manipulates the list by doing

	if (should_stop_tx(conn)) {
		/* Move reference off the list and into the `conn` variable. */
		__maybe_unused sys_snode_t *s = sys_slist_get(&bt_dev.le.conn_ready);

		__ASSERT_NO_MSG(s == node);
		(void)atomic_set(&conn->_conn_ready_lock, 0);

		/* Append connection to list if it still has data */
		if (conn->has_data(conn)) {
			LOG_DBG("appending %p to back of TX queue", conn);
			bt_conn_data_ready(conn);
		}

		return conn;
	}

since sys_slist_get pops the list. Should this also be guarded by a k_sched_lock?

Copy link
Author

Choose a reason for hiding this comment

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

That one is okay, because get_conn_ready is called on bt_conn_tx_processor which is executed in the system workqueue. Since the system workqueue is being executed at a cooperative priority, we don't need it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need it here.

for now :D

@kartben kartben merged commit 9f77362 into zephyrproject-rtos:main Jan 13, 2025
33 checks passed
@kartben
Copy link
Collaborator

kartben commented Jan 13, 2025

@Yagoor this is failing in main, please have a look:

west twister -p unit_testing/unit_testing -s bluetooth.host.conn.test_default --no-detailed-test-id


...
: && /usr/bin/gcc -m32 -gdwarf-4 -T /__w/zephyr/zephyr/subsys/testsuite/include/zephyr/ztest_unittest.ld -T /__w/zephyr/zephyr/tests/bluetooth/host/conn/mocks/mock-sections.ld CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/testsuite/ztest/src/ztest.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/testsuite/ztest/src/ztest_mock.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/testsuite/ztest/src/ztest_rules.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/testsuite/ztest/src/ztest_defaults.c.obj CMakeFiles/testbinary.dir/src/main.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/bluetooth/host/conn.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/logging/log_minimal.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/lib/net_buf/buf.c.obj CMakeFiles/testbinary.dir/__w/zephyr/zephyr/lib/net_buf/buf_simple.c.obj -o testbinary  mocks/libmocks.a  host_mocks/libhost_mocks.a && :
/usr/bin/ld: CMakeFiles/testbinary.dir/__w/zephyr/zephyr/subsys/bluetooth/host/conn.c.obj: in function `bt_conn_data_ready':
/__w/zephyr/zephyr/subsys/bluetooth/host/conn.c:865: undefined reference to `k_sched_lock'
/usr/bin/ld: /__w/zephyr/zephyr/subsys/bluetooth/host/conn.c:868: undefined reference to `k_sched_unlock'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants