From 01354c0045024a606ba8ae7cee1008a2ed067013 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 | 124 ++++++++++++++++++++++++++ subsys/bluetooth/host/conn_internal.h | 1 + subsys/bluetooth/host/hci_core.c | 102 ++------------------- subsys/bluetooth/host/hci_core.h | 6 ++ 4 files changed, 137 insertions(+), 96 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 3ffbc2c38cc3a7..5d83db752ff54f 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -1660,9 +1660,133 @@ int bt_conn_disconnect(struct bt_conn *conn, uint8_t reason) /* Group Connected BT_CONN only in this */ #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 perform_auto_initiated_procedures(struct bt_conn *conn, void *unused) +{ + int err; + + ARG_UNUSED(unused); + + LOG_DBG("[%p] Running auto-initiated procedures", conn); + + 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_and_set_bit(conn->flags, BT_CONN_AUTO_INIT_PROCEDURES_DONE)) { + /* We have already run the auto-initiated procedures */ + return; + } + + if (!atomic_test_bit(conn->flags, BT_CONN_LE_FEATURES_EXCHANGED) && + ((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) { + return; + } + } + + 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) { + return; + } + } + + 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) { + return; + } + } + + 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. + */ + } + } + + LOG_DBG("[%p] Successfully ran auto-initiated procedures", conn); +} + +/* Executes procedures after a connection is established: + * - read remote features + * - read remote version + * - update PHY + * - update data length + */ +static void auto_initiated_procedures(struct k_work *unused) +{ + ARG_UNUSED(unused); + + bt_conn_foreach(BT_CONN_TYPE_LE, perform_auto_initiated_procedures, NULL); +} + +static K_WORK_DEFINE(procedures_on_connect, auto_initiated_procedures); + +static void schedule_auto_initiated_procedures(struct bt_conn *conn) +{ + LOG_DBG("[%p] Scheduling auto-init procedures", conn); + k_work_submit(&procedures_on_connect); +} void bt_conn_connected(struct bt_conn *conn) { + schedule_auto_initiated_procedures(conn); bt_l2cap_connected(conn); notify_connected(conn); } diff --git a/subsys/bluetooth/host/conn_internal.h b/subsys/bluetooth/host/conn_internal.h index d890368ee4e307..58228588d0c60e 100644 --- a/subsys/bluetooth/host/conn_internal.h +++ b/subsys/bluetooth/host/conn_internal.h @@ -62,6 +62,7 @@ enum { BT_CONN_BR_NOBOND, /* SSP no bond pairing tracker */ BT_CONN_BR_PAIRING_INITIATOR, /* local host starts authentication */ BT_CONN_CLEANUP, /* Disconnected, pending cleanup */ + BT_CONN_AUTO_INIT_PROCEDURES_DONE, /* Auto-initiated procedures have run */ BT_CONN_PERIPHERAL_PARAM_UPDATE, /* If periph param update timer fired */ BT_CONN_PERIPHERAL_PARAM_AUTO_UPDATE, /* If periph param auto update on timer fired */ BT_CONN_PERIPHERAL_PARAM_SET, /* If periph param were set from app */ diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index cfd44929fb2ce1..6a6ba4c2bfe6c1 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; @@ -1039,7 +1039,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; @@ -1173,89 +1173,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_LE_FEATURES_EXCHANGED) && - ((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,10 +1478,6 @@ 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); - bt_conn_unref(conn); if (IS_ENABLED(CONFIG_BT_CENTRAL) && conn->role == BT_HCI_ROLE_CENTRAL) { @@ -1657,9 +1570,6 @@ void bt_hci_le_enh_conn_complete_sync(struct bt_hci_evt_le_enh_conn_complete_v2 * for peripheral connections, we need to release this reference here. */ bt_conn_unref(conn); - - /* Start auto-initiated procedures */ - conn_auto_initiate(conn); } #endif /* CONFIG_BT_PER_ADV_SYNC_RSP */ @@ -3630,7 +3540,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 4f632dd9f313e7..b39f219024dd51 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);