From d82ab7a2ee604902523f6334afe89ccfdf5f4ff4 Mon Sep 17 00:00:00 2001 From: Marek Pieta Date: Thu, 2 Oct 2025 15:14:30 +0200 Subject: [PATCH 1/4] Revert "[nrf noup] bluetooth: conn: Skip buffer ref count check in send_buf" This reverts commit adb8f4e2ec19c3f4146b595daf4d579320a7506b. Signed-off-by: Marek Pieta --- subsys/bluetooth/host/conn.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index d0b6264aaed..7248f7c5495 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -711,27 +711,19 @@ static int send_buf(struct bt_conn *conn, struct net_buf *buf, uint16_t frag_len = MIN(conn_mtu(conn), len); - /* If ATT sent callback is delayed until data transmission - * is done by BLE controller, the transmitted buffer may - * have an additional reference. The reference is used to - * extend lifetime of the net buffer until the data - * transmission is confirmed by ACK of the remote. + /* Check that buf->ref is 1 or 2. It would be 1 if this was + * the only reference (e.g. buf was removed from the conn + * tx_queue). It would be 2 if the tx_data_pull kept it on + * the tx_queue for segmentation. * - * send_buf function can be called multiple times, if buffer - * has to be fragmented over HCI. In that case, the callback - * is provided as an argument only for the last transmitted - * fragment. The `buf->ref == 1` (or 2) check is skipped - * because it's impossible to properly validate number of - * references for the sent fragments if buffers may have the - * additional reference. - * - * Otherwise, check that buf->ref is 1 or 2. It would be 1 - * if this was the only reference (e.g. buf was removed from - * the conn tx_queue). It would be 2 if the tx_data_pull - * kept it on the tx_queue for segmentation. + * Allow for an additional buffer reference if callback is + * provided. This can be used to extend lifetime of the net + * buffer until the data transmission is confirmed by ACK of + * the remote. */ - __ASSERT_NO_MSG(IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX) || (buf->ref == 1) || - (buf->ref == 2)); + if (buf->ref > 2 + (cb ? 1 : 0)) { + __ASSERT_NO_MSG(false); + } /* The reference is always transferred to the frag, so when * the frag is destroyed, the parent reference is decremented. From 65b81283a4cab3515878ddd8e48d57dcd915e89e Mon Sep 17 00:00:00 2001 From: Marek Pieta Date: Thu, 2 Oct 2025 15:14:41 +0200 Subject: [PATCH 2/4] Revert "[nrf noup] bluetooth: conn: Allow for an extra ref in bt_l2cap_send_pdu" This reverts commit 510e36ccf52ba402fe52199287c0a01ffb27ccef. Signed-off-by: Marek Pieta --- subsys/bluetooth/host/conn.c | 17 +++++------------ subsys/bluetooth/host/l2cap.c | 8 ++------ 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 7248f7c5495..f3abf67d4fb 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -711,19 +711,12 @@ static int send_buf(struct bt_conn *conn, struct net_buf *buf, uint16_t frag_len = MIN(conn_mtu(conn), len); - /* Check that buf->ref is 1 or 2. It would be 1 if this was - * the only reference (e.g. buf was removed from the conn - * tx_queue). It would be 2 if the tx_data_pull kept it on - * the tx_queue for segmentation. - * - * Allow for an additional buffer reference if callback is - * provided. This can be used to extend lifetime of the net - * buffer until the data transmission is confirmed by ACK of - * the remote. + /* Check that buf->ref is 1 or 2. It would be 1 if this + * was the only reference (e.g. buf was removed + * from the conn tx_queue). It would be 2 if the + * tx_data_pull kept it on the tx_queue for segmentation. */ - if (buf->ref > 2 + (cb ? 1 : 0)) { - __ASSERT_NO_MSG(false); - } + __ASSERT_NO_MSG((buf->ref == 1) || (buf->ref == 2)); /* The reference is always transferred to the frag, so when * the frag is destroyed, the parent reference is decremented. diff --git a/subsys/bluetooth/host/l2cap.c b/subsys/bluetooth/host/l2cap.c index 0879163a8a1..f65827fac1b 100644 --- a/subsys/bluetooth/host/l2cap.c +++ b/subsys/bluetooth/host/l2cap.c @@ -761,17 +761,13 @@ int bt_l2cap_send_pdu(struct bt_l2cap_le_chan *le_chan, struct net_buf *pdu, return -ENOTCONN; } - /* Allow for an additional buffer reference if callback is provided. This can be used to - * extend lifetime of the net buffer until the data transmission is confirmed by ACK of the - * remote. - */ - if (pdu->ref > 1 + (cb ? 1 : 0)) { + if (pdu->ref != 1) { /* The host may alter the buf contents when fragmenting. Higher * layers cannot expect the buf contents to stay intact. Extra * refs suggests a silent data corruption would occur if not for * this error. */ - LOG_ERR("Expecting up to %d refs, got %d", cb ? 2 : 1, pdu->ref); + LOG_ERR("Expecting 1 ref, got %d", pdu->ref); return -EINVAL; } From e5a694bbe2a869cf68780ab3ad65dd085c6abd37 Mon Sep 17 00:00:00 2001 From: Marek Pieta Date: Thu, 2 Oct 2025 15:14:55 +0200 Subject: [PATCH 3/4] Revert "[nrf noup] bluetooth: att: Allow ATT sent callback after data TX is done" This reverts commit d1d9b2d1e653ca70f8aa34bc74cf13c4d09a4a4a. Signed-off-by: Marek Pieta --- subsys/bluetooth/host/Kconfig.gatt | 17 ----------------- subsys/bluetooth/host/att.c | 17 +---------------- 2 files changed, 1 insertion(+), 33 deletions(-) diff --git a/subsys/bluetooth/host/Kconfig.gatt b/subsys/bluetooth/host/Kconfig.gatt index 221bc413c4b..a04241a3e94 100644 --- a/subsys/bluetooth/host/Kconfig.gatt +++ b/subsys/bluetooth/host/Kconfig.gatt @@ -38,23 +38,6 @@ config BT_ATT_RETRY_ON_SEC_ERR If an ATT request fails due to insufficient security, the host will try to elevate the security level and retry the ATT request. -config BT_ATT_SENT_CB_AFTER_TX - bool "Delay ATT sent callback until data transmission is done by controller [EXPERIMENTAL]" - select EXPERIMENTAL - help - By default, the BLE stack calls sent callback for ATT data when the - data is passed to BLE controller for transmission. Enabling this - Kconfig option delays calling the sent callback until data - transmission is finished by BLE controller (the callback is called - upon receiving the Number of Completed Packets HCI Event). - - The feature is not available in Zephyr RTOS (it's specific to NCS - Zephyr fork). It is a temporary solution allowing to control flow of - GATT notifications with HID reports for HID use-case. - - Enabling this option may require increasing CONFIG_BT_CONN_TX_MAX in - configuration, because ATT would use additional TX contexts. - 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 37d8856bad2..82d82526b5b 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -328,13 +328,6 @@ static void att_disconnect(struct bt_att_chan *chan) } } -static void chan_sent_cb(struct bt_conn *conn, void *user_data, int err) -{ - struct net_buf *nb = user_data; - - net_buf_unref(nb); -} - /* In case of success the ownership of the buffer is transferred to the stack * which takes care of releasing it when it completes transmitting to the * controller. @@ -428,15 +421,7 @@ static int chan_send(struct bt_att_chan *chan, struct net_buf *buf) data->att_chan = chan; - if (IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX)) { - err = bt_l2cap_send_pdu(&chan->chan, buf, chan_sent_cb, net_buf_ref(buf)); - if (err) { - net_buf_unref(buf); - } - } else { - err = bt_l2cap_send_pdu(&chan->chan, buf, NULL, NULL); - } - + err = bt_l2cap_send_pdu(&chan->chan, buf, NULL, NULL); if (err) { if (err == -ENOBUFS) { LOG_ERR("Ran out of TX buffers or contexts."); From 5d62bf51afcf9fe652f8bd84cc67c049bdc489fe Mon Sep 17 00:00:00 2001 From: Marek Pieta Date: Fri, 14 Jun 2024 09:13:48 +0200 Subject: [PATCH 4/4] [nrf noup] bluetooth: att: Allow ATT sent callback after data TX is done By default, the BLE stack calls sent callback for ATT data when the data is passed to BLE controller for transmission. Enabling this Kconfig option delays calling the sent callback until data transmission is finished by BLE controller (the callback is delayed until receiving the Number of Completed Packets HCI Event). If the ATT sent callback is delayed until data transmission is done by BLE controller, the transmitted buffer may have an additional reference. The reference is used to extend lifetime of the net buffer until the data transmission is confirmed by ACK of the remote. Jira: NCSDK-27422 Jira: NCSDK-28624 Jira: NCSDK-35650 Signed-off-by: Marek Pieta --- subsys/bluetooth/host/Kconfig.gatt | 14 ++++++++++++++ subsys/bluetooth/host/att.c | 17 ++++++++++++++++- subsys/bluetooth/host/conn.c | 27 ++++++++++++++++++++++----- subsys/bluetooth/host/l2cap.c | 28 ++++++++++++++++++++++++++-- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/subsys/bluetooth/host/Kconfig.gatt b/subsys/bluetooth/host/Kconfig.gatt index a04241a3e94..3403e8d308b 100644 --- a/subsys/bluetooth/host/Kconfig.gatt +++ b/subsys/bluetooth/host/Kconfig.gatt @@ -38,6 +38,20 @@ config BT_ATT_RETRY_ON_SEC_ERR If an ATT request fails due to insufficient security, the host will try to elevate the security level and retry the ATT request. +config BT_ATT_SENT_CB_AFTER_TX + bool "Delay ATT sent callback until data transmission is done by controller [EXPERIMENTAL]" + select EXPERIMENTAL + help + By default, the BLE stack calls sent callback for ATT data when the + data is passed to BLE controller for transmission. Enabling this + Kconfig option delays calling the sent callback until data + transmission is finished by BLE controller (the callback is called + upon receiving the Number of Completed Packets HCI Event). + + The feature is not available in Zephyr RTOS (it's specific to NCS + Zephyr fork). It is a temporary solution allowing to control flow of + GATT notifications with HID reports for HID use-case. + 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 82d82526b5b..37d8856bad2 100644 --- a/subsys/bluetooth/host/att.c +++ b/subsys/bluetooth/host/att.c @@ -328,6 +328,13 @@ static void att_disconnect(struct bt_att_chan *chan) } } +static void chan_sent_cb(struct bt_conn *conn, void *user_data, int err) +{ + struct net_buf *nb = user_data; + + net_buf_unref(nb); +} + /* In case of success the ownership of the buffer is transferred to the stack * which takes care of releasing it when it completes transmitting to the * controller. @@ -421,7 +428,15 @@ static int chan_send(struct bt_att_chan *chan, struct net_buf *buf) data->att_chan = chan; - err = bt_l2cap_send_pdu(&chan->chan, buf, NULL, NULL); + if (IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX)) { + err = bt_l2cap_send_pdu(&chan->chan, buf, chan_sent_cb, net_buf_ref(buf)); + if (err) { + net_buf_unref(buf); + } + } else { + err = bt_l2cap_send_pdu(&chan->chan, buf, NULL, NULL); + } + if (err) { if (err == -ENOBUFS) { LOG_ERR("Ran out of TX buffers or contexts."); diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index f3abf67d4fb..f4d1f84c8cd 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -711,12 +711,29 @@ static int send_buf(struct bt_conn *conn, struct net_buf *buf, uint16_t frag_len = MIN(conn_mtu(conn), len); - /* Check that buf->ref is 1 or 2. It would be 1 if this - * was the only reference (e.g. buf was removed - * from the conn tx_queue). It would be 2 if the - * tx_data_pull kept it on the tx_queue for segmentation. + /* If ATT sent callback is delayed until data transmission + * is done by BLE controller (CONFIG_BT_ATT_SENT_CB_AFTER_TX), + * the `chan_send` function from `att.c` introduces an additional + * reference. The reference is used to extend lifetime of the net + * buffer until the data transmission is confirmed by ACK of the + * remote (the reference is removed when the TX callback passed + * to `bt_l2cap_send_pdu` is called). + * + * send_buf function can be called multiple times, if buffer + * has to be fragmented over HCI. In that case, the callback + * is provided as an argument only for the last transmitted + * fragment. The `buf->ref == 1` (or 2) check is skipped + * because it's impossible to properly validate number of + * references for the sent fragments if buffers may have the + * additional reference. + * + * Otherwise, check that buf->ref is 1 or 2. It would be 1 + * if this was the only reference (e.g. buf was removed from + * the conn tx_queue). It would be 2 if the tx_data_pull + * kept it on the tx_queue for segmentation. */ - __ASSERT_NO_MSG((buf->ref == 1) || (buf->ref == 2)); + __ASSERT_NO_MSG(IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX) || (buf->ref == 1) || + (buf->ref == 2)); /* The reference is always transferred to the frag, so when * the frag is destroyed, the parent reference is decremented. diff --git a/subsys/bluetooth/host/l2cap.c b/subsys/bluetooth/host/l2cap.c index f65827fac1b..fe5d59b9b58 100644 --- a/subsys/bluetooth/host/l2cap.c +++ b/subsys/bluetooth/host/l2cap.c @@ -273,6 +273,24 @@ static void l2cap_chan_del(struct bt_l2cap_chan *chan) * `l2cap_chan_destroy()` as it is not called for fixed channels. */ while ((buf = k_fifo_get(&le_chan->tx_queue, K_NO_WAIT))) { + /* If ATT sent callback is delayed until data transmission is done by BLE controller + * (CONFIG_BT_ATT_SENT_CB_AFTER_TX), the `chan_send` function from `att.c` + * introduces an additional reference. The reference is used to extend lifetime of + * the net buffer until the data transmission is confirmed by ACK of the + * remote (the reference is removed when the TX callback passed to + * `bt_l2cap_send_pdu` is called). + * + * The TX callbacks of the buffers removed here are not called, therefore the + * additional reference needs to be removed here. + */ + if (IS_ENABLED(CONFIG_BT_ATT_SENT_CB_AFTER_TX)) { + __ASSERT_NO_MSG(buf->ref <= 2); + + if (buf->ref == 2) { + net_buf_unref(buf); + } + } + net_buf_unref(buf); } @@ -761,13 +779,19 @@ int bt_l2cap_send_pdu(struct bt_l2cap_le_chan *le_chan, struct net_buf *pdu, return -ENOTCONN; } - if (pdu->ref != 1) { + /* If ATT sent callback is delayed until data transmission is done by BLE controller + * (CONFIG_BT_ATT_SENT_CB_AFTER_TX), the `chan_send` function from `att.c` introduces an + * additional reference. The reference is used to extend lifetime of the net buffer until + * the data transmission is confirmed by ACK of the remote (the reference is removed when + * the TX callback passed to `bt_l2cap_send_pdu` is called). + */ + if (pdu->ref > 1 + (cb ? 1 : 0)) { /* The host may alter the buf contents when fragmenting. Higher * layers cannot expect the buf contents to stay intact. Extra * refs suggests a silent data corruption would occur if not for * this error. */ - LOG_ERR("Expecting 1 ref, got %d", pdu->ref); + LOG_ERR("Expecting up to %d refs, got %d", cb ? 2 : 1, pdu->ref); return -EINVAL; }