From 1f53b460e0bddb134c170ba0853532aae6e4d72d Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Tue, 9 Feb 2021 20:04:35 +0100 Subject: [PATCH 1/9] Bluetooth: shell: Fix gatt write command not cleanup up on error Fix gatt write command returned "write in progress" when either hex2bin or bt_gatt_write returned an error. The write_params.func should not be set if the write command was not successful. Signed-off-by: Joakim Andersson --- subsys/bluetooth/shell/gatt.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/subsys/bluetooth/shell/gatt.c b/subsys/bluetooth/shell/gatt.c index 2a1cec004d1d5..3942004633ede 100644 --- a/subsys/bluetooth/shell/gatt.c +++ b/subsys/bluetooth/shell/gatt.c @@ -377,21 +377,21 @@ static int cmd_write(const struct shell *shell, size_t argc, char *argv[]) handle = strtoul(argv[1], NULL, 16); offset = strtoul(argv[2], NULL, 16); - write_params.data = gatt_write_buf; - write_params.handle = handle; - write_params.offset = offset; - write_params.func = write_func; - write_params.length = hex2bin(argv[3], strlen(argv[3]), gatt_write_buf, sizeof(gatt_write_buf)); - if (write_params.length == 0) { shell_error(shell, "No data set"); return -ENOEXEC; } + write_params.data = gatt_write_buf; + write_params.handle = handle; + write_params.offset = offset; + write_params.func = write_func; + err = bt_gatt_write(default_conn, &write_params); if (err) { + write_params.func = NULL; shell_error(shell, "Write failed (err %d)", err); } else { shell_print(shell, "Write pending"); From c2b562bdc87eaa27ce3397744233e96560c31acd Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Wed, 16 Dec 2020 14:31:41 +0100 Subject: [PATCH 2/9] Bluetooth: host: Fix update keys when using debug public key check Fix the update keys check allowing to overwrite the keys when using debug keys. Instead the check disallowed overwriting keys made using debug keys. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/smp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/subsys/bluetooth/host/smp.c b/subsys/bluetooth/host/smp.c index caad469b9b08b..5c17fa8fcea87 100644 --- a/subsys/bluetooth/host/smp.c +++ b/subsys/bluetooth/host/smp.c @@ -799,10 +799,10 @@ static bool update_debug_keys_check(struct bt_smp *smp) } if (conn->le.keys->flags & BT_KEYS_DEBUG) { - return false; + return true; } - return true; + return false; } #if defined(CONFIG_BT_PRIVACY) || defined(CONFIG_BT_SIGNING) || \ From 66dc90173d5333273c801ad149d7b9d5d92fe2bb Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Fri, 8 Jan 2021 10:27:37 +0100 Subject: [PATCH 3/9] Bluetooth: host: Handle multiple step security elevation When ATT resends an ATT request it is sent as a "response" instead of as a request. This causes the ATT request buffer to be released and the ATT request cannot be resent one more time. This causes a problem when the ATT request requires authentication but the elevation of security is not enforcing MITM protection. In this case the ATT will first require security level 2 and then resend the request once this has been reached. This will lead to a new ATT error response and ATT will require security level L3. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/att.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c index 216788b0cd7a9..afb713b0f2171 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -119,6 +119,7 @@ static void att_req_destroy(struct bt_att_req *req) if (req->buf) { net_buf_unref(req->buf); + req->buf = NULL; } if (req->destroy) { @@ -2074,14 +2075,9 @@ static uint8_t att_error_rsp(struct bt_att_chan *chan, struct net_buf *buf) err = rsp->error; #if defined(CONFIG_BT_SMP) - if (chan->req->retrying) { - goto done; - } - - /* Check if security needs to be changed */ + /* Check if error can be handled by elevating security. */ if (!att_change_security(chan->chan.chan.conn, err)) { chan->req->retrying = true; - /* Wait security_changed: TODO: Handle fail case */ return 0; } #endif /* CONFIG_BT_SMP */ @@ -2725,10 +2721,14 @@ static void bt_att_encrypt_change(struct bt_l2cap_chan *chan, BT_DBG("Retrying"); /* Resend buffer */ - bt_att_chan_send_rsp(att_chan, att_chan->req->buf, - chan_cb(att_chan->req->buf)); - att_chan->req->buf = NULL; + /* Since packets are created in ATT and released in L2CAP we need to + * take a new reference to "create" the packet in ATT again. + */ + if (chan_send(att_chan, net_buf_ref(att_chan->req->buf), NULL)) { + net_buf_unref(att_chan->req->buf); + att_handle_rsp(att_chan, NULL, 0, BT_ATT_ERR_AUTHENTICATION); + } } #endif /* CONFIG_BT_SMP */ From dc2f26affc179af074d1bbbca3d1aa3d54101e16 Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Fri, 8 Jan 2021 10:27:48 +0100 Subject: [PATCH 4/9] Bluetooth: host: Allow requesting new security when security changed Allow to request a higher security level during the key distribution phase. This is required by ATT and L2CAP since they only react to the encrypt change event where they resend the current request. The current request might require a higher security level still and might have to request a higher security level before the pairing procedure has been finished. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/smp.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/subsys/bluetooth/host/smp.c b/subsys/bluetooth/host/smp.c index 5c17fa8fcea87..3bf0e7914dbac 100644 --- a/subsys/bluetooth/host/smp.c +++ b/subsys/bluetooth/host/smp.c @@ -1796,12 +1796,6 @@ static void smp_reset(struct bt_smp *smp) atomic_set(&smp->allowed_cmds, 0); atomic_set(smp->flags, 0); - if (conn->required_sec_level != conn->sec_level) { - /* TODO report error */ - /* reset required security level in case of error */ - conn->required_sec_level = conn->sec_level; - } - if (IS_ENABLED(CONFIG_BT_CENTRAL) && conn->role == BT_HCI_ROLE_MASTER) { atomic_set_bit(&smp->allowed_cmds, BT_SMP_CMD_SECURITY_REQUEST); @@ -1867,6 +1861,10 @@ static void smp_pairing_complete(struct bt_smp *smp, uint8_t status) } smp_reset(smp); + + if (conn->sec_level != conn->required_sec_level) { + bt_smp_start_security(conn); + } } static void smp_timeout(struct k_work *work) @@ -3059,6 +3057,13 @@ static int smp_send_pairing_req(struct bt_conn *conn) return -EIO; } + /* A higher security level is requested during the key distribution + * phase, once pairing is complete a new pairing procedure will start. + */ + if (atomic_test_bit(smp->flags, SMP_FLAG_KEYS_DISTR)) { + return 0; + } + /* pairing is in progress */ if (atomic_test_bit(smp->flags, SMP_FLAG_PAIRING)) { return -EBUSY; @@ -3920,6 +3925,13 @@ static uint8_t smp_security_request(struct bt_smp *smp, struct net_buf *buf) BT_DBG(""); + /* A higher security level is requested during the key distribution + * phase, once pairing is complete a new pairing procedure will start. + */ + if (atomic_test_bit(smp->flags, SMP_FLAG_KEYS_DISTR)) { + return 0; + } + if (atomic_test_bit(smp->flags, SMP_FLAG_PAIRING)) { /* We have already started pairing process */ return 0; From 2797902f0550f423de783ffaf338f4c8b1c3612f Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Thu, 17 Dec 2020 12:48:29 +0100 Subject: [PATCH 5/9] Bluetooth: host: Refactor smp handling of conn pointer Refactor SMP to have a conn pointer where this pointer is used multiple times. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/smp.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/subsys/bluetooth/host/smp.c b/subsys/bluetooth/host/smp.c index 3bf0e7914dbac..71ee163f5e14d 100644 --- a/subsys/bluetooth/host/smp.c +++ b/subsys/bluetooth/host/smp.c @@ -1812,6 +1812,8 @@ static void smp_reset(struct bt_smp *smp) */ static void smp_pairing_complete(struct bt_smp *smp, uint8_t status) { + struct bt_conn *conn = smp->chan.chan.conn; + BT_DBG("status 0x%x", status); if (!status) { @@ -1829,12 +1831,11 @@ static void smp_pairing_complete(struct bt_smp *smp, uint8_t status) bool bond_flag = atomic_test_bit(smp->flags, SMP_FLAG_BOND); if (bond_flag) { - bt_keys_store(smp->chan.chan.conn->le.keys); + bt_keys_store(conn->le.keys); } if (bt_auth && bt_auth->pairing_complete) { - bt_auth->pairing_complete(smp->chan.chan.conn, - bond_flag); + bt_auth->pairing_complete(conn, bond_flag); } } else { uint8_t auth_err = auth_err_get(status); @@ -1843,20 +1844,19 @@ static void smp_pairing_complete(struct bt_smp *smp, uint8_t status) * keys already existed before the pairing procedure or the * pairing failed during key distribution. */ - if (smp->chan.chan.conn->le.keys && - (!smp->chan.chan.conn->le.keys->enc_size || + if (conn->le.keys && + (!conn->le.keys->enc_size || atomic_test_bit(smp->flags, SMP_FLAG_KEYS_DISTR))) { - bt_keys_clear(smp->chan.chan.conn->le.keys); - smp->chan.chan.conn->le.keys = NULL; + bt_keys_clear(conn->le.keys); + conn->le.keys = NULL; } if (!atomic_test_bit(smp->flags, SMP_FLAG_KEYS_DISTR)) { - bt_conn_security_changed(smp->chan.chan.conn, status, - auth_err); + bt_conn_security_changed(conn, status, auth_err); } if (bt_auth && bt_auth->pairing_failed) { - bt_auth->pairing_failed(smp->chan.chan.conn, auth_err); + bt_auth->pairing_failed(conn, auth_err); } } @@ -3173,8 +3173,7 @@ static uint8_t smp_pairing_rsp(struct bt_smp *smp, struct net_buf *buf) if (IS_ENABLED(CONFIG_BT_SMP_APP_PAIRING_ACCEPT)) { uint8_t err; - err = smp_pairing_accept_query(smp->chan.chan.conn, - rsp); + err = smp_pairing_accept_query(conn, rsp); if (err) { return err; } @@ -3202,7 +3201,7 @@ static uint8_t smp_pairing_rsp(struct bt_smp *smp, struct net_buf *buf) if (IS_ENABLED(CONFIG_BT_SMP_APP_PAIRING_ACCEPT)) { uint8_t err; - err = smp_pairing_accept_query(smp->chan.chan.conn, rsp); + err = smp_pairing_accept_query(conn, rsp); if (err) { return err; } @@ -3212,7 +3211,7 @@ static uint8_t smp_pairing_rsp(struct bt_smp *smp, struct net_buf *buf) atomic_test_bit(smp->flags, SMP_FLAG_SEC_REQ) && bt_auth && bt_auth->pairing_confirm) { atomic_set_bit(smp->flags, SMP_FLAG_USER); - bt_auth->pairing_confirm(smp->chan.chan.conn); + bt_auth->pairing_confirm(conn); return 0; } From e680dc83e742af72a8d704259cd3addf8078906f Mon Sep 17 00:00:00 2001 From: Luiz Augusto von Dentz Date: Mon, 5 Oct 2020 11:41:06 -0700 Subject: [PATCH 6/9] Bluetooth: ATT: Remove BT_ATT_TX_MAX ATT channels do support queueing buffer so it no longer need to block waiting the tx_sem besides the buffer allocation already serves the same purpose as the application will not be able to have more requests than there are buffers available. Signed-off-by: Luiz Augusto von Dentz --- subsys/bluetooth/host/Kconfig.gatt | 10 ----- subsys/bluetooth/host/att.c | 37 ++----------------- .../edtt_ble_test_app/gatt_test_app/prj.conf | 1 - 3 files changed, 3 insertions(+), 45 deletions(-) diff --git a/subsys/bluetooth/host/Kconfig.gatt b/subsys/bluetooth/host/Kconfig.gatt index 407abb91f6e9b..22994ff69c4d9 100644 --- a/subsys/bluetooth/host/Kconfig.gatt +++ b/subsys/bluetooth/host/Kconfig.gatt @@ -24,16 +24,6 @@ config BT_ATT_PREPARE_COUNT Number of buffers available for ATT prepare write, setting this to 0 disables GATT long/reliable writes. -config BT_ATT_TX_MAX - int "Maximum number of queued outgoing ATT PDUs" - default BT_L2CAP_TX_BUF_COUNT - range 1 BT_L2CAP_TX_BUF_COUNT - help - Number of ATT PDUs that can be at a single moment queued for - transmission. If the application tries to send more than this - amount the calls will block until an existing queued PDU gets - sent. - config BT_EATT bool "Enhanced ATT Bearers support [EXPERIMENTAL]" depends on BT_L2CAP_ECRED diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c index afb713b0f2171..e78c42750e8ea 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -66,7 +66,7 @@ NET_BUF_POOL_DEFINE(prep_pool, CONFIG_BT_ATT_PREPARE_COUNT, BT_ATT_MTU, #endif /* CONFIG_BT_ATT_PREPARE_COUNT */ K_MEM_SLAB_DEFINE(req_slab, sizeof(struct bt_att_req), - CONFIG_BT_ATT_TX_MAX, __alignof__(struct bt_att_req)); + CONFIG_BT_L2CAP_TX_BUF_COUNT, __alignof__(struct bt_att_req)); enum { ATT_PENDING_RSP, @@ -88,7 +88,6 @@ struct bt_att_chan { struct bt_att_req *req; struct k_fifo tx_queue; struct k_delayed_work timeout_work; - struct k_sem tx_sem; void (*sent)(struct bt_att_chan *chan); sys_snode_t node; }; @@ -315,12 +314,7 @@ static void bt_att_sent(struct bt_l2cap_chan *ch) } /* Process global queue */ - err = process_queue(chan, &att->tx_queue); - if (!err) { - return; - } - - k_sem_give(&chan->tx_sem); + (void)process_queue(chan, &att->tx_queue); } static void chan_cfm_sent(struct bt_att_chan *chan) @@ -464,13 +458,6 @@ static int bt_att_chan_send(struct bt_att_chan *chan, struct net_buf *buf, BT_DBG("chan %p flags %u code 0x%02x", chan, atomic_get(chan->flags), hdr->code); - /* Don't use tx_sem if caller has set it own callback */ - if (!cb) { - if (k_sem_take(&chan->tx_sem, K_NO_WAIT) < 0) { - return -EAGAIN; - } - } - return chan_send(chan, buf, cb); } @@ -565,8 +552,6 @@ static uint8_t att_mtu_req(struct bt_att_chan *chan, struct net_buf *buf) static int bt_att_chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req) { - int err; - __ASSERT_NO_MSG(chan); __ASSERT_NO_MSG(req); __ASSERT_NO_MSG(req->func); @@ -574,16 +559,7 @@ static int bt_att_chan_req_send(struct bt_att_chan *chan, BT_DBG("req %p", req); - if (k_sem_take(&chan->tx_sem, K_NO_WAIT) < 0) { - return -EAGAIN; - } - - err = chan_req_send(chan, req); - if (err < 0) { - k_sem_give(&chan->tx_sem); - } - - return err; + return chan_req_send(chan, req); } static void att_process(struct bt_att *att) @@ -2567,17 +2543,11 @@ static void att_reset(struct bt_att *att) static void att_chan_detach(struct bt_att_chan *chan) { struct net_buf *buf; - int i; BT_DBG("chan %p", chan); sys_slist_find_and_remove(&chan->att->chans, &chan->node); - /* Ensure that any waiters are woken up */ - for (i = 0; i < CONFIG_BT_ATT_TX_MAX; i++) { - k_sem_give(&chan->tx_sem); - } - /* Release pending buffers */ while ((buf = net_buf_get(&chan->tx_queue, K_NO_WAIT))) { net_buf_unref(buf); @@ -2811,7 +2781,6 @@ static struct bt_att_chan *att_chan_new(struct bt_att *att, atomic_val_t flags) (void)memset(chan, 0, sizeof(*chan)); chan->chan.chan.ops = &ops; k_fifo_init(&chan->tx_queue); - k_sem_init(&chan->tx_sem, CONFIG_BT_ATT_TX_MAX, CONFIG_BT_ATT_TX_MAX); atomic_set(chan->flags, flags); chan->att = att; diff --git a/tests/bluetooth/bsim_bt/edtt_ble_test_app/gatt_test_app/prj.conf b/tests/bluetooth/bsim_bt/edtt_ble_test_app/gatt_test_app/prj.conf index e1b0e695538cd..3f501faa7bd12 100644 --- a/tests/bluetooth/bsim_bt/edtt_ble_test_app/gatt_test_app/prj.conf +++ b/tests/bluetooth/bsim_bt/edtt_ble_test_app/gatt_test_app/prj.conf @@ -7,7 +7,6 @@ CONFIG_BT_SIGNING=y CONFIG_BT_TINYCRYPT_ECC=y CONFIG_BT_PERIPHERAL=y CONFIG_BT_ATT_PREPARE_COUNT=2 -CONFIG_BT_ATT_TX_MAX=3 CONFIG_BT_PRIVACY=y CONFIG_BT_DEVICE_NAME="Test Database" CONFIG_BT_DEVICE_APPEARANCE=833 From 80ceaf10c548c3a99c2a28aa953ab315c6cac6ef Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Fri, 18 Dec 2020 21:13:40 +0100 Subject: [PATCH 7/9] Bluetooth: host: Fix indicate without user callback Fix indicate without func not working properly, when sent as a non-req by GATT this has two propblems: - The indicate would not be treated as a transaction, and back to back indicate would be sent without waiting for the confirm - The destroy callback would not be called on the indicate parameters since the indicate_rsp callback would not be called. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/gatt.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c index f062997afbb02..daeb96c73aec7 100644 --- a/subsys/bluetooth/host/gatt.c +++ b/subsys/bluetooth/host/gatt.c @@ -1900,8 +1900,11 @@ static void gatt_indicate_rsp(struct bt_conn *conn, uint8_t err, { struct bt_gatt_indicate_params *params = user_data; + if (params->func) { + params->func(conn, params, err); + } + params->_ref--; - params->func(conn, params, err); if (params->destroy && (params->_ref == 0)) { params->destroy(params); } @@ -1976,10 +1979,6 @@ static int gatt_indicate(struct bt_conn *conn, uint16_t handle, net_buf_add(buf, params->len); memcpy(ind->value, params->data, params->len); - if (!params->func) { - return gatt_send(conn, buf, NULL, NULL, NULL); - } - return gatt_send(conn, buf, gatt_indicate_rsp, params, NULL); } From ddf0703c4598eb07b7ca62bf34180ac2152dd00c Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Wed, 6 Jan 2021 10:30:58 +0100 Subject: [PATCH 8/9] Bluetooth: host: Remove unused ATT request destroy callback Remove the ATT request destroy callback which is never assigned by any of the ATT requests. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/att.c | 4 ---- subsys/bluetooth/host/att_internal.h | 2 -- subsys/bluetooth/host/gatt.c | 36 +++++++++++++--------------- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c index e78c42750e8ea..0bab75286cfa8 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -121,10 +121,6 @@ static void att_req_destroy(struct bt_att_req *req) req->buf = NULL; } - if (req->destroy) { - req->destroy(req); - } - bt_att_req_free(req); } diff --git a/subsys/bluetooth/host/att_internal.h b/subsys/bluetooth/host/att_internal.h index a1dab5c6fc44b..90c35516a790f 100644 --- a/subsys/bluetooth/host/att_internal.h +++ b/subsys/bluetooth/host/att_internal.h @@ -262,13 +262,11 @@ struct bt_att_signed_write_cmd { typedef void (*bt_att_func_t)(struct bt_conn *conn, uint8_t err, const void *pdu, uint16_t length, void *user_data); -typedef void (*bt_att_destroy_t)(void *user_data); /* ATT request context */ struct bt_att_req { sys_snode_t node; bt_att_func_t func; - bt_att_destroy_t destroy; struct net_buf_simple_state state; struct net_buf *buf; #if defined(CONFIG_BT_SMP) diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c index daeb96c73aec7..fb1683d9bb7b1 100644 --- a/subsys/bluetooth/host/gatt.c +++ b/subsys/bluetooth/host/gatt.c @@ -1911,8 +1911,7 @@ static void gatt_indicate_rsp(struct bt_conn *conn, uint8_t err, } static int gatt_send(struct bt_conn *conn, struct net_buf *buf, - bt_att_func_t func, void *params, - bt_att_destroy_t destroy) + bt_att_func_t func, void *params) { int err; @@ -1927,7 +1926,6 @@ static int gatt_send(struct bt_conn *conn, struct net_buf *buf, req->buf = buf; req->func = func; - req->destroy = destroy; req->user_data = params; err = bt_att_req_send(conn, req); @@ -1979,7 +1977,7 @@ static int gatt_indicate(struct bt_conn *conn, uint16_t handle, net_buf_add(buf, params->len); memcpy(ind->value, params->data, params->len); - return gatt_send(conn, buf, gatt_indicate_rsp, params, NULL); + return gatt_send(conn, buf, gatt_indicate_rsp, params); } static uint8_t notify_cb(const struct bt_gatt_attr *attr, uint16_t handle, @@ -2752,7 +2750,7 @@ int bt_gatt_exchange_mtu(struct bt_conn *conn, req = net_buf_add(buf, sizeof(*req)); req->mtu = sys_cpu_to_le16(mtu); - return gatt_send(conn, buf, gatt_mtu_rsp, params, NULL); + return gatt_send(conn, buf, gatt_mtu_rsp, params); } static void gatt_discover_next(struct bt_conn *conn, uint16_t last_handle, @@ -2882,7 +2880,7 @@ static int gatt_find_type(struct bt_conn *conn, return -EINVAL; } - return gatt_send(conn, buf, gatt_find_type_rsp, params, NULL); + return gatt_send(conn, buf, gatt_find_type_rsp, params); } static void read_included_uuid_cb(struct bt_conn *conn, uint8_t err, @@ -2947,7 +2945,7 @@ static int read_included_uuid(struct bt_conn *conn, BT_DBG("handle 0x%04x", params->_included.start_handle); - return gatt_send(conn, buf, read_included_uuid_cb, params, NULL); + return gatt_send(conn, buf, read_included_uuid_cb, params); } static uint16_t parse_include(struct bt_conn *conn, const void *pdu, @@ -3169,7 +3167,7 @@ static int gatt_read_type(struct bt_conn *conn, BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_read_type_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_type_rsp, params); } static uint16_t parse_service(struct bt_conn *conn, const void *pdu, @@ -3303,7 +3301,7 @@ static int gatt_read_group(struct bt_conn *conn, BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_read_group_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_group_rsp, params); } static void gatt_find_info_rsp(struct bt_conn *conn, uint8_t err, @@ -3437,7 +3435,7 @@ static int gatt_find_info(struct bt_conn *conn, BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_find_info_rsp, params, NULL); + return gatt_send(conn, buf, gatt_find_info_rsp, params); } int bt_gatt_discover(struct bt_conn *conn, @@ -3594,7 +3592,7 @@ static int gatt_read_blob(struct bt_conn *conn, BT_DBG("handle 0x%04x offset 0x%04x", params->single.handle, params->single.offset); - return gatt_send(conn, buf, gatt_read_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_rsp, params); } static int gatt_read_uuid(struct bt_conn *conn, @@ -3622,7 +3620,7 @@ static int gatt_read_uuid(struct bt_conn *conn, params->by_uuid.start_handle, params->by_uuid.end_handle, bt_uuid_str(params->by_uuid.uuid)); - return gatt_send(conn, buf, gatt_read_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_rsp, params); } #if defined(CONFIG_BT_GATT_READ_MULTIPLE) @@ -3660,7 +3658,7 @@ static int gatt_read_mult(struct bt_conn *conn, net_buf_add_le16(buf, params->handles[i]); } - return gatt_send(conn, buf, gatt_read_mult_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_mult_rsp, params); } #if defined(CONFIG_BT_EATT) @@ -3723,7 +3721,7 @@ static int gatt_read_mult_vl(struct bt_conn *conn, net_buf_add_le16(buf, params->handles[i]); } - return gatt_send(conn, buf, gatt_read_mult_vl_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_mult_vl_rsp, params); } #endif /* CONFIG_BT_EATT */ @@ -3779,7 +3777,7 @@ int bt_gatt_read(struct bt_conn *conn, struct bt_gatt_read_params *params) BT_DBG("handle 0x%04x", params->single.handle); - return gatt_send(conn, buf, gatt_read_rsp, params, NULL); + return gatt_send(conn, buf, gatt_read_rsp, params); } static void gatt_write_rsp(struct bt_conn *conn, uint8_t err, const void *pdu, @@ -3858,7 +3856,7 @@ static int gatt_exec_write(struct bt_conn *conn, BT_DBG(""); - return gatt_send(conn, buf, gatt_write_rsp, params, NULL); + return gatt_send(conn, buf, gatt_write_rsp, params); } static void gatt_prepare_write_rsp(struct bt_conn *conn, uint8_t err, @@ -3918,7 +3916,7 @@ static int gatt_prepare_write(struct bt_conn *conn, BT_DBG("handle 0x%04x offset %u len %u", params->handle, params->offset, params->length); - return gatt_send(conn, buf, gatt_prepare_write_rsp, params, NULL); + return gatt_send(conn, buf, gatt_prepare_write_rsp, params); } int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params) @@ -3959,7 +3957,7 @@ int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params) BT_DBG("handle 0x%04x length %u", params->handle, params->length); - return gatt_send(conn, buf, gatt_write_rsp, params, NULL); + return gatt_send(conn, buf, gatt_write_rsp, params); } static void gatt_write_ccc_rsp(struct bt_conn *conn, uint8_t err, @@ -4021,7 +4019,7 @@ static int gatt_write_ccc(struct bt_conn *conn, uint16_t handle, uint16_t value, atomic_set_bit(params->flags, BT_GATT_SUBSCRIBE_FLAG_WRITE_PENDING); - return gatt_send(conn, buf, func, params, NULL); + return gatt_send(conn, buf, func, params); } #if defined(CONFIG_BT_GATT_AUTO_DISCOVER_CCC) From 21131e2cf7db069797e242aa3d2be5ee594b47c9 Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Thu, 7 Jan 2021 17:19:34 +0100 Subject: [PATCH 9/9] Bluetooth: host: Release ATT request buffers once sent The ATT request buffers are held until the ATT response has been received. This means that the ATT request buffers are released by the RX thread, instead of the from the RX priority context of num_complete. This can cause a deadlock in the RX thread when we allocate buffers and all the available buffers are ATT requests, since the RX thread is the only thread that can release buffers. Release the ATT request buffers once they have been sent and instead handle ATT request resending by reconstructing the buffer from the GATT parameters. Also re-order the order of resource allocation by allocating the request context before the buffer. This ensures that we cannot allocate more buffers for ATT requests than there are ATT requests. Fixed a buf reference leak that could occur when the ATT request buffer has been allocated, but GATT returns an error before handing the responsebility of the buffer to ATT, for example when bt_att_req_alloc fails. This is fixed by moving the functionality of att_req_destroy to bt_att_req_free. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/att.c | 113 +++--- subsys/bluetooth/host/att_internal.h | 9 +- subsys/bluetooth/host/gatt.c | 530 ++++++++++++++++----------- 3 files changed, 382 insertions(+), 270 deletions(-) diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c index 0bab75286cfa8..cfef40f40bc55 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -112,18 +112,6 @@ K_MEM_SLAB_DEFINE(chan_slab, sizeof(struct bt_att_chan), __alignof__(struct bt_att_chan)); static struct bt_att_req cancel; -static void att_req_destroy(struct bt_att_req *req) -{ - BT_DBG("req %p", req); - - if (req->buf) { - net_buf_unref(req->buf); - req->buf = NULL; - } - - bt_att_req_free(req); -} - typedef void (*bt_att_chan_sent_t)(struct bt_att_chan *chan); static bt_att_chan_sent_t chan_cb(struct net_buf *buf); @@ -201,19 +189,14 @@ static int chan_send(struct bt_att_chan *chan, struct net_buf *buf, chan->sent = cb ? cb : chan_cb(buf); - /* Take a ref since bt_l2cap_send_cb takes ownership of the buffer */ err = bt_l2cap_send_cb(chan->att->conn, BT_L2CAP_CID_ATT, - net_buf_ref(buf), att_cb(chan->sent), - &chan->chan.chan); - if (!err) { - net_buf_unref(buf); - return 0; + buf, att_cb(chan->sent), + &chan->chan.chan); + if (err) { + net_buf_simple_restore(&buf->b, &state); } - net_buf_simple_restore(&buf->b, &state); - return err; - } static int process_queue(struct bt_att_chan *chan, struct k_fifo *queue) @@ -239,6 +222,7 @@ static int process_queue(struct bt_att_chan *chan, struct k_fifo *queue) /* Send requests without taking tx_sem */ static int chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req) { + struct net_buf *buf; int err; if (chan->chan.tx.mtu < net_buf_frags_len(req->buf)) { @@ -250,19 +234,14 @@ static int chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req) chan->req = req; - /* Save request state so it can be resent */ - net_buf_simple_save(&req->buf->b, &req->state); + /* Release since bt_l2cap_send_cb takes ownership of the buffer */ + buf = req->buf; + req->buf = NULL; - /* Keep a reference for resending the req in case the security - * needs to be changed. - */ - err = chan_send(chan, net_buf_ref(req->buf), NULL); + err = chan_send(chan, buf, NULL); if (err) { - /* Drop the extra reference if buffer could not be sent but - * don't reset the buffer as it will likelly be pushed back to - * request queue to be send later. - */ - net_buf_unref(req->buf); + /* We still have the ownership of the buffer */ + req->buf = buf; } return err; @@ -607,19 +586,13 @@ static uint8_t att_handle_rsp(struct bt_att_chan *chan, void *pdu, uint16_t len, goto process; } - /* Release original buffer */ - if (chan->req->buf) { - net_buf_unref(chan->req->buf); - chan->req->buf = NULL; - } - /* Reset func so it can be reused by the callback */ func = chan->req->func; chan->req->func = NULL; params = chan->req->user_data; /* free allocated request so its memory can be reused */ - att_req_destroy(chan->req); + bt_att_req_free(chan->req); chan->req = NULL; process: @@ -2040,11 +2013,6 @@ static uint8_t att_error_rsp(struct bt_att_chan *chan, struct net_buf *buf) goto done; } - if (chan->req->buf) { - /* Restore state to be resent */ - net_buf_simple_restore(&chan->req->buf->b, &chan->req->state); - } - err = rsp->error; #if defined(CONFIG_BT_SMP) /* Check if error can be handled by elevating security. */ @@ -2530,7 +2498,7 @@ static void att_reset(struct bt_att *att) req->user_data); } - att_req_destroy(req); + bt_att_req_free(req); } k_mem_slab_free(&att_slab, (void **)&att); @@ -2646,12 +2614,43 @@ static void bt_att_disconnected(struct bt_l2cap_chan *chan) } #if defined(CONFIG_BT_SMP) +static uint8_t att_req_retry(struct bt_att_chan *att_chan) +{ + struct bt_att_req *req = att_chan->req; + struct net_buf *buf; + + /* Resend buffer */ + if (!req->encode) { + /* This request does not support resending */ + return BT_ATT_ERR_AUTHENTICATION; + } + + + buf = bt_att_chan_create_pdu(att_chan, req->att_op, req->len); + if (!buf) { + return BT_ATT_ERR_UNLIKELY; + } + + if (req->encode(buf, req->len, req->user_data)) { + net_buf_unref(buf); + return BT_ATT_ERR_UNLIKELY; + } + + if (chan_send(att_chan, buf, NULL)) { + net_buf_unref(buf); + return BT_ATT_ERR_UNLIKELY; + } + + return BT_ATT_ERR_SUCCESS; +} + static void bt_att_encrypt_change(struct bt_l2cap_chan *chan, uint8_t hci_status) { struct bt_att_chan *att_chan = ATT_CHAN(chan); struct bt_l2cap_le_chan *ch = BT_L2CAP_LE_CHAN(chan); struct bt_conn *conn = ch->chan.conn; + uint8_t err; BT_DBG("chan %p conn %p handle %u sec_level 0x%02x status 0x%02x", ch, conn, conn->handle, conn->sec_level, hci_status); @@ -2686,14 +2685,10 @@ static void bt_att_encrypt_change(struct bt_l2cap_chan *chan, BT_DBG("Retrying"); - /* Resend buffer */ - - /* Since packets are created in ATT and released in L2CAP we need to - * take a new reference to "create" the packet in ATT again. - */ - if (chan_send(att_chan, net_buf_ref(att_chan->req->buf), NULL)) { - net_buf_unref(att_chan->req->buf); - att_handle_rsp(att_chan, NULL, 0, BT_ATT_ERR_AUTHENTICATION); + err = att_req_retry(att_chan); + if (err) { + BT_DBG("Retry failed (%d)", err); + att_handle_rsp(att_chan, NULL, 0, err); } } #endif /* CONFIG_BT_SMP */ @@ -2932,6 +2927,7 @@ struct bt_att_req *bt_att_req_alloc(k_timeout_t timeout) /* Reserve space for request */ if (k_mem_slab_alloc(&req_slab, (void **)&req, timeout)) { + BT_DBG("No space for req"); return NULL; } @@ -2946,6 +2942,11 @@ void bt_att_req_free(struct bt_att_req *req) { BT_DBG("req %p", req); + if (req->buf) { + net_buf_unref(req->buf); + req->buf = NULL; + } + k_mem_slab_free(&req_slab, (void **)&req); } @@ -3003,8 +3004,6 @@ int bt_att_req_send(struct bt_conn *conn, struct bt_att_req *req) att = att_get(conn); if (!att) { - net_buf_unref(req->buf); - req->buf = NULL; return -ENOTCONN; } @@ -3037,7 +3036,7 @@ static bool bt_att_chan_req_cancel(struct bt_att_chan *chan, chan->req = &cancel; - att_req_destroy(req); + bt_att_req_free(req); return true; } @@ -3068,5 +3067,5 @@ void bt_att_req_cancel(struct bt_conn *conn, struct bt_att_req *req) /* Remove request from the list */ sys_slist_find_and_remove(&att->reqs, &req->node); - att_req_destroy(req); + bt_att_req_free(req); } diff --git a/subsys/bluetooth/host/att_internal.h b/subsys/bluetooth/host/att_internal.h index 90c35516a790f..c44e6b7e67a7c 100644 --- a/subsys/bluetooth/host/att_internal.h +++ b/subsys/bluetooth/host/att_internal.h @@ -263,14 +263,19 @@ typedef void (*bt_att_func_t)(struct bt_conn *conn, uint8_t err, const void *pdu, uint16_t length, void *user_data); +typedef int (*bt_att_encode_t)(struct net_buf *buf, size_t len, + void *user_data); + /* ATT request context */ struct bt_att_req { sys_snode_t node; bt_att_func_t func; - struct net_buf_simple_state state; struct net_buf *buf; #if defined(CONFIG_BT_SMP) - bool retrying; + bt_att_encode_t encode; + uint8_t retrying : 1; + uint8_t att_op; + size_t len; #endif /* CONFIG_BT_SMP */ void *user_data; }; diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c index fb1683d9bb7b1..7886a8cc51958 100644 --- a/subsys/bluetooth/host/gatt.c +++ b/subsys/bluetooth/host/gatt.c @@ -1910,44 +1910,75 @@ static void gatt_indicate_rsp(struct bt_conn *conn, uint8_t err, } } -static int gatt_send(struct bt_conn *conn, struct net_buf *buf, - bt_att_func_t func, void *params) +static struct bt_att_req *gatt_req_alloc(bt_att_func_t func, void *params, + bt_att_encode_t encode, + uint8_t op, + size_t len) { + struct bt_att_req *req; + + /* Allocate new request */ + req = bt_att_req_alloc(BT_ATT_TIMEOUT); + if (!req) { + return NULL; + } + +#if defined(CONFIG_BT_SMP) + req->att_op = op; + req->len = len; + req->encode = encode; +#endif + req->func = func; + req->user_data = params; + + return req; +} + +#ifdef CONFIG_BT_GATT_CLIENT +static int gatt_req_send(struct bt_conn *conn, bt_att_func_t func, void *params, + bt_att_encode_t encode, uint8_t op, size_t len) + +{ + struct bt_att_req *req; + struct net_buf *buf; int err; - if (params) { - struct bt_att_req *req; + req = gatt_req_alloc(func, params, encode, op, len); + if (!req) { + return -ENOMEM; + } - /* Allocate new request */ - req = bt_att_req_alloc(BT_ATT_TIMEOUT); - if (!req) { - return -ENOMEM; - } + buf = bt_att_create_pdu(conn, op, len); + if (!buf) { + bt_att_req_free(req); + return -ENOMEM; + } - req->buf = buf; - req->func = func; - req->user_data = params; + req->buf = buf; - err = bt_att_req_send(conn, req); - if (err) { - bt_att_req_free(req); - } - } else { - err = bt_att_send(conn, buf, NULL, NULL); + err = encode(buf, len, params); + if (err) { + bt_att_req_free(req); + return err; } + err = bt_att_req_send(conn, req); if (err) { - BT_ERR("Error sending ATT PDU: %d", err); + bt_att_req_free(req); } return err; } +#endif static int gatt_indicate(struct bt_conn *conn, uint16_t handle, struct bt_gatt_indicate_params *params) { struct net_buf *buf; struct bt_att_indicate *ind; + struct bt_att_req *req; + size_t len; + int err; #if defined(CONFIG_BT_GATT_ENFORCE_CHANGE_UNAWARE) /* BLUETOOTH CORE SPECIFICATION Version 5.1 | Vol 3, Part G page 2350: @@ -1961,23 +1992,37 @@ static int gatt_indicate(struct bt_conn *conn, uint16_t handle, return -EAGAIN; } #endif + len = sizeof(*ind) + params->len; + + req = gatt_req_alloc(gatt_indicate_rsp, params, NULL, + BT_ATT_OP_INDICATE, len); + if (!req) { + return -ENOMEM; + } - buf = bt_att_create_pdu(conn, BT_ATT_OP_INDICATE, - sizeof(*ind) + params->len); + buf = bt_att_create_pdu(conn, BT_ATT_OP_INDICATE, len); if (!buf) { BT_WARN("No buffer available to send indication"); + bt_att_req_free(req); return -ENOMEM; } - BT_DBG("conn %p handle 0x%04x", conn, handle); - ind = net_buf_add(buf, sizeof(*ind)); ind->handle = sys_cpu_to_le16(handle); net_buf_add(buf, params->len); memcpy(ind->value, params->data, params->len); - return gatt_send(conn, buf, gatt_indicate_rsp, params); + BT_DBG("conn %p handle 0x%04x", conn, handle); + + req->buf = buf; + + err = bt_att_req_send(conn, req); + if (err) { + bt_att_req_free(req); + } + + return err; } static uint8_t notify_cb(const struct bt_gatt_attr *attr, uint16_t handle, @@ -2724,25 +2769,12 @@ static void gatt_mtu_rsp(struct bt_conn *conn, uint8_t err, const void *pdu, params->func(conn, err, params); } -int bt_gatt_exchange_mtu(struct bt_conn *conn, - struct bt_gatt_exchange_params *params) +static int gatt_exchange_mtu_encode(struct net_buf *buf, size_t len, + void *user_data) { struct bt_att_exchange_mtu_req *req; - struct net_buf *buf; uint16_t mtu; - __ASSERT(conn, "invalid parameter\n"); - __ASSERT(params && params->func, "invalid parameters\n"); - - if (conn->state != BT_CONN_CONNECTED) { - return -ENOTCONN; - } - - buf = bt_att_create_pdu(conn, BT_ATT_OP_MTU_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - mtu = BT_ATT_MTU; BT_DBG("Client MTU %u", mtu); @@ -2750,7 +2782,22 @@ int bt_gatt_exchange_mtu(struct bt_conn *conn, req = net_buf_add(buf, sizeof(*req)); req->mtu = sys_cpu_to_le16(mtu); - return gatt_send(conn, buf, gatt_mtu_rsp, params); + return 0; +} + +int bt_gatt_exchange_mtu(struct bt_conn *conn, + struct bt_gatt_exchange_params *params) +{ + __ASSERT(conn, "invalid parameter\n"); + __ASSERT(params && params->func, "invalid parameters\n"); + + if (conn->state != BT_CONN_CONNECTED) { + return -ENOTCONN; + } + + return gatt_req_send(conn, gatt_mtu_rsp, params, + gatt_exchange_mtu_encode, BT_ATT_OP_MTU_REQ, + sizeof(struct bt_att_exchange_mtu_req)); } static void gatt_discover_next(struct bt_conn *conn, uint16_t last_handle, @@ -2839,17 +2886,12 @@ static void gatt_find_type_rsp(struct bt_conn *conn, uint8_t err, params->func(conn, NULL, params); } -static int gatt_find_type(struct bt_conn *conn, - struct bt_gatt_discover_params *params) +static int gatt_find_type_encode(struct net_buf *buf, size_t len, + void *user_data) { - uint16_t uuid_val; - struct net_buf *buf; + struct bt_gatt_discover_params *params = user_data; struct bt_att_find_type_req *req; - - buf = bt_att_create_pdu(conn, BT_ATT_OP_FIND_TYPE_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } + uint16_t uuid_val; req = net_buf_add(buf, sizeof(*req)); req->start_handle = sys_cpu_to_le16(params->start_handle); @@ -2874,13 +2916,33 @@ static int gatt_find_type(struct bt_conn *conn, case BT_UUID_TYPE_128: net_buf_add_mem(buf, BT_UUID_128(params->uuid)->val, 16); break; + } + + return 0; +} + +static int gatt_find_type(struct bt_conn *conn, + struct bt_gatt_discover_params *params) +{ + size_t len; + + len = sizeof(struct bt_att_find_type_req); + + switch (params->uuid->type) { + case BT_UUID_TYPE_16: + len += BT_UUID_SIZE_16; + break; + case BT_UUID_TYPE_128: + len += BT_UUID_SIZE_128; + break; default: BT_ERR("Unknown UUID type %u", params->uuid->type); - net_buf_unref(buf); return -EINVAL; } - return gatt_send(conn, buf, gatt_find_type_rsp, params); + return gatt_req_send(conn, gatt_find_type_rsp, params, + gatt_find_type_encode, BT_ATT_OP_FIND_TYPE_REQ, + len); } static void read_included_uuid_cb(struct bt_conn *conn, uint8_t err, @@ -2929,23 +2991,26 @@ static void read_included_uuid_cb(struct bt_conn *conn, uint8_t err, return; } -static int read_included_uuid(struct bt_conn *conn, - struct bt_gatt_discover_params *params) +static int read_included_uuid_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_discover_params *params = user_data; struct bt_att_read_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->handle = sys_cpu_to_le16(params->_included.start_handle); + return 0; +} + +static int read_included_uuid(struct bt_conn *conn, + struct bt_gatt_discover_params *params) +{ BT_DBG("handle 0x%04x", params->_included.start_handle); - return gatt_send(conn, buf, read_included_uuid_cb, params); + return gatt_req_send(conn, read_included_uuid_cb, params, + read_included_uuid_encode, BT_ATT_OP_READ_REQ, + sizeof(struct bt_att_read_req)); } static uint16_t parse_include(struct bt_conn *conn, const void *pdu, @@ -3143,17 +3208,12 @@ static void gatt_read_type_rsp(struct bt_conn *conn, uint8_t err, gatt_discover_next(conn, handle, params); } -static int gatt_read_type(struct bt_conn *conn, - struct bt_gatt_discover_params *params) +static int gatt_read_type_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_discover_params *params = user_data; struct bt_att_read_type_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_TYPE_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->start_handle = sys_cpu_to_le16(params->start_handle); req->end_handle = sys_cpu_to_le16(params->end_handle); @@ -3164,10 +3224,18 @@ static int gatt_read_type(struct bt_conn *conn, net_buf_add_le16(buf, BT_UUID_GATT_CHRC_VAL); } + return 0; +} + +static int gatt_read_type(struct bt_conn *conn, + struct bt_gatt_discover_params *params) +{ BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_read_type_rsp, params); + return gatt_req_send(conn, gatt_read_type_rsp, params, + gatt_read_type_encode, BT_ATT_OP_READ_TYPE_REQ, + sizeof(struct bt_att_read_type_req)); } static uint16_t parse_service(struct bt_conn *conn, const void *pdu, @@ -3277,17 +3345,12 @@ static void gatt_read_group_rsp(struct bt_conn *conn, uint8_t err, gatt_discover_next(conn, handle, params); } -static int gatt_read_group(struct bt_conn *conn, - struct bt_gatt_discover_params *params) +static int gatt_read_group_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_discover_params *params = user_data; struct bt_att_read_group_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_GROUP_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->start_handle = sys_cpu_to_le16(params->start_handle); req->end_handle = sys_cpu_to_le16(params->end_handle); @@ -3298,10 +3361,19 @@ static int gatt_read_group(struct bt_conn *conn, net_buf_add_le16(buf, BT_UUID_GATT_SECONDARY_VAL); } + return 0; +} + +static int gatt_read_group(struct bt_conn *conn, + struct bt_gatt_discover_params *params) +{ BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_read_group_rsp, params); + return gatt_req_send(conn, gatt_read_group_rsp, params, + gatt_read_group_encode, + BT_ATT_OP_READ_GROUP_REQ, + sizeof(struct bt_att_read_group_req)); } static void gatt_find_info_rsp(struct bt_conn *conn, uint8_t err, @@ -3417,25 +3489,28 @@ static void gatt_find_info_rsp(struct bt_conn *conn, uint8_t err, params->func(conn, NULL, params); } -static int gatt_find_info(struct bt_conn *conn, - struct bt_gatt_discover_params *params) +static int gatt_find_info_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_discover_params *params = user_data; struct bt_att_find_info_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_FIND_INFO_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->start_handle = sys_cpu_to_le16(params->start_handle); req->end_handle = sys_cpu_to_le16(params->end_handle); + return 0; +} + +static int gatt_find_info(struct bt_conn *conn, + struct bt_gatt_discover_params *params) +{ BT_DBG("start_handle 0x%04x end_handle 0x%04x", params->start_handle, params->end_handle); - return gatt_send(conn, buf, gatt_find_info_rsp, params); + return gatt_req_send(conn, gatt_find_info_rsp, params, + gatt_find_info_encode, BT_ATT_OP_FIND_INFO_REQ, + sizeof(struct bt_att_find_info_req)); } int bt_gatt_discover(struct bt_conn *conn, @@ -3574,38 +3649,36 @@ static void gatt_read_rsp(struct bt_conn *conn, uint8_t err, const void *pdu, } } -static int gatt_read_blob(struct bt_conn *conn, - struct bt_gatt_read_params *params) +static int gatt_read_blob_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_read_params *params = user_data; struct bt_att_read_blob_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_BLOB_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->handle = sys_cpu_to_le16(params->single.handle); req->offset = sys_cpu_to_le16(params->single.offset); + return 0; +} + +static int gatt_read_blob(struct bt_conn *conn, + struct bt_gatt_read_params *params) +{ BT_DBG("handle 0x%04x offset 0x%04x", params->single.handle, params->single.offset); - return gatt_send(conn, buf, gatt_read_rsp, params); + return gatt_req_send(conn, gatt_read_rsp, params, + gatt_read_blob_encode, BT_ATT_OP_READ_BLOB_REQ, + sizeof(struct bt_att_read_blob_req)); } -static int gatt_read_uuid(struct bt_conn *conn, - struct bt_gatt_read_params *params) +static int gatt_read_uuid_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_read_params *params = user_data; struct bt_att_read_type_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_TYPE_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->start_handle = sys_cpu_to_le16(params->by_uuid.start_handle); req->end_handle = sys_cpu_to_le16(params->by_uuid.end_handle); @@ -3616,11 +3689,19 @@ static int gatt_read_uuid(struct bt_conn *conn, net_buf_add_mem(buf, BT_UUID_128(params->by_uuid.uuid)->val, 16); } + return 0; +} + +static int gatt_read_uuid(struct bt_conn *conn, + struct bt_gatt_read_params *params) +{ BT_DBG("start_handle 0x%04x end_handle 0x%04x uuid %s", params->by_uuid.start_handle, params->by_uuid.end_handle, bt_uuid_str(params->by_uuid.uuid)); - return gatt_send(conn, buf, gatt_read_rsp, params); + return gatt_req_send(conn, gatt_read_rsp, params, + gatt_read_uuid_encode, BT_ATT_OP_READ_TYPE_REQ, + sizeof(struct bt_att_read_type_req)); } #if defined(CONFIG_BT_GATT_READ_MULTIPLE) @@ -3642,23 +3723,27 @@ static void gatt_read_mult_rsp(struct bt_conn *conn, uint8_t err, const void *pd params->func(conn, 0, params, NULL, 0); } -static int gatt_read_mult(struct bt_conn *conn, - struct bt_gatt_read_params *params) +static int gatt_read_mult_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_read_params *params = user_data; uint8_t i; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_MULT_REQ, - params->handle_count * sizeof(uint16_t)); - if (!buf) { - return -ENOMEM; - } - for (i = 0U; i < params->handle_count; i++) { net_buf_add_le16(buf, params->handles[i]); } - return gatt_send(conn, buf, gatt_read_mult_rsp, params); + return 0; +} + +static int gatt_read_mult(struct bt_conn *conn, + struct bt_gatt_read_params *params) +{ + BT_DBG("handle_count %zu", params->handle_count); + + return gatt_req_send(conn, gatt_read_mult_rsp, params, + gatt_read_mult_encode, BT_ATT_OP_READ_MULT_REQ, + params->handle_count * sizeof(uint16_t)); } #if defined(CONFIG_BT_EATT) @@ -3705,23 +3790,28 @@ static void gatt_read_mult_vl_rsp(struct bt_conn *conn, uint8_t err, params->func(conn, 0, params, NULL, 0); } -static int gatt_read_mult_vl(struct bt_conn *conn, - struct bt_gatt_read_params *params) +static int gatt_read_mult_vl_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_read_params *params = user_data; uint8_t i; - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_MULT_VL_REQ, - params->handle_count * sizeof(uint16_t)); - if (!buf) { - return -ENOMEM; - } - for (i = 0U; i < params->handle_count; i++) { net_buf_add_le16(buf, params->handles[i]); } - return gatt_send(conn, buf, gatt_read_mult_vl_rsp, params); + return 0; +} + +static int gatt_read_mult_vl(struct bt_conn *conn, + struct bt_gatt_read_params *params) +{ + BT_DBG("handle_count %zu", params->handle_count); + + return gatt_req_send(conn, gatt_read_mult_vl_rsp, params, + gatt_read_mult_vl_encode, + BT_ATT_OP_READ_MULT_VL_REQ, + params->handle_count * sizeof(uint16_t)); } #endif /* CONFIG_BT_EATT */ @@ -3733,17 +3823,25 @@ static int gatt_read_mult(struct bt_conn *conn, } static int gatt_read_mult_vl(struct bt_conn *conn, - struct bt_gatt_read_params *params) + struct bt_gatt_read_params *params) { return -ENOTSUP; } #endif /* CONFIG_BT_GATT_READ_MULTIPLE */ -int bt_gatt_read(struct bt_conn *conn, struct bt_gatt_read_params *params) +static int gatt_read_encode(struct net_buf *buf, size_t len, void *user_data) { - struct net_buf *buf; + struct bt_gatt_read_params *params = user_data; struct bt_att_read_req *req; + req = net_buf_add(buf, sizeof(*req)); + req->handle = sys_cpu_to_le16(params->single.handle); + + return 0; +} + +int bt_gatt_read(struct bt_conn *conn, struct bt_gatt_read_params *params) +{ __ASSERT(conn, "invalid parameters\n"); __ASSERT(params && params->func, "invalid parameters\n"); @@ -3767,17 +3865,11 @@ int bt_gatt_read(struct bt_conn *conn, struct bt_gatt_read_params *params) return gatt_read_blob(conn, params); } - buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - - req = net_buf_add(buf, sizeof(*req)); - req->handle = sys_cpu_to_le16(params->single.handle); - BT_DBG("handle 0x%04x", params->single.handle); - return gatt_send(conn, buf, gatt_read_rsp, params); + return gatt_req_send(conn, gatt_read_rsp, params, gatt_read_encode, + BT_ATT_OP_READ_REQ, + sizeof(struct bt_att_read_req)); } static void gatt_write_rsp(struct bt_conn *conn, uint8_t err, const void *pdu, @@ -3840,23 +3932,24 @@ int bt_gatt_write_without_response_cb(struct bt_conn *conn, uint16_t handle, return bt_att_send(conn, buf, func, user_data); } -static int gatt_exec_write(struct bt_conn *conn, - struct bt_gatt_write_params *params) +static int gatt_exec_encode(struct net_buf *buf, size_t len, void *user_data) { - struct net_buf *buf; struct bt_att_exec_write_req *req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_EXEC_WRITE_REQ, sizeof(*req)); - if (!buf) { - return -ENOMEM; - } - req = net_buf_add(buf, sizeof(*req)); req->flags = BT_ATT_FLAG_EXEC; + return 0; +} + +static int gatt_exec_write(struct bt_conn *conn, + struct bt_gatt_write_params *params) +{ BT_DBG(""); - return gatt_send(conn, buf, gatt_write_rsp, params); + return gatt_req_send(conn, gatt_write_rsp, params, gatt_exec_encode, + BT_ATT_OP_EXEC_WRITE_REQ, + sizeof(struct bt_att_exec_write_req)); } static void gatt_prepare_write_rsp(struct bt_conn *conn, uint8_t err, @@ -3864,16 +3957,28 @@ static void gatt_prepare_write_rsp(struct bt_conn *conn, uint8_t err, void *user_data) { struct bt_gatt_write_params *params = user_data; + const struct bt_att_prepare_write_rsp *rsp = pdu; + size_t len; BT_DBG("err 0x%02x", err); - /* Don't continue in case of error */ if (err) { params->func(conn, err, params); return; } + len = length - sizeof(*rsp); + if (len > params->length) { + params->func(conn, BT_ATT_ERR_INVALID_PDU, params); + return; + } + + /* Update params */ + params->offset += len; + params->data = (const uint8_t *)params->data + len; + params->length -= len; + /* If there is no more data execute */ if (!params->length) { gatt_exec_write(conn, params); @@ -3884,47 +3989,65 @@ static void gatt_prepare_write_rsp(struct bt_conn *conn, uint8_t err, bt_gatt_write(conn, params); } -static int gatt_prepare_write(struct bt_conn *conn, - struct bt_gatt_write_params *params) +static int gatt_prepare_write_encode(struct net_buf *buf, size_t len, + void *user_data) { - struct net_buf *buf; + struct bt_gatt_write_params *params = user_data; struct bt_att_prepare_write_req *req; - uint16_t len; size_t write; - len = MIN(params->length, bt_att_get_mtu(conn) - sizeof(*req) - 1); + req = net_buf_add(buf, sizeof(*req)); + req->handle = sys_cpu_to_le16(params->handle); + req->offset = sys_cpu_to_le16(params->offset); - buf = bt_att_create_pdu(conn, BT_ATT_OP_PREPARE_WRITE_REQ, - sizeof(*req) + len); - if (!buf) { + write = net_buf_append_bytes(buf, len - sizeof(*req), + (uint8_t *)params->data, K_NO_WAIT, NULL, + NULL); + if (write != (len - sizeof(*req))) { return -ENOMEM; } - req = net_buf_add(buf, sizeof(*req)); - req->handle = sys_cpu_to_le16(params->handle); - req->offset = sys_cpu_to_le16(params->offset); + return 0; +} - /* Append as much as possible */ - write = net_buf_append_bytes(buf, len, params->data, K_NO_WAIT, - NULL, NULL); +static int gatt_prepare_write(struct bt_conn *conn, + struct bt_gatt_write_params *params) +{ + uint16_t len, req_len; - /* Update params */ - params->offset += write; - params->data = (const uint8_t *)params->data + len; - params->length -= write; + req_len = sizeof(struct bt_att_prepare_write_req); - BT_DBG("handle 0x%04x offset %u len %u", params->handle, params->offset, - params->length); + len = bt_att_get_mtu(conn) - req_len - 1; + len = MIN(params->length, len); + len += req_len; - return gatt_send(conn, buf, gatt_prepare_write_rsp, params); + return gatt_req_send(conn, gatt_prepare_write_rsp, params, + gatt_prepare_write_encode, + BT_ATT_OP_PREPARE_WRITE_REQ, len); } -int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params) +static int gatt_write_encode(struct net_buf *buf, size_t len, void *user_data) { - struct net_buf *buf; + struct bt_gatt_write_params *params = user_data; struct bt_att_write_req *req; size_t write; + req = net_buf_add(buf, sizeof(*req)); + req->handle = sys_cpu_to_le16(params->handle); + + write = net_buf_append_bytes(buf, params->length, params->data, + K_NO_WAIT, NULL, NULL); + if (write != params->length) { + return -ENOMEM; + } + + return 0; +} + +int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params) +{ + size_t len; + __ASSERT(conn, "invalid parameters\n"); __ASSERT(params && params->func, "invalid parameters\n"); __ASSERT(params->handle, "invalid parameters\n"); @@ -3933,31 +4056,17 @@ int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params) return -ENOTCONN; } + len = sizeof(struct bt_att_write_req) + params->length; + /* Use Prepare Write if offset is set or Long Write is required */ - if (params->offset || - params->length > (bt_att_get_mtu(conn) - sizeof(*req) - 1)) { + if (params->offset || len > (bt_att_get_mtu(conn) - 1)) { return gatt_prepare_write(conn, params); } - buf = bt_att_create_pdu(conn, BT_ATT_OP_WRITE_REQ, - sizeof(*req) + params->length); - if (!buf) { - return -ENOMEM; - } - - req = net_buf_add(buf, sizeof(*req)); - req->handle = sys_cpu_to_le16(params->handle); - - write = net_buf_append_bytes(buf, params->length, params->data, - K_NO_WAIT, NULL, NULL); - if (write != params->length) { - net_buf_unref(buf); - return -ENOMEM; - } - BT_DBG("handle 0x%04x length %u", params->handle, params->length); - return gatt_send(conn, buf, gatt_write_rsp, params); + return gatt_req_send(conn, gatt_write_rsp, params, gatt_write_encode, + BT_ATT_OP_WRITE_REQ, len); } static void gatt_write_ccc_rsp(struct bt_conn *conn, uint8_t err, @@ -3998,28 +4107,30 @@ static void gatt_write_ccc_rsp(struct bt_conn *conn, uint8_t err, } } -static int gatt_write_ccc(struct bt_conn *conn, uint16_t handle, uint16_t value, - bt_att_func_t func, - struct bt_gatt_subscribe_params *params) + +static int gatt_write_ccc_buf(struct net_buf *buf, size_t len, void *user_data) { - struct net_buf *buf; - struct bt_att_write_req *req; + struct bt_gatt_subscribe_params *params = user_data; + struct bt_att_write_req *write_req; - buf = bt_att_create_pdu(conn, BT_ATT_OP_WRITE_REQ, - sizeof(*req) + sizeof(uint16_t)); - if (!buf) { - return -ENOMEM; - } + write_req = net_buf_add(buf, sizeof(*write_req)); + write_req->handle = sys_cpu_to_le16(params->ccc_handle); + net_buf_add_le16(buf, params->value); - req = net_buf_add(buf, sizeof(*req)); - req->handle = sys_cpu_to_le16(handle); - net_buf_add_le16(buf, value); + atomic_set_bit(params->flags, BT_GATT_SUBSCRIBE_FLAG_WRITE_PENDING); - BT_DBG("handle 0x%04x value 0x%04x", handle, value); + return 0; +} - atomic_set_bit(params->flags, BT_GATT_SUBSCRIBE_FLAG_WRITE_PENDING); +static int gatt_write_ccc(struct bt_conn *conn, + struct bt_gatt_subscribe_params *params) +{ + size_t len = sizeof(struct bt_att_write_req) + sizeof(uint16_t); + + BT_DBG("handle 0x%04x value 0x%04x", params->ccc_handle, params->value); - return gatt_send(conn, buf, func, params); + return gatt_req_send(conn, gatt_write_ccc_rsp, params, + gatt_write_ccc_buf, BT_ATT_OP_WRITE_REQ, len); } #if defined(CONFIG_BT_GATT_AUTO_DISCOVER_CCC) @@ -4129,8 +4240,7 @@ int bt_gatt_subscribe(struct bt_conn *conn, return gatt_ccc_discover(conn, params); } #endif - err = gatt_write_ccc(conn, params->ccc_handle, params->value, - gatt_write_ccc_rsp, params); + err = gatt_write_ccc(conn, params); if (err) { gatt_sub_remove(conn, sub, NULL, NULL); return err; @@ -4228,8 +4338,7 @@ int bt_gatt_unsubscribe(struct bt_conn *conn, params->value = 0x0000; - return gatt_write_ccc(conn, params->ccc_handle, params->value, - gatt_write_ccc_rsp, params); + return gatt_write_ccc(conn, params); } void bt_gatt_cancel(struct bt_conn *conn, void *params) @@ -4255,8 +4364,7 @@ static void add_subscriptions(struct bt_conn *conn) /* Force write to CCC to workaround devices that don't * track it properly. */ - gatt_write_ccc(conn, params->ccc_handle, params->value, - gatt_write_ccc_rsp, params); + gatt_write_ccc(conn, params); } } }