Bluetooth: AVRCP: fix bitfield issue.

The bit order can be incorrect when use bit field definition.

Signed-off-by: Zihao Gao <[email protected]>

subsys/bluetooth/host/classic/avctp.c

@@ -73,6 +73,8 @@ static int avctp_l2cap_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 	struct net_buf *rsp;
 	struct bt_avctp *session = AVCTP_CHAN(chan);
 	struct bt_avctp_header *hdr = (void *)buf->data;
+	uint8_t tid = BT_AVCTP_HDR_GET_TRANSACTION_LABLE(hdr);
+	bt_avctp_cr_t cr = BT_AVCTP_HDR_GET_CR(hdr);
 
 	if (buf->len < sizeof(*hdr)) {
 		LOG_ERR("invalid AVCTP header received");
@@ -86,9 +88,10 @@ static int avctp_l2cap_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 #endif
 	default:
 		LOG_ERR("unsupported AVCTP PID received: 0x%04x", sys_be16_to_cpu(hdr->pid));
-		if (hdr->cr == BT_AVCTP_CMD) {
-			rsp = bt_avctp_create_pdu(session, BT_AVCTP_RESPONSE, BT_AVCTP_IPID_INVALID,
-						  &hdr->tid, hdr->pid);
+		if (cr == BT_AVCTP_CMD) {
+			rsp = bt_avctp_create_pdu(session, BT_AVCTP_RESPONSE,
+						  BT_AVCTP_PKT_TYPE_SINGLE, BT_AVCTP_IPID_INVALID,
+						  &tid, hdr->pid);
 			if (!rsp) {
 				return -ENOMEM;
 			}
@@ -132,7 +135,8 @@ int bt_avctp_disconnect(struct bt_avctp *session)
 }
 
 struct net_buf *bt_avctp_create_pdu(struct bt_avctp *session, bt_avctp_cr_t cr,
-				    bt_avctp_ipid_t ipid, uint8_t *tid, uint16_t pid)
+				    bt_avctp_pkt_type_t pkt_type, bt_avctp_ipid_t ipid,
+				    uint8_t *tid, uint16_t pid)
 {
 	struct net_buf *buf;
 	struct bt_avctp_header *hdr;
@@ -146,17 +150,18 @@ struct net_buf *bt_avctp_create_pdu(struct bt_avctp *session, bt_avctp_cr_t cr,
 	}
 
 	hdr = net_buf_add(buf, sizeof(*hdr));
-	hdr->cr = cr;
-	hdr->ipid = ipid;
-	hdr->pkt_type = BT_AVCTP_PKT_TYPE_SINGLE;
-	hdr->tid = *tid;
+	BT_AVCTP_HDR_SET_TRANSACTION_LABLE(hdr, *tid);
+	BT_AVCTP_HDR_SET_PACKET_TYPE(hdr, pkt_type);
+	BT_AVCTP_HDR_SET_CR(hdr, cr);
+	BT_AVCTP_HDR_SET_IPID(hdr, ipid);
 	hdr->pid = pid;
 
 	if (cr == BT_AVCTP_CMD) {
 		*tid = (*tid + 1) & 0x0F; /* Incremented by one */
 	}
 
-	LOG_DBG("cr:0x%X, tid:0x%02X", hdr->cr, hdr->tid);
+	LOG_DBG("cr:0x%lX, tid:0x%02lX", BT_AVCTP_HDR_GET_CR(hdr),
+		BT_AVCTP_HDR_GET_TRANSACTION_LABLE(hdr));
 	return buf;
 }
 

subsys/bluetooth/host/classic/avctp_internal.h

@@ -23,16 +23,54 @@ typedef enum __packed {
 
 typedef enum __packed {
 	BT_AVCTP_PKT_TYPE_SINGLE = 0b00,
+	BT_AVCTP_PKT_TYPE_START = 0b01,
+	BT_AVCTP_PKT_TYPE_CONTINUE = 0b10,
+	BT_AVCTP_PKT_TYPE_END = 0b11,
 } bt_avctp_pkt_type_t;
 
 struct bt_avctp_header {
-	uint8_t ipid: 1;     /* Invalid Profile Identifier (1), otherwise zero (0) */
-	uint8_t cr: 1;       /* Command(0) or Respone(1) */
-	uint8_t pkt_type: 2; /* Set to zero (00) for single L2CAP packet */
-	uint8_t tid: 4;      /* Transaction label */
-	uint16_t pid;        /* Profile Identifier */
+	uint8_t byte0; /** [7:4]: Transaction label, [3:2]: Packet_type, [1]: C/R, [0]: IPID */
+	uint16_t pid;  /** Profile Identifier */
 } __packed;
 
+/** Transaction label provided by the application and is replicated by the sender of the message in
+ *  each packet of the sequence. It isused at the receiver side to identify packets that belong to
+ *  the same message.
+ */
+#define BT_AVCTP_HDR_GET_TRANSACTION_LABLE(hdr) FIELD_GET(GENMASK(7, 4), ((hdr)->byte0))
+/** Set to zero (00) to indicate that the command/response message is transmitted in a single L2CAP
+ *  packet. Alternatively, set to (01) for start, (10) for continue, or (11) for end packet.
+ */
+#define BT_AVCTP_HDR_GET_PACKET_TYPE(hdr)       FIELD_GET(GENMASK(3, 2), ((hdr)->byte0))
+/** Indicates whether the messageconveys a command frame (0) or a response frame (1). */
+#define BT_AVCTP_HDR_GET_CR(hdr)                FIELD_GET(BIT(1), ((hdr)->byte0))
+/** The IPID bit is set in a response message to indicate an invalid Profile Identifier received in
+ *  the command message of the same transaction; otherwise this bit is set to zero. In command
+ *  messages this bit is set to zero. This field is only present in the start packet of the message.
+ */
+#define BT_AVCTP_HDR_GET_IPID(hdr)              FIELD_GET(BIT(0), ((hdr)->byte0))
+
+/** Transaction label provided by the application and is replicated by the sender of the message in
+ *  each packet of the sequence. It isused at the receiver side to identify packets that belong to
+ *  the same message.
+ */
+#define BT_AVCTP_HDR_SET_TRANSACTION_LABLE(hdr, tl)                                                \
+	(hdr)->byte0 = (((hdr)->byte0) & ~GENMASK(7, 4)) | FIELD_PREP(GENMASK(7, 4), (tl))
+/** Set to zero (00) to indicate that the command/response message is transmitted in a single L2CAP
+ *  packet. Alternatively, set to (01) for start, (10) for continue, or (11) for end packet.
+ */
+#define BT_AVCTP_HDR_SET_PACKET_TYPE(hdr, packet_type)                                             \
+	(hdr)->byte0 = (((hdr)->byte0) & ~GENMASK(3, 2)) | FIELD_PREP(GENMASK(3, 2), (packet_type))
+/** Indicates whether the messageconveys a command frame (0) or a response frame (1). */
+#define BT_AVCTP_HDR_SET_CR(hdr, cr)                                                               \
+	(hdr)->byte0 = (((hdr)->byte0) & ~BIT(1)) | FIELD_PREP(BIT(1), (cr))
+/** The IPID bit is set in a response message to indicate an invalid Profile Identifier received in
+ *  the command message of the same transaction; otherwise this bit is set to zero. In command
+ *  messages this bit is set to zero. This field is only present in the start packet of the message.
+ */
+#define BT_AVCTP_HDR_SET_IPID(hdr, ipid)                                                           \
+	(hdr)->byte0 = (((hdr)->byte0) & ~BIT(0)) | FIELD_PREP(BIT(0), (ipid))
+
 struct bt_avctp;
 
 struct bt_avctp_ops_cb {
@@ -64,7 +102,8 @@ int bt_avctp_disconnect(struct bt_avctp *session);
 
 /* Create AVCTP PDU */
 struct net_buf *bt_avctp_create_pdu(struct bt_avctp *session, bt_avctp_cr_t cr,
-				    bt_avctp_ipid_t ipid, uint8_t *tid, uint16_t pid);
+				    bt_avctp_pkt_type_t pkt_type, bt_avctp_ipid_t ipid,
+				    uint8_t *tid, uint16_t pid);
 
 /* Send AVCTP PDU */
 int bt_avctp_send(struct bt_avctp *session, struct net_buf *buf);

subsys/bluetooth/host/classic/avrcp.c

@@ -246,26 +246,36 @@ static int avrcp_recv(struct bt_avctp *session, struct net_buf *buf)
 	struct bt_avrcp *avrcp = AVRCP_AVCTP(session);
 	struct bt_avctp_header *avctp_hdr;
 	struct bt_avrcp_header *avrcp_hdr;
+	uint8_t tid;
+	bt_avctp_cr_t cr;
+	bt_avrcp_ctype_t ctype;
+	bt_avrcp_subunit_id_t subunit_id;
+	bt_avrcp_subunit_type_t subunit_type;
 
 	avctp_hdr = (void *)buf->data;
 	net_buf_pull(buf, sizeof(*avctp_hdr));
 	avrcp_hdr = (void *)buf->data;
 
+	tid = BT_AVCTP_HDR_GET_TRANSACTION_LABLE(avctp_hdr);
+	cr = BT_AVCTP_HDR_GET_CR(avctp_hdr);
+	ctype = BT_AVRCP_HDR_GET_CTYPE(avrcp_hdr);
+	subunit_id = BT_AVRCP_HDR_GET_SUBUNIT_ID(avrcp_hdr);
+	subunit_type = BT_AVRCP_HDR_GET_SUBUNIT_TYPE(avrcp_hdr);
+
 	if (avctp_hdr->pid != sys_cpu_to_be16(BT_SDP_AV_REMOTE_SVCLASS)) {
 		return -EINVAL; /* Ignore other profile */
 	}
 
-	LOG_DBG("AVRCP msg received, cr:0x%X, tid:0x%X, ctype: 0x%X, opc:0x%02X,", avctp_hdr->cr,
-		avctp_hdr->tid, avrcp_hdr->ctype, avrcp_hdr->opcode);
-	if (avctp_hdr->cr == BT_AVCTP_RESPONSE) {
+	LOG_DBG("AVRCP msg received, cr:0x%X, tid:0x%X, ctype: 0x%X, opc:0x%02X,", cr, tid, ctype,
+		avrcp_hdr->opcode);
+	if (cr == BT_AVCTP_RESPONSE) {
 		if (avrcp_hdr->opcode == BT_AVRCP_OPC_VENDOR_DEPENDENT &&
-		    avrcp_hdr->ctype == BT_AVRCP_CTYPE_CHANGED) {
+		    ctype == BT_AVRCP_CTYPE_CHANGED) {
 			/* Status changed notifiation, do not reset timer */
 		} else if (avrcp_hdr->opcode == BT_AVRCP_OPC_PASS_THROUGH) {
 			/* No max response time for pass through commands  */
-		} else if (avrcp->req.tid != avctp_hdr->tid ||
-			   avrcp->req.subunit != avrcp_hdr->subunit_id ||
-			   avrcp->req.opcode != avrcp_hdr->opcode) {
+		} else if (tid != avrcp->req.tid || subunit_id != avrcp->req.subunit ||
+			   avrcp_hdr->opcode != avrcp->req.opcode) {
 			LOG_WRN("unexpected AVRCP response, expected tid:0x%X, subunit:0x%X, "
 				"opc:0x%02X",
 				avrcp->req.tid, avrcp->req.subunit, avrcp->req.opcode);
@@ -370,7 +380,8 @@ static struct net_buf *avrcp_create_pdu(struct bt_avrcp *avrcp, bt_avctp_cr_t cr
 {
 	struct net_buf *buf;
 
-	buf = bt_avctp_create_pdu(&(avrcp->session), cr, BT_AVCTP_IPID_NONE, &avrcp->local_tid,
+	buf = bt_avctp_create_pdu(&(avrcp->session), cr, BT_AVCTP_PKT_TYPE_SINGLE,
+				  BT_AVCTP_IPID_NONE, &avrcp->local_tid,
 				  sys_cpu_to_be16(BT_SDP_AV_REMOTE_SVCLASS));
 
 	return buf;
@@ -388,10 +399,10 @@ static struct net_buf *avrcp_create_unit_pdu(struct bt_avrcp *avrcp, bt_avctp_cr
 
 	cmd = net_buf_add(buf, sizeof(*cmd));
 	memset(cmd, 0, sizeof(*cmd));
-	cmd->hdr.ctype =
-		(cr == BT_AVCTP_CMD) ? BT_AVRCP_CTYPE_STATUS : BT_AVRCP_CTYPE_IMPLEMENTED_STABLE;
-	cmd->hdr.subunit_id = BT_AVRCP_SUBUNIT_ID_IGNORE;
-	cmd->hdr.subunit_type = BT_AVRCP_SUBUNIT_TYPE_UNIT;
+	BT_AVRCP_HDR_SET_CTYPE(&cmd->hdr, cr == BT_AVCTP_CMD ? BT_AVRCP_CTYPE_STATUS
+							     : BT_AVRCP_CTYPE_IMPLEMENTED_STABLE);
+	BT_AVRCP_HDR_SET_SUBUNIT_ID(&cmd->hdr, BT_AVRCP_SUBUNIT_ID_IGNORE);
+	BT_AVRCP_HDR_SET_SUBUNIT_TYPE(&cmd->hdr, BT_AVRCP_SUBUNIT_TYPE_UNIT);
 	cmd->hdr.opcode = BT_AVRCP_OPC_UNIT_INFO;
 
 	return buf;
@@ -400,23 +411,24 @@ static struct net_buf *avrcp_create_unit_pdu(struct bt_avrcp *avrcp, bt_avctp_cr
 static int avrcp_send(struct bt_avrcp *avrcp, struct net_buf *buf)
 {
 	int err;
-	struct bt_avctp_header avctp_hdr;
-	struct bt_avrcp_header avrcp_hdr;
-
-	memcpy(&avctp_hdr, buf->data, sizeof(avctp_hdr));
-	memcpy(&avrcp_hdr, buf->data + sizeof(avctp_hdr), sizeof(avrcp_hdr));
-
-	LOG_DBG("AVRCP send cr:0x%X, tid:0x%X, ctype: 0x%X, opc:0x%02X\n", avctp_hdr.cr,
-		avctp_hdr.tid, avrcp_hdr.ctype, avrcp_hdr.opcode);
+	struct bt_avctp_header *avctp_hdr = (struct bt_avctp_header *)(buf->data);
+	struct bt_avrcp_header *avrcp_hdr = (struct bt_avrcp_header *)(buf->data + sizeof(*avctp_hdr));
+	uint8_t tid = BT_AVCTP_HDR_GET_TRANSACTION_LABLE(avctp_hdr);
+	bt_avctp_cr_t cr = BT_AVCTP_HDR_GET_CR(avctp_hdr);
+	bt_avrcp_ctype_t ctype = BT_AVRCP_HDR_GET_CTYPE(avrcp_hdr);
+	bt_avrcp_subunit_type_t subunit_id = BT_AVRCP_HDR_GET_SUBUNIT_ID(avrcp_hdr);
+
+	LOG_DBG("AVRCP send cr:0x%X, tid:0x%X, ctype: 0x%X, opc:0x%02X\n", cr, tid, ctype,
+		avrcp_hdr->opcode);
 	err = bt_avctp_send(&(avrcp->session), buf);
 	if (err < 0) {
 		return err;
 	}
 
-	if (avctp_hdr.cr == BT_AVCTP_CMD && avrcp_hdr.opcode != BT_AVRCP_OPC_PASS_THROUGH) {
-		avrcp->req.tid = avctp_hdr.tid;
-		avrcp->req.subunit = avrcp_hdr.subunit_id;
-		avrcp->req.opcode = avrcp_hdr.opcode;
+	if (cr == BT_AVCTP_CMD && avrcp_hdr->opcode != BT_AVRCP_OPC_PASS_THROUGH) {
+		avrcp->req.tid = tid;
+		avrcp->req.subunit = subunit_id;
+		avrcp->req.opcode = avrcp_hdr->opcode;
 
 		k_work_reschedule(&avrcp->timeout_work, AVRCP_TIMEOUT);
 	}

subsys/bluetooth/host/classic/avrcp_internal.h

@@ -56,13 +56,44 @@ struct bt_avrcp_req {
 };
 
 struct bt_avrcp_header {
-	uint8_t ctype: 4;        /* Command type codes */
-	uint8_t rfa: 4;          /* Zero according to AV/C command frame */
-	uint8_t subunit_id: 3;   /* Zero (0x0) or Ignore (0x7) according to AVRCP */
-	uint8_t subunit_type: 5; /* Unit (0x1F) or Panel (0x9) according to AVRCP */
-	uint8_t opcode;          /* Unit Info, Subunit Info, Vendor Dependent, or Pass Through */
+	uint8_t byte0;  /** [7:4]: RFA, [3:0]: Ctype */
+	uint8_t byte1;  /** [7:3]: Subunit_type, [2:0]: Subunit_ID */
+	uint8_t opcode; /** Unit Info, Subunit Info, Vendor Dependent, or Pass Through */
 } __packed;
 
+/** The 4-bit command type or the 4-bit response code. */
+#define BT_AVRCP_HDR_GET_CTYPE(hdr)        FIELD_GET(GENMASK(3, 0), ((hdr)->byte0))
+/** Taken together, the subunit_type and subunit_ID fields define the command recipient’s  address
+ *  within the target. These fields enable the target to determine  whether the command is
+ *  addressed to the target  unit, or to a specific subunit within the target. The values in these
+ *  fields remain unchanged in the response frame.
+ */
+#define BT_AVRCP_HDR_GET_SUBUNIT_ID(hdr)   FIELD_GET(GENMASK(2, 0), ((hdr)->byte1))
+/** Taken together, the subunit_type and subunit_ID fields define the command recipient’s  address
+ *  within the target. These fields enable the target to determine  whether the command is
+ *  addressed to the target  unit, or to a specific subunit within the target. The values in these
+ *  fields remain unchanged in the response frame.
+ */
+#define BT_AVRCP_HDR_GET_SUBUNIT_TYPE(hdr) FIELD_GET(GENMASK(7, 3), ((hdr)->byte1))
+
+/** The 4-bit command type or the 4-bit response code. */
+#define BT_AVRCP_HDR_SET_CTYPE(hdr, ctype)                                                         \
+	(hdr)->byte0 = (((hdr)->byte0) & ~GENMASK(3, 0)) | FIELD_PREP(GENMASK(3, 0), (ctype))
+/** Taken together, the subunit_type and subunit_ID fields define the command recipient’s  address
+ *  within the target. These fields enable the target to determine  whether the command is
+ *  addressed to the target  unit, or to a specific subunit within the target. The values in these
+ *  fields remain unchanged in the response frame.
+ */
+#define BT_AVRCP_HDR_SET_SUBUNIT_ID(hdr, subunit_id)                                               \
+	(hdr)->byte1 = (((hdr)->byte1) & ~GENMASK(2, 0)) | FIELD_PREP(GENMASK(2, 0), (subunit_id))
+/** Taken together, the subunit_type and subunit_ID fields define the command recipient’s  address
+ *  within the target. These fields enable the target to determine  whether the command is
+ *  addressed to the target  unit, or to a specific subunit within the target. The values in these
+ *  fields remain unchanged in the response frame.
+ */
+#define BT_AVRCP_HDR_SET_SUBUNIT_TYPE(hdr, subunit_type)                                           \
+	(hdr)->byte1 = (((hdr)->byte1) & ~GENMASK(7, 3)) | FIELD_PREP(GENMASK(7, 3), (subunit_type))
+
 struct bt_avrcp_unit_info_cmd {
 	struct bt_avrcp_header hdr;
 	uint8_t data[0];