Bluetooth: host: remove TX thread
We don't need the TX thread anymore. Generalizing the pull-based architecture (ie. `tx_processor`) to HCI commands makes it possible to run the whole TX path from the the system workqueue, or any workqueue really. There is an edge-case, where we call `bt_hci_cmd_send_sync()` from the syswq, stalling the system. The proposed mitigation is to attempt to drain the command queue from within `bt_hci_cmd_send_sync()`. My spidey sense tingles however, and it would be better to just remove the capability of calling this fn from the syswq. But doing this requires refactoring a bunch of synchronous procedures in the stack (e.g. stack init, connection establishment, address setting etc), dragging in more work. I will do it, but in a later patch. Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
This commit is contained in:
Jonathan Rico
Alberto Escolar
parent
48d1cffb4d
commit
28be8909a6
6 changed files with 113 additions and 100 deletions
|
|
@ -51,9 +51,6 @@ LOG_MODULE_REGISTER(bt_conn);
|
|||
|
||||
K_FIFO_DEFINE(free_tx);
|
||||
|
||||
void tx_processor(struct k_work *item);
|
||||
K_WORK_DELAYABLE_DEFINE(tx_work, tx_processor);
|
||||
|
||||
static void tx_free(struct bt_conn_tx *tx);
|
||||
|
||||
static void conn_tx_destroy(struct bt_conn *conn, struct bt_conn_tx *tx)
|
||||
|
|
@ -794,12 +791,6 @@ static bool should_stop_tx(struct bt_conn *conn)
|
|||
return true;
|
||||
}
|
||||
|
||||
void bt_tx_irq_raise(void)
|
||||
{
|
||||
LOG_DBG("");
|
||||
k_work_reschedule(&tx_work, K_NO_WAIT);
|
||||
}
|
||||
|
||||
void bt_conn_data_ready(struct bt_conn *conn)
|
||||
{
|
||||
LOG_DBG("DR");
|
||||
|
|
@ -922,7 +913,20 @@ static void destroy_and_callback(struct bt_conn *conn,
|
|||
}
|
||||
}
|
||||
|
||||
void tx_processor(struct k_work *item)
|
||||
static volatile bool _suspend_tx;
|
||||
|
||||
#if defined(CONFIG_BT_TESTING)
|
||||
void bt_conn_suspend_tx(bool suspend)
|
||||
{
|
||||
_suspend_tx = suspend;
|
||||
|
||||
LOG_DBG("%sing all data TX", suspend ? "suspend" : "resum");
|
||||
|
||||
bt_tx_irq_raise();
|
||||
}
|
||||
#endif /* CONFIG_BT_TESTING */
|
||||
|
||||
void bt_conn_tx_processor(void)
|
||||
{
|
||||
LOG_DBG("start");
|
||||
struct bt_conn *conn;
|
||||
|
|
@ -935,6 +939,10 @@ void tx_processor(struct k_work *item)
|
|||
return;
|
||||
}
|
||||
|
||||
if (IS_ENABLED(CONFIG_BT_TESTING) && _suspend_tx) {
|
||||
return;
|
||||
}
|
||||
|
||||
conn = get_conn_ready();
|
||||
|
||||
if (!conn) {
|
||||
|
|
@ -1009,7 +1017,7 @@ void tx_processor(struct k_work *item)
|
|||
/* Always kick the TX work. It will self-suspend if it doesn't get
|
||||
* resources or there is nothing left to send.
|
||||
*/
|
||||
k_work_reschedule(&tx_work, K_NO_WAIT);
|
||||
bt_tx_irq_raise();
|
||||
}
|
||||
|
||||
static void process_unack_tx(struct bt_conn *conn)
|
||||
|
|
|
|||
|
|
@ -541,8 +541,7 @@ void bt_conn_cleanup_all(void);
|
|||
/* Selects based on connection type right semaphore for ACL packets */
|
||||
struct k_sem *bt_conn_get_pkts(struct bt_conn *conn);
|
||||
|
||||
/* k_poll related helpers for the TX thread */
|
||||
int bt_conn_prepare_events(struct k_poll_event events[]);
|
||||
void bt_conn_tx_processor(void);
|
||||
|
||||
/* To be called by upper layers when they want to send something.
|
||||
* Functions just like an IRQ.
|
||||
|
|
|
|||
|
|
@ -71,6 +71,8 @@ LOG_MODULE_REGISTER(bt_hci_core);
|
|||
#define BT_HCI_BUS BT_DT_HCI_BUS_GET(BT_HCI_DEV)
|
||||
#define BT_HCI_NAME BT_DT_HCI_NAME_GET(BT_HCI_DEV)
|
||||
|
||||
void bt_tx_irq_raise(void);
|
||||
|
||||
#define HCI_CMD_TIMEOUT K_SECONDS(10)
|
||||
|
||||
/* Stacks for the threads */
|
||||
|
|
@ -80,8 +82,6 @@ static K_WORK_DEFINE(rx_work, rx_work_handler);
|
|||
static struct k_work_q bt_workq;
|
||||
static K_KERNEL_STACK_DEFINE(rx_thread_stack, CONFIG_BT_RX_STACK_SIZE);
|
||||
#endif /* CONFIG_BT_RECV_WORKQ_BT */
|
||||
static struct k_thread tx_thread_data;
|
||||
static K_KERNEL_STACK_DEFINE(tx_thread_stack, CONFIG_BT_HCI_TX_STACK_SIZE);
|
||||
|
||||
static void init_work(struct k_work *work);
|
||||
|
||||
|
|
@ -337,10 +337,12 @@ int bt_hci_cmd_send(uint16_t opcode, struct net_buf *buf)
|
|||
}
|
||||
|
||||
net_buf_put(&bt_dev.cmd_tx_queue, buf);
|
||||
bt_tx_irq_raise();
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
static bool process_pending_cmd(k_timeout_t timeout);
|
||||
int bt_hci_cmd_send_sync(uint16_t opcode, struct net_buf *buf,
|
||||
struct net_buf **rsp)
|
||||
{
|
||||
|
|
@ -357,21 +359,47 @@ int bt_hci_cmd_send_sync(uint16_t opcode, struct net_buf *buf,
|
|||
|
||||
LOG_DBG("buf %p opcode 0x%04x len %u", buf, opcode, buf->len);
|
||||
|
||||
/* This local sem is just for suspending the current thread until the
|
||||
* command is processed by the LL. It is given (and we are awaken) by
|
||||
* the cmd_complete/status handlers.
|
||||
*/
|
||||
k_sem_init(&sync_sem, 0, 1);
|
||||
cmd(buf)->sync = &sync_sem;
|
||||
|
||||
net_buf_put(&bt_dev.cmd_tx_queue, net_buf_ref(buf));
|
||||
bt_tx_irq_raise();
|
||||
|
||||
/* Wait for a response from the Bluetooth Controller.
|
||||
* The Controller may fail to respond if:
|
||||
* - It was never programmed or connected.
|
||||
* - There was a fatal error.
|
||||
*
|
||||
* See the `BT_HCI_OP_` macros in hci_types.h or
|
||||
* Core_v5.4, Vol 4, Part E, Section 5.4.1 and Section 7
|
||||
* to map the opcode to the HCI command documentation.
|
||||
* Example: 0x0c03 represents HCI_Reset command.
|
||||
/* TODO: disallow sending sync commands from syswq altogether */
|
||||
|
||||
/* Since the commands are now processed in the syswq, we cannot suspend
|
||||
* and wait. We have to send the command from the current context.
|
||||
*/
|
||||
if (k_current_get() == &k_sys_work_q.thread) {
|
||||
/* drain the command queue until we get to send the command of interest. */
|
||||
struct net_buf *cmd = NULL;
|
||||
|
||||
do {
|
||||
cmd = k_fifo_peek_head(&bt_dev.cmd_tx_queue);
|
||||
LOG_DBG("process cmd %p want %p", cmd, buf);
|
||||
|
||||
/* Wait for a response from the Bluetooth Controller.
|
||||
* The Controller may fail to respond if:
|
||||
* - It was never programmed or connected.
|
||||
* - There was a fatal error.
|
||||
*
|
||||
* See the `BT_HCI_OP_` macros in hci_types.h or
|
||||
* Core_v5.4, Vol 4, Part E, Section 5.4.1 and Section 7
|
||||
* to map the opcode to the HCI command documentation.
|
||||
* Example: 0x0c03 represents HCI_Reset command.
|
||||
*/
|
||||
bool success = process_pending_cmd(HCI_CMD_TIMEOUT);
|
||||
|
||||
BT_ASSERT_MSG(success, "command opcode 0x%04x timeout", opcode);
|
||||
(void)success; /* cmon zephyr fix your assert macros */
|
||||
} while (buf != cmd);
|
||||
}
|
||||
|
||||
/* Now that we have sent the command, suspend until the LL replies */
|
||||
err = k_sem_take(&sync_sem, HCI_CMD_TIMEOUT);
|
||||
BT_ASSERT_MSG(err == 0,
|
||||
"Controller unresponsive, command opcode 0x%04x timeout with err %d",
|
||||
|
|
@ -2425,6 +2453,7 @@ static void hci_cmd_done(uint16_t opcode, uint8_t status, struct net_buf *evt_bu
|
|||
|
||||
/* If the command was synchronous wake up bt_hci_cmd_send_sync() */
|
||||
if (cmd(buf)->sync) {
|
||||
LOG_DBG("sync cmd released");
|
||||
cmd(buf)->status = status;
|
||||
k_sem_give(cmd(buf)->sync);
|
||||
}
|
||||
|
|
@ -2465,6 +2494,7 @@ static void hci_cmd_complete(struct net_buf *buf)
|
|||
/* Allow next command to be sent */
|
||||
if (ncmd) {
|
||||
k_sem_give(&bt_dev.ncmd_sem);
|
||||
bt_tx_irq_raise();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -2485,6 +2515,7 @@ static void hci_cmd_status(struct net_buf *buf)
|
|||
/* Allow next command to be sent */
|
||||
if (ncmd) {
|
||||
k_sem_give(&bt_dev.ncmd_sem);
|
||||
bt_tx_irq_raise();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -2896,20 +2927,16 @@ static void hci_event(struct net_buf *buf)
|
|||
net_buf_unref(buf);
|
||||
}
|
||||
|
||||
static void send_cmd(void)
|
||||
static void hci_core_send_cmd(void)
|
||||
{
|
||||
struct net_buf *buf;
|
||||
int err;
|
||||
|
||||
/* Get next command */
|
||||
LOG_DBG("calling net_buf_get");
|
||||
LOG_DBG("fetch cmd");
|
||||
buf = net_buf_get(&bt_dev.cmd_tx_queue, K_NO_WAIT);
|
||||
BT_ASSERT(buf);
|
||||
|
||||
/* Wait until ncmd > 0 */
|
||||
LOG_DBG("calling sem_take_wait");
|
||||
k_sem_take(&bt_dev.ncmd_sem, K_FOREVER);
|
||||
|
||||
/* Clear out any existing sent command */
|
||||
if (bt_dev.sent_cmd) {
|
||||
LOG_ERR("Uncleared pending sent_cmd");
|
||||
|
|
@ -2927,27 +2954,7 @@ static void send_cmd(void)
|
|||
k_sem_give(&bt_dev.ncmd_sem);
|
||||
hci_cmd_done(cmd(buf)->opcode, BT_HCI_ERR_UNSPECIFIED, buf);
|
||||
net_buf_unref(buf);
|
||||
}
|
||||
}
|
||||
|
||||
static void process_events(struct k_poll_event *ev, int count)
|
||||
{
|
||||
LOG_DBG("count %d", count);
|
||||
|
||||
for (; count; ev++, count--) {
|
||||
LOG_DBG("ev->state %u", ev->state);
|
||||
|
||||
switch (ev->state) {
|
||||
case K_POLL_STATE_FIFO_DATA_AVAILABLE:
|
||||
if (ev->tag == BT_EVENT_CMD_TX) {
|
||||
send_cmd();
|
||||
}
|
||||
break;
|
||||
default:
|
||||
LOG_WRN("Unexpected k_poll event state %u", ev->state);
|
||||
__ASSERT_NO_MSG(0);
|
||||
break;
|
||||
}
|
||||
bt_tx_irq_raise();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -2969,38 +2976,6 @@ static void process_events(struct k_poll_event *ev, int count)
|
|||
#endif /* CONFIG_BT_ISO */
|
||||
#endif /* CONFIG_BT_CONN */
|
||||
|
||||
static void hci_tx_thread(void *p1, void *p2, void *p3)
|
||||
{
|
||||
static struct k_poll_event events[EV_COUNT] = {
|
||||
K_POLL_EVENT_STATIC_INITIALIZER(K_POLL_TYPE_FIFO_DATA_AVAILABLE,
|
||||
K_POLL_MODE_NOTIFY_ONLY,
|
||||
&bt_dev.cmd_tx_queue,
|
||||
BT_EVENT_CMD_TX),
|
||||
};
|
||||
|
||||
LOG_DBG("Started");
|
||||
|
||||
while (1) {
|
||||
int ev_count, err;
|
||||
|
||||
events[0].state = K_POLL_STATE_NOT_READY;
|
||||
ev_count = 1;
|
||||
|
||||
LOG_DBG("Calling k_poll with %d events", ev_count);
|
||||
|
||||
err = k_poll(events, ev_count, K_FOREVER);
|
||||
BT_ASSERT(err == 0);
|
||||
|
||||
process_events(events, ev_count);
|
||||
|
||||
/* Make sure we don't hog the CPU if there's all the time
|
||||
* some ready events.
|
||||
*/
|
||||
k_yield();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
static void read_local_ver_complete(struct net_buf *buf)
|
||||
{
|
||||
struct bt_hci_rp_read_local_version_info *rp = (void *)buf->data;
|
||||
|
|
@ -4210,7 +4185,8 @@ static void rx_work_handler(struct k_work *work)
|
|||
#if defined(CONFIG_BT_TESTING)
|
||||
k_tid_t bt_testing_tx_tid_get(void)
|
||||
{
|
||||
return &tx_thread_data;
|
||||
/* We now TX everything from the syswq */
|
||||
return &k_sys_work_q.thread;
|
||||
}
|
||||
#endif
|
||||
|
||||
|
|
@ -4262,13 +4238,6 @@ int bt_enable(bt_ready_cb_t cb)
|
|||
k_sem_init(&bt_dev.ncmd_sem, 0, 1);
|
||||
}
|
||||
k_fifo_init(&bt_dev.cmd_tx_queue);
|
||||
/* TX thread */
|
||||
k_thread_create(&tx_thread_data, tx_thread_stack,
|
||||
K_KERNEL_STACK_SIZEOF(tx_thread_stack),
|
||||
hci_tx_thread, NULL, NULL, NULL,
|
||||
K_PRIO_COOP(CONFIG_BT_HCI_TX_PRIO),
|
||||
0, K_NO_WAIT);
|
||||
k_thread_name_set(&tx_thread_data, "BT TX");
|
||||
|
||||
#if defined(CONFIG_BT_RECV_WORKQ_BT)
|
||||
/* RX thread */
|
||||
|
|
@ -4341,9 +4310,6 @@ int bt_disable(void)
|
|||
disconnected_handles_reset();
|
||||
#endif /* CONFIG_BT_CONN */
|
||||
|
||||
/* Abort TX thread */
|
||||
k_thread_abort(&tx_thread_data);
|
||||
|
||||
#if defined(CONFIG_BT_RECV_WORKQ_BT)
|
||||
/* Abort RX thread */
|
||||
k_thread_abort(&bt_workq.thread);
|
||||
|
|
@ -4644,3 +4610,41 @@ int bt_configure_data_path(uint8_t dir, uint8_t id, uint8_t vs_config_len,
|
|||
|
||||
return err;
|
||||
}
|
||||
|
||||
/* Return `true` if a command was processed/sent */
|
||||
static bool process_pending_cmd(k_timeout_t timeout)
|
||||
{
|
||||
if (!k_fifo_is_empty(&bt_dev.cmd_tx_queue)) {
|
||||
if (k_sem_take(&bt_dev.ncmd_sem, K_NO_WAIT) == 0) {
|
||||
hci_core_send_cmd();
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
static void tx_processor(struct k_work *item)
|
||||
{
|
||||
LOG_DBG("TX process start");
|
||||
if (process_pending_cmd(K_NO_WAIT)) {
|
||||
/* If we processed a command, let the scheduler run before
|
||||
* processing another command (or data).
|
||||
*/
|
||||
bt_tx_irq_raise();
|
||||
return;
|
||||
}
|
||||
|
||||
/* Hand over control to conn to process pending data */
|
||||
if (IS_ENABLED(CONFIG_BT_CONN_TX)) {
|
||||
bt_conn_tx_processor();
|
||||
}
|
||||
}
|
||||
|
||||
K_WORK_DELAYABLE_DEFINE(tx_work, tx_processor);
|
||||
|
||||
void bt_tx_irq_raise(void)
|
||||
{
|
||||
LOG_DBG("kick TX");
|
||||
k_work_reschedule(&tx_work, K_NO_WAIT);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -594,3 +594,5 @@ void bt_hci_le_df_cte_req_failed(struct net_buf *buf);
|
|||
|
||||
void bt_hci_le_per_adv_subevent_data_request(struct net_buf *buf);
|
||||
void bt_hci_le_per_adv_response_report(struct net_buf *buf);
|
||||
|
||||
void bt_tx_irq_raise(void);
|
||||
|
|
|
|||
|
|
@ -729,9 +729,9 @@ static bool iso_has_data(struct bt_conn *conn)
|
|||
{
|
||||
#if defined(CONFIG_BT_ISO_TX)
|
||||
return !k_fifo_is_empty(&conn->iso.txq);
|
||||
#else
|
||||
#else /* !CONFIG_BT_ISO_TX */
|
||||
return false;
|
||||
#endif
|
||||
#endif /* CONFIG_BT_ISO_TX */
|
||||
}
|
||||
|
||||
static struct net_buf *iso_data_pull(struct bt_conn *conn, size_t amount)
|
||||
|
|
|
|||
|
|
@ -32,8 +32,8 @@ static atomic_t nwrites;
|
|||
static atomic_t indications;
|
||||
static atomic_t notifications;
|
||||
|
||||
/* Defined in hci_core.c */
|
||||
extern k_tid_t bt_testing_tx_tid_get(void);
|
||||
/* Defined in conn.c */
|
||||
extern void bt_conn_suspend_tx(bool suspend);
|
||||
|
||||
static struct bt_conn *dconn;
|
||||
|
||||
|
|
@ -159,7 +159,7 @@ static ssize_t written_to(struct bt_conn *conn,
|
|||
if (atomic_get(&nwrites) == 0) {
|
||||
/* Suspend on the first write, which is an ATT Request */
|
||||
LOG_INF("suspending HCI TX thread");
|
||||
k_thread_suspend(bt_testing_tx_tid_get());
|
||||
bt_conn_suspend_tx(true);
|
||||
}
|
||||
|
||||
atomic_inc(&nwrites);
|
||||
|
|
@ -311,7 +311,7 @@ void test_procedure_0(void)
|
|||
WAIT_FOR_VAL(nwrites, 3);
|
||||
|
||||
/* Send RSP to LL */
|
||||
k_thread_resume(bt_testing_tx_tid_get());
|
||||
bt_conn_suspend_tx(false);
|
||||
|
||||
PASS("DUT done\n");
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue