[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 <[email protected]>

Author: MarekPieta

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

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.");

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.

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;
 	}