[nrf fromtree] Revert "Bluetooth: att: re-use REQ buf for RSP"

This reverts commit aa7954bd4725bbd46e974a03c0d0312b7e9a483f.

Signed-off-by: Jonathan Rico <[email protected]>
(cherry picked from commit 946aed54febdec53afabce843eb036a4216cfe89)

Author: jori-nordic

subsys/bluetooth/host/att.c

@@ -103,7 +103,6 @@ struct bt_att_chan {
 	ATOMIC_DEFINE(flags, ATT_NUM_FLAGS);
 	struct bt_att_req	*req;
 	struct k_fifo		tx_queue;
-	struct net_buf		*rsp_buf;
 	struct bt_att_tx_meta_data rsp_meta;
 	struct k_work_delayable	timeout_work;
 	sys_snode_t		node;
@@ -660,7 +659,7 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 	struct net_buf *buf;
 	struct bt_att_tx_meta_data *data;
 	k_timeout_t timeout;
-	bool re_use = false;
+	bool is_rsp = false;
 
 	if (len + sizeof(op) > bt_att_mtu(chan)) {
 		LOG_WRN("ATT MTU exceeded, max %u, wanted %zu", bt_att_mtu(chan),
@@ -673,23 +672,19 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 	case ATT_CONFIRMATION:
 		/* Use a timeout only when responding/confirming */
 		timeout = BT_ATT_TIMEOUT;
-		re_use = true;
+		is_rsp = true;
 		break;
 	default:
 		timeout = K_FOREVER;
 	}
 
-	if (IS_ENABLED(CONFIG_BT_GATT_READ_MULTIPLE) &&
-	    (op == BT_ATT_OP_READ_MULT_RSP ||
-	     op == BT_ATT_OP_READ_MULT_VL_RSP)) {
-		/* We can't re-use the REQ buffer (see below) for these two
-		 * opcodes, as the handler will read from it _after_ allocating
-		 * the RSP buffer.
-		 */
-		re_use = false;
+	buf = bt_l2cap_create_pdu_timeout(NULL, 0, timeout);
+	if (!buf) {
+		LOG_ERR("Unable to allocate buffer for op 0x%02x", op);
+		return NULL;
 	}
 
-	if (re_use) {
+	if (is_rsp) {
 		/* There can only ever be one transaction at a time on a
 		 * bearer/channel. Use a dedicated channel meta-data to ensure
 		 * we can always queue an (error) RSP for each REQ. The ATT
@@ -706,27 +701,8 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 			return NULL;
 		}
 		data = &chan->rsp_meta;
-
-		/* Re-use REQ buf to avoid dropping the REQ and timing out.
-		 * This only works if the bearer used to RX REQs is the same as
-		 * for sending the RSP. That should always be the case
-		 * (per-spec).
-		 */
-		__ASSERT_NO_MSG(chan->rsp_buf);
-		buf = net_buf_ref(chan->rsp_buf);
-
-		net_buf_reset(buf);
-		net_buf_reserve(buf, BT_L2CAP_BUF_SIZE(0));
-
-		LOG_DBG("re-using REQ buf %p for RSP", buf);
+		LOG_INF("alloc rsp meta");
 	} else {
-		LOG_DBG("alloc buf & meta from global pools");
-		buf = bt_l2cap_create_pdu_timeout(NULL, 0, timeout);
-		if (!buf) {
-			LOG_ERR("Unable to allocate buffer for op 0x%02x", op);
-			return NULL;
-		}
-
 		data = tx_meta_data_alloc(timeout);
 		if (!data) {
 			LOG_WRN("Unable to allocate ATT TX meta");
@@ -810,7 +786,6 @@ static void send_err_rsp(struct bt_att_chan *chan, uint8_t req, uint16_t handle,
 
 	buf = bt_att_chan_create_pdu(chan, BT_ATT_OP_ERROR_RSP, sizeof(*rsp));
 	if (!buf) {
-		LOG_ERR("unable to allocate buf for error response");
 		return;
 	}
 
@@ -2894,40 +2869,26 @@ static int bt_att_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 		}
 	}
 
-	/* Thread-local variable, shouldn't be used by anything else */
-	__ASSERT_NO_MSG(!att_chan->rsp_buf);
-
-	/* Mark buffer free for re-use by the opcode handler.
-	 *
-	 * This allows ATT to always be able to send a RSP (or err RSP)
-	 * to the peer, regardless of the TX buffer usage by other stack
-	 * users (e.g. GATT notifications, L2CAP using global pool, SMP,
-	 * etc..), avoiding an ATT timeout due to resource usage.
-	 *
-	 * The ref is taken by `bt_att_chan_create_pdu`.
-	 */
-	att_chan->rsp_buf = net_buf_ref(buf);
-
 	if (!handler) {
 		LOG_WRN("Unhandled ATT code 0x%02x", hdr->code);
 		if (att_op_get_type(hdr->code) != ATT_COMMAND &&
 		    att_op_get_type(hdr->code) != ATT_INDICATION) {
 			send_err_rsp(att_chan, hdr->code, 0,
 				     BT_ATT_ERR_NOT_SUPPORTED);
 		}
-		goto exit;
+		return 0;
 	}
 
 	if (IS_ENABLED(CONFIG_BT_ATT_ENFORCE_FLOW)) {
 		if (handler->type == ATT_REQUEST &&
 		    atomic_test_and_set_bit(att_chan->flags, ATT_PENDING_RSP)) {
 			LOG_WRN("Ignoring unexpected request");
-			goto exit;
+			return 0;
 		} else if (handler->type == ATT_INDICATION &&
 			   atomic_test_and_set_bit(att_chan->flags,
 						   ATT_PENDING_CFM)) {
 			LOG_WRN("Ignoring unexpected indication");
-			goto exit;
+			return 0;
 		}
 	}
 
@@ -2943,10 +2904,6 @@ static int bt_att_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 		send_err_rsp(att_chan, hdr->code, 0, err);
 	}
 
-exit:
-	net_buf_unref(att_chan->rsp_buf);
-	att_chan->rsp_buf = NULL;
-
 	return 0;
 }
 

subsys/bluetooth/host/buf.c

@@ -43,15 +43,14 @@ NET_BUF_POOL_FIXED_DEFINE(discardable_pool, CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT,
 #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
 NET_BUF_POOL_DEFINE(acl_in_pool, CONFIG_BT_BUF_ACL_RX_COUNT,
 		    BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
-		    MAX(sizeof(struct bt_buf_data), CONFIG_BT_CONN_TX_USER_DATA_SIZE),
-		    bt_hci_host_num_completed_packets);
+		    sizeof(struct acl_data), bt_hci_host_num_completed_packets);
 
 NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT,
 			  BT_BUF_EVT_RX_SIZE, 8,
 			  NULL);
 #else
 NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT,
-			  BT_BUF_RX_SIZE, CONFIG_BT_CONN_TX_USER_DATA_SIZE,
+			  BT_BUF_RX_SIZE, 8,
 			  NULL);
 #endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 

subsys/bluetooth/host/conn_internal.h

@@ -146,6 +146,17 @@ struct bt_conn_tx {
 	uint32_t pending_no_cb;
 };
 
+struct acl_data {
+	/* Extend the bt_buf user data */
+	struct bt_buf_data buf_data;
+
+	/* Index into the bt_conn storage array */
+	uint8_t  index;
+
+	/** ACL connection handle */
+	uint16_t handle;
+};
+
 struct bt_conn {
 	uint16_t			handle;
 	enum bt_conn_type	type;

subsys/bluetooth/host/hci_core.c

@@ -111,16 +111,8 @@ struct cmd_data {
 
 static struct cmd_data cmd_data[CONFIG_BT_BUF_CMD_TX_COUNT];
 
-#if defined(CONFIG_BT_CONN)
-struct acl_data {
-	uint16_t acl_handle;
-};
-
-static struct acl_data acl_data[CONFIG_BT_BUF_ACL_RX_COUNT];
-#endif
-
 #define cmd(buf) (&cmd_data[net_buf_id(buf)])
-#define acl(buf) (&acl_data[net_buf_id(buf)])
+#define acl(buf) ((struct acl_data *)net_buf_user_data(buf))
 
 void bt_hci_cmd_state_set_init(struct net_buf *buf,
 			       struct bt_hci_cmd_state_set *state,
@@ -209,9 +201,10 @@ void bt_hci_host_num_completed_packets(struct net_buf *buf)
 {
 
 	struct bt_hci_cp_host_num_completed_packets *cp;
-	uint16_t handle = acl(buf)->acl_handle;
+	uint16_t handle = acl(buf)->handle;
 	struct bt_hci_handle_count *hc;
 	struct bt_conn *conn;
+	uint8_t index = acl(buf)->index;
 
 	net_buf_destroy(buf);
 
@@ -220,9 +213,9 @@ void bt_hci_host_num_completed_packets(struct net_buf *buf)
 		return;
 	}
 
-	conn = bt_conn_lookup_handle(handle, BT_CONN_TYPE_ALL);
+	conn = bt_conn_lookup_index(index);
 	if (!conn) {
-		LOG_WRN("Unable to look up conn with ACL handle %u", handle);
+		LOG_WRN("Unable to look up conn with index 0x%02x", index);
 		return;
 	}
 
@@ -516,23 +509,26 @@ static void hci_acl(struct net_buf *buf)
 	handle = sys_le16_to_cpu(hdr->handle);
 	flags = bt_acl_flags(handle);
 
-	acl(buf)->acl_handle = bt_acl_handle(handle);
+	acl(buf)->handle = bt_acl_handle(handle);
+	acl(buf)->index = BT_CONN_INDEX_INVALID;
 
-	LOG_DBG("handle %u len %u flags %u", acl(buf)->acl_handle, len, flags);
+	LOG_DBG("handle %u len %u flags %u", acl(buf)->handle, len, flags);
 
 	if (buf->len != len) {
 		LOG_ERR("ACL data length mismatch (%u != %u)", buf->len, len);
 		net_buf_unref(buf);
 		return;
 	}
 
-	conn = bt_conn_lookup_handle(acl(buf)->acl_handle, BT_CONN_TYPE_ALL);
+	conn = bt_conn_lookup_handle(acl(buf)->handle, BT_CONN_TYPE_ALL);
 	if (!conn) {
-		LOG_ERR("Unable to find conn for handle %u", acl(buf)->acl_handle);
+		LOG_ERR("Unable to find conn for handle %u", acl(buf)->handle);
 		net_buf_unref(buf);
 		return;
 	}
 
+	acl(buf)->index = bt_conn_index(conn);
+
 	bt_conn_recv(conn, buf, flags);
 	bt_conn_unref(conn);
 }