From 958b700e592d529722ae834bc3fedd49d16f8f64 Mon Sep 17 00:00:00 2001 From: Jonathan Rico Date: Wed, 28 Aug 2024 15:12:10 +0200 Subject: [PATCH] Bluetooth: conn: move auto-init procedures to system workqueue `conn_auto_initiate()` starts a bunch of controller procedures (read: HCI commands) that are fired off right after connection establishment. Right now, it's called from the RX context, which is the same context where resources (cmd & acl buffers) are freed. This not ideal. But the procedures are all async, so it should be fine to schedule this function on the system workqueue, where we have less risk of deadlocks. Signed-off-by: Jonathan Rico --- subsys/bluetooth/host/conn.c | 103 ++++++++++++++++++++++++++ subsys/bluetooth/host/conn_internal.h | 8 ++ subsys/bluetooth/host/hci_core.c | 103 +++----------------------- subsys/bluetooth/host/hci_core.h | 6 ++ 4 files changed, 127 insertions(+), 93 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index e11375b196c7068..fcc2845ab4de0e9 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -302,6 +302,10 @@ static void tx_notify(struct bt_conn *conn) } #endif /* CONFIG_BT_CONN_TX */ +#if defined(CONFIG_BT_CONN) +static void auto_initiated_procedures(struct k_work *work); +#endif /* defined(CONFIG_BT_CONN) */ + struct bt_conn *bt_conn_new(struct bt_conn *conns, size_t size) { struct bt_conn *conn = NULL; @@ -322,6 +326,7 @@ struct bt_conn *bt_conn_new(struct bt_conn *conns, size_t size) #if defined(CONFIG_BT_CONN) k_work_init_delayable(&conn->deferred_work, deferred_work); + k_work_init(&conn->auto_initiated_procedures, auto_initiated_procedures); #endif /* CONFIG_BT_CONN */ #if defined(CONFIG_BT_CONN_TX) k_work_init(&conn->tx_complete_work, tx_complete_work); @@ -1153,6 +1158,104 @@ struct bt_conn *conn_lookup_handle(struct bt_conn *conns, size_t size, return NULL; } +#if defined(CONFIG_BT_CONN) +/* We don't want the application to get a PHY update callback upon connection + * establishment on 2M PHY. Therefore we must prevent issuing LE Set PHY + * in this scenario. + */ +static bool skip_auto_phy_update_on_conn_establishment(struct bt_conn *conn) +{ +#if defined(CONFIG_BT_USER_PHY_UPDATE) + if (IS_ENABLED(CONFIG_BT_AUTO_PHY_UPDATE) && + IS_ENABLED(CONFIG_BT_EXT_ADV) && + BT_DEV_FEAT_LE_EXT_ADV(bt_dev.le.features)) { + if (conn->le.phy.tx_phy == BT_HCI_LE_PHY_2M && + conn->le.phy.rx_phy == BT_HCI_LE_PHY_2M) { + return true; + } + } +#else + ARG_UNUSED(conn); +#endif /* defined(CONFIG_BT_USER_PHY_UPDATE) */ + + return false; +} + +static void auto_initiated_procedures(struct k_work *work) +{ + int err; + struct bt_conn *conn = CONTAINER_OF(work, struct bt_conn, auto_initiated_procedures); + + if (conn->state != BT_CONN_CONNECTED) { + /* It is possible that connection was disconnected directly from + * connected callback so we must check state before doing + * connection parameters update. + */ + goto exit; + } + + if (!atomic_test_bit(conn->flags, BT_CONN_AUTO_FEATURE_EXCH) && + ((conn->role == BT_HCI_ROLE_CENTRAL) || + BT_FEAT_LE_PER_INIT_FEAT_XCHG(bt_dev.le.features))) { + err = bt_hci_le_read_remote_features(conn); + if (err) { + LOG_ERR("Failed read remote features (%d)", err); + } + if (conn->state != BT_CONN_CONNECTED) { + goto exit; + } + } + + if (IS_ENABLED(CONFIG_BT_REMOTE_VERSION) && + !atomic_test_bit(conn->flags, BT_CONN_AUTO_VERSION_INFO)) { + err = bt_hci_read_remote_version(conn); + if (err) { + LOG_ERR("Failed read remote version (%d)", err); + } + if (conn->state != BT_CONN_CONNECTED) { + goto exit; + } + } + + if (IS_ENABLED(CONFIG_BT_AUTO_PHY_UPDATE) && + BT_FEAT_LE_PHY_2M(bt_dev.le.features) && + !skip_auto_phy_update_on_conn_establishment(conn)) { + err = bt_le_set_phy(conn, 0U, BT_HCI_LE_PHY_PREFER_2M, + BT_HCI_LE_PHY_PREFER_2M, + BT_HCI_LE_PHY_CODED_ANY); + if (err) { + LOG_ERR("Failed LE Set PHY (%d)", err); + } + if (conn->state != BT_CONN_CONNECTED) { + goto exit; + } + } + + if (IS_ENABLED(CONFIG_BT_AUTO_DATA_LEN_UPDATE) && + BT_FEAT_LE_DLE(bt_dev.le.features)) { + if (bt_drv_quirk_no_auto_dle()) { + uint16_t tx_octets, tx_time; + + err = bt_hci_le_read_max_data_len(&tx_octets, &tx_time); + if (!err) { + err = bt_le_set_data_len(conn, + tx_octets, tx_time); + if (err) { + LOG_ERR("Failed to set data len (%d)", err); + } + } + } else { + /* No need to auto-initiate DLE procedure. + * It is done by the controller. + */ + } + } + +exit: + bt_conn_unref(conn); +} +#endif /* defined(CONFIG_BT_CONN) */ + void bt_conn_set_state(struct bt_conn *conn, bt_conn_state_t state) { bt_conn_state_t old_state; diff --git a/subsys/bluetooth/host/conn_internal.h b/subsys/bluetooth/host/conn_internal.h index 1b9993e483665d3..61b5e14b1478c68 100644 --- a/subsys/bluetooth/host/conn_internal.h +++ b/subsys/bluetooth/host/conn_internal.h @@ -253,6 +253,14 @@ struct bt_conn { */ struct k_work_delayable deferred_work; + /* Executes procedures after a connection is established: + * - read remote features + * - read remote version + * - update PHY + * - update data length + */ + struct k_work auto_initiated_procedures; + union { struct bt_conn_le le; #if defined(CONFIG_BT_CLASSIC) diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index 427e0c18acd10e8..625994e21b7ede6 100644 --- a/subsys/bluetooth/host/hci_core.c +++ b/subsys/bluetooth/host/hci_core.c @@ -130,7 +130,7 @@ static bool drv_quirk_no_reset(void) return ((BT_DT_HCI_QUIRKS_GET(DT_CHOSEN(zephyr_bt_hci)) & BT_HCI_QUIRK_NO_RESET) != 0); } -__maybe_unused static bool drv_quirk_no_auto_dle(void) +bool bt_drv_quirk_no_auto_dle(void) { return ((BT_DT_HCI_QUIRKS_GET(DT_CHOSEN(zephyr_bt_hci)) & BT_HCI_QUIRK_NO_AUTO_DLE) != 0); } @@ -140,7 +140,7 @@ static bool drv_quirk_no_reset(void) return ((bt_dev.drv->quirks & BT_QUIRK_NO_RESET) != 0); } -__maybe_unused static bool drv_quirk_no_auto_dle(void) +bool bt_drv_quirk_no_auto_dle(void) { return ((bt_dev.drv->quirks & BT_QUIRK_NO_AUTO_DLE) != 0); } @@ -493,7 +493,7 @@ int bt_hci_le_rand(void *buffer, size_t len) return 0; } -static int hci_le_read_max_data_len(uint16_t *tx_octets, uint16_t *tx_time) +int bt_hci_le_read_max_data_len(uint16_t *tx_octets, uint16_t *tx_time) { struct bt_hci_rp_le_read_max_data_len *rp; struct net_buf *rsp; @@ -1022,7 +1022,7 @@ static void hci_disconn_complete(struct net_buf *buf) bt_conn_unref(conn); } -static int hci_le_read_remote_features(struct bt_conn *conn) +int bt_hci_le_read_remote_features(struct bt_conn *conn) { struct bt_hci_cp_le_read_remote_features *cp; struct net_buf *buf; @@ -1038,7 +1038,7 @@ static int hci_le_read_remote_features(struct bt_conn *conn) return bt_hci_cmd_send_sync(BT_HCI_OP_LE_READ_REMOTE_FEATURES, buf, NULL); } -static int hci_read_remote_version(struct bt_conn *conn) +int bt_hci_read_remote_version(struct bt_conn *conn) { struct bt_hci_cp_read_remote_version_info *cp; struct net_buf *buf; @@ -1172,89 +1172,6 @@ static struct bt_conn *find_pending_connect(uint8_t role, bt_addr_le_t *peer_add return NULL; } -/* We don't want the application to get a PHY update callback upon connection - * establishment on 2M PHY. Therefore we must prevent issuing LE Set PHY - * in this scenario. - */ -static bool skip_auto_phy_update_on_conn_establishment(struct bt_conn *conn) -{ -#if defined(CONFIG_BT_USER_PHY_UPDATE) - if (IS_ENABLED(CONFIG_BT_AUTO_PHY_UPDATE) && - IS_ENABLED(CONFIG_BT_EXT_ADV) && - BT_DEV_FEAT_LE_EXT_ADV(bt_dev.le.features)) { - if (conn->le.phy.tx_phy == BT_HCI_LE_PHY_2M && - conn->le.phy.rx_phy == BT_HCI_LE_PHY_2M) { - return true; - } - } -#else - ARG_UNUSED(conn); -#endif /* defined(CONFIG_BT_USER_PHY_UPDATE) */ - - return false; -} - -static void conn_auto_initiate(struct bt_conn *conn) -{ - int err; - - if (conn->state != BT_CONN_CONNECTED) { - /* It is possible that connection was disconnected directly from - * connected callback so we must check state before doing - * connection parameters update. - */ - return; - } - - if (!atomic_test_bit(conn->flags, BT_CONN_AUTO_FEATURE_EXCH) && - ((conn->role == BT_HCI_ROLE_CENTRAL) || - BT_FEAT_LE_PER_INIT_FEAT_XCHG(bt_dev.le.features))) { - err = hci_le_read_remote_features(conn); - if (err) { - LOG_ERR("Failed read remote features (%d)", err); - } - } - - if (IS_ENABLED(CONFIG_BT_REMOTE_VERSION) && - !atomic_test_bit(conn->flags, BT_CONN_AUTO_VERSION_INFO)) { - err = hci_read_remote_version(conn); - if (err) { - LOG_ERR("Failed read remote version (%d)", err); - } - } - - if (IS_ENABLED(CONFIG_BT_AUTO_PHY_UPDATE) && - BT_FEAT_LE_PHY_2M(bt_dev.le.features) && - !skip_auto_phy_update_on_conn_establishment(conn)) { - err = bt_le_set_phy(conn, 0U, BT_HCI_LE_PHY_PREFER_2M, - BT_HCI_LE_PHY_PREFER_2M, - BT_HCI_LE_PHY_CODED_ANY); - if (err) { - LOG_ERR("Failed LE Set PHY (%d)", err); - } - } - - if (IS_ENABLED(CONFIG_BT_AUTO_DATA_LEN_UPDATE) && - BT_FEAT_LE_DLE(bt_dev.le.features)) { - if (drv_quirk_no_auto_dle()) { - uint16_t tx_octets, tx_time; - - err = hci_le_read_max_data_len(&tx_octets, &tx_time); - if (!err) { - err = bt_le_set_data_len(conn, - tx_octets, tx_time); - if (err) { - LOG_ERR("Failed to set data len (%d)", err); - } - } - } else { - /* No need to auto-initiate DLE procedure. - * It is done by the controller. - */ - } - } -} - static void le_conn_complete_cancel(uint8_t err) { int ret; @@ -1561,8 +1478,8 @@ void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt) bt_conn_connected(conn); - /* Start auto-initiated procedures */ - conn_auto_initiate(conn); + /* Schedule auto-initiated procedures */ + k_work_submit(&bt_conn_ref(conn)->auto_initiated_procedures); bt_conn_unref(conn); @@ -1657,8 +1574,8 @@ void bt_hci_le_enh_conn_complete_sync(struct bt_hci_evt_le_enh_conn_complete_v2 */ bt_conn_unref(conn); - /* Start auto-initiated procedures */ - conn_auto_initiate(conn); + /* Schedule auto-initiated procedures */ + k_work_submit(&bt_conn_ref(conn)->auto_initiated_procedures); } #endif /* CONFIG_BT_PER_ADV_SYNC_RSP */ @@ -3629,7 +3546,7 @@ static int le_init(void) struct bt_hci_cp_le_write_default_data_len *cp; uint16_t tx_octets, tx_time; - err = hci_le_read_max_data_len(&tx_octets, &tx_time); + err = bt_hci_le_read_max_data_len(&tx_octets, &tx_time); if (err) { return err; } diff --git a/subsys/bluetooth/host/hci_core.h b/subsys/bluetooth/host/hci_core.h index 4f632dd9f313e76..b39f219024dd518 100644 --- a/subsys/bluetooth/host/hci_core.h +++ b/subsys/bluetooth/host/hci_core.h @@ -564,6 +564,12 @@ 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); +int bt_hci_read_remote_version(struct bt_conn *conn); +int bt_hci_le_read_remote_features(struct bt_conn *conn); +int bt_hci_le_read_max_data_len(uint16_t *tx_octets, uint16_t *tx_time); + +bool bt_drv_quirk_no_auto_dle(void); + void bt_tx_irq_raise(void); void bt_send_one_host_num_completed_packets(uint16_t handle); void bt_acl_set_ncp_sent(struct net_buf *packet, bool value);