Bluetooth: hci_qca: Add support for controller debug logs.

PATCH Superseded Balakrishna Godavarthi 2 participants 11 messages Oct 3, 2018 – Oct 24, 2018

Subsystems (3)

Arm Qualcomm mailing list Bluetooth drivers Qualcomm Bluetooth driver

lore

Share & tools

lore

Balakrishna Godavarthi

author

2018-10-03 15:11 UTC #0 lore raw

PATCH v2 1/1 Bluetooth: hci_qca: Add support for controller debug logs.

This patch will prevent error messages splashing on console.

[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804

QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.

Signed-off-by Harish Bandi <[email protected]>

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
v2:
* Addressed reviewer comments.
v1:
* initial patch

---
 drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

drivers/bluetooth/hci_qca.c +19 -1

index d98ed0442201..63ac3c6b4149 100644
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,

@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
+		return hci_recv_diag(hdev, skb);
+
+	return hci_recv_frame(hdev, skb);
+}
+
 #define QCA_IBS_SLEEP_IND_EVENT \
 	.type = HCI_IBS_SLEEP_IND, \
 	.hlen = 0, \

@@ -871,7 +889,7 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	.maxlen = HCI_MAX_IBS_SIZE
 
 static const struct h4_recv_pkt qca_recv_pkts[] = {
-	{ H4_RECV_ACL,             .recv = hci_recv_frame    },
+	{ H4_RECV_ACL,             .recv = qca_recv_acl_data },
 	{ H4_RECV_SCO,             .recv = hci_recv_frame    },
 	{ H4_RECV_EVENT,           .recv = hci_recv_frame    },
 	{ QCA_IBS_WAKE_IND_EVENT,  .recv = qca_ibs_wake_ind  },

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Marcel Holtmann

maintainer

2018-10-03 16:56 UTC #1 lore raw

Hi Balakrishna,

Quoted 27 lines

This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644

drivers/bluetooth/hci_qca.c line 69

Quoted 7 lines

drivers/bluetooth/hci_qca.c +4 -0

@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+

since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.

Quoted 6 lines

drivers/bluetooth/hci_qca.c +0 -0

 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,

@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }

drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864

Quoted 10 lines

drivers/bluetooth/hci_qca.c +9 -0

 
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))

Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.

And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).

drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865

Quoted 1 line

drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);

Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?

drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 869

Quoted 4 lines

drivers/bluetooth/hci_qca.c +4 -0
+
+	return hci_recv_frame(hdev, skb);
+}
+

Regards

Marcel

Balakrishna Godavarthi

author

2018-10-16 14:02 UTC #2 lore raw

Hi Marcel,

Quoted 32 lines · Marcel Holtmann

Hi Balakrishna,
Quoted 30 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 

Quoted 7 lines

drivers/bluetooth/hci_qca.c line 69
Quoted 4 lines
drivers/bluetooth/hci_qca.c +4 -0
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.

[Bala]: will update.

Quoted 23 lines

Quoted 8 lines
drivers/bluetooth/hci_qca.c +0 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, 
 struct sk_buff *skb)
 	return 0;
 }
 
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, · line 866
Quoted 10 lines
drivers/bluetooth/hci_qca.c +9 -0
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff 
 *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.

Quoted 3 lines

And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).

[Bala] : will update condition with _le16()

Quoted 7 lines

drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?

[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.

Quoted 8 lines

drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 869
Quoted 4 lines
drivers/bluetooth/hci_qca.c +4 -0
+
+	return hci_recv_frame(hdev, skb);
+}
+
Regards
Marcel

Signature · 2 lines

Regards
Balakrishna.

Marcel Holtmann

maintainer

2018-10-16 14:35 UTC #3 lore raw

Hi Balakrishna,

Quoted 55 lines

Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 12 lines
Quoted 12 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()

Quoted 7 lines

Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.

please explain that a bit more. There is just one debug handle or do you have more special handles?

Regards

Marcel

Balakrishna Godavarthi

author

2018-10-16 14:56 UTC #4 lore raw

Hi Marcel,

Quoted 65 lines · Marcel Holtmann

Hi Balakrishna,
Quoted 63 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev 
 *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff 
 *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?

Quoted 5 lines

Quoted 1 line
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?

[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt
chip and it is not used for any other connection handlers.
so here we are checking this handler and sending to stack as an
a diagnostic packet.. so during the log capture we will read
this diagnostic packet and handler,to decode the debug packet.
that is the reason why we don't remove handler during condition
check, in future, if qualcomm chip sends diagnostic packet,
then this will create an problem to upper layers to decide
whether it is an diagnostic packet or the debug packet.

this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.

Quoted 3 lines

Regards
Marcel

Signature · 2 lines

Regards
Balakrishna.

Marcel Holtmann

maintainer

2018-10-16 15:09 UTC #5 lore raw

Hi Balakrishna,

Quoted 67 lines

Quoted 52 lines
Quoted 52 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 12 lines
Quoted 12 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt chip and it is not used for any other connection handlers. so here we are checking this handler and sending to stack as an
a diagnostic packet.. so during the log capture we will read this diagnostic packet and handler,to decode the debug packet. that is the reason why we don't remove handler during condition check, in future, if qualcomm chip sends diagnostic packet, then this will create an problem to upper layers to decide whether it is an diagnostic packet or the debug packet.

Quoted 6 lines

this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.

let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.

Regards

Marcel

Balakrishna Godavarthi

author

2018-10-16 15:19 UTC #6 lore raw

Hi Marcel,

Quoted 82 lines · Marcel Holtmann

Hi Balakrishna,
Quoted 73 lines
Quoted 54 lines
Quoted 54 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev 
 *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff 
 *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 865
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt
chip and it is not used for any other connection handlers.
so here we are checking this handler and sending to stack as an
a diagnostic packet.. so during the log capture we will read
this diagnostic packet and handler,to decode the debug packet.
that is the reason why we don't remove handler during
condition check, in future, if qualcomm chip sends diagnostic packet,
then this will create an problem to upper layers to decide
whether it is an diagnostic packet or the debug packet.
Quoted 6 lines
this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.

Quoted 4 lines

let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.

No, 0x2edc is an dedicated handle for debug logs, i don't really think they we will have an new handle coming up. this is only the handle we will send via hci_recv_diag channel to the stack.

what i mean in my previous answer was,we don't to remove debug connection handle from the skb, because in the stack we look for a diagnostic packet and debug connection handler to decode the message.

Quoted 3 lines

Regards
Marcel

Signature · 2 lines

Regards
Balakrishna.

Marcel Holtmann

maintainer

2018-10-16 15:31 UTC #7 lore raw

Hi Balakrishna,

Quoted 75 lines

Quoted 71 lines
Quoted 60 lines
Quoted 52 lines
Quoted 52 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 12 lines
Quoted 12 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 863
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
Quoted 11 lines
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt chip and it is not used for any other connection handlers. so here we are checking this handler and sending to stack as an
a diagnostic packet.. so during the log capture we will read this diagnostic packet and handler,to decode the debug packet. that is the reason why we don't remove handler during condition check, in future, if qualcomm chip sends diagnostic packet, then this will create an problem to upper layers to decide whether it is an diagnostic packet or the debug packet.
this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.
Quoted 3 lines
let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.

Quoted 5 lines

No, 0x2edc is an dedicated handle for debug logs, i don't really think they we will have an new handle coming up. this is only the handle we will send via hci_recv_diag channel to the stack.
what i mean in my previous answer was,we don't to remove debug connection handle from the skb, because in the stack we look for a diagnostic packet and debug connection handler to decode the message.

if 0x2edc is the only magic connection handle, then lets strip the SKB of the ACL packet header and send only the payload to via hci_recv_diag. All the information is present and you would be just sending the ACL packet header for no reason. The diagnostic packets get a separate channel on the monitor interface.

Regards

Marcel

Balakrishna Godavarthi

author

2018-10-16 15:43 UTC #8 lore raw

Hi Marcel,

Quoted 87 lines · Marcel Holtmann

Hi Balakrishna,
Quoted 85 lines
Quoted 78 lines
Quoted 62 lines
Quoted 54 lines
Quoted 54 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev 
 *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct 
 sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev · line 865
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev · line 866
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
Quoted 16 lines
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt
chip and it is not used for any other connection handlers.
so here we are checking this handler and sending to stack as
an
a diagnostic packet.. so during the log capture we will read
this diagnostic packet and handler,to decode the debug packet.
that is the reason why we don't remove handler during
condition check, in future, if qualcomm chip sends diagnostic packet,
then this will create an problem to upper layers to decide
whether it is an diagnostic packet or the debug packet.
this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.
Quoted 3 lines
let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.
No, 0x2edc is an dedicated handle for debug logs, i don't really think they we will have an new handle coming up. this is only the handle we will send via hci_recv_diag channel to the stack.

Quoted 9 lines

Quoted 2 lines
what i mean in my previous answer was,we don't to remove debug connection handle from the skb, because in the stack we look for a diagnostic packet and debug connection handler to decode the message.
if 0x2edc is the only magic connection handle, then lets strip the SKB of the ACL packet header and send only the payload to via hci_recv_diag. All the information is present and you would be just sending the ACL packet header for no reason. The diagnostic packets get a separate channel on the monitor interface.

[Bala]: even we the felt the same to remove ACL packet header before sending to stack via an diagnostic channel,

here is the case, let us assume that an in future qaulcomm bt
chip want to send diagnosis data as a diagnostic packet.
if we remove ACL header for ACL debug packet, how can the stack
differentiate between debug packet and diagnosis packet.

Quoted 3 lines

Regards
Marcel

Signature · 2 lines

Regards
Balakrishna.

Marcel Holtmann

maintainer

2018-10-18 07:58 UTC #9 lore raw

Hi Balakrishna,

Quoted 83 lines

Quoted 78 lines
Quoted 71 lines
Quoted 71 lines
Quoted 71 lines
Quoted 52 lines
Quoted 52 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 12 lines
Quoted 12 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 863
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb) · line 864
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt chip and it is not used for any other connection handlers. so here we are checking this handler and sending to stack as an
a diagnostic packet.. so during the log capture we will read this diagnostic packet and handler,to decode the debug packet. that is the reason why we don't remove handler during condition check, in future, if qualcomm chip sends diagnostic packet, then this will create an problem to upper layers to decide whether it is an diagnostic packet or the debug packet.
this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.
Quoted 7 lines
Quoted 3 lines
let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.
No, 0x2edc is an dedicated handle for debug logs, i don't really think they we will have an new handle coming up. this is only the handle we will send via hci_recv_diag channel to the stack.
what i mean in my previous answer was,we don't to remove debug connection handle from the skb, because in the stack we look for a diagnostic packet and debug connection handler to decode the message.
Quoted 5 lines
if 0x2edc is the only magic connection handle, then lets strip the SKB of the ACL packet header and send only the payload to via hci_recv_diag. All the information is present and you would be just sending the ACL packet header for no reason. The diagnostic packets get a separate channel on the monitor interface.

Quoted 4 lines

[Bala]: even we the felt the same to remove ACL packet header before sending to stack via an diagnostic channel,
here is the case, let us assume that an in future qaulcomm bt chip want to send diagnosis data as a diagnostic packet. if we remove ACL header for ACL debug packet, how can the stack differentiate between debug packet and diagnosis packet.

fair enough. Can we also do the same for the other Qualcomm / Atheros devices on USB and SMD bus? And what about implementing hdev->set_diag to enable / disable sending of debug logs. And of course the addition to btmon to decode the diagnostic channel.

Regards

Marcel

Balakrishna Godavarthi

author

2018-10-24 14:55 UTC #10 lore raw

Hi Balakrishna,

Quoted 99 lines · Marcel Holtmann

Hi Balakrishna,
Quoted 90 lines
Quoted 85 lines
Quoted 78 lines
Quoted 78 lines
Quoted 78 lines
Quoted 54 lines
Quoted 54 lines
Quoted 22 lines
Quoted 22 lines
This patch will prevent error messages splashing on console.
[   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
[   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
QCA wcn3990 will send the debug logs in the form of ACL packets. While decoding packet in qca_recv(), marking the received debug log packet as diagnostic packet.
Signed-off-by Harish Bandi <[email protected]>
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v2:
* Addressed reviewer comments.
v1:
* initial patch
---
drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
drivers/bluetooth/hci_qca.c +0 -0
index d98ed0442201..63ac3c6b4149 100644
Quoted 8 lines
drivers/bluetooth/hci_qca.c line 68
Quoted 7 lines
drivers/bluetooth/hci_qca.c +4 -0
@@ -63,6 +63,10 @@
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
+/* Controller debug log header */
+#define QCA_DEBUG_HDR_MSB	0xDC
+#define QCA_DEBUG_HDR_LSB	0x2E
+
since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
[Bala]: will update.
Quoted 14 lines
Quoted 14 lines
drivers/bluetooth/hci_qca.c +6 -0
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct 
 hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
+static int qca_recv_acl_data(struct hci_dev *hdev, struct 
 sk_buff *skb)
+{
+	/* We receive debug logs from chip as an ACL packets.
+	 * Instead of sending the data to ACL to decode the
+	 * received data, we are pushing them to the above layers
+	 * as a diagnostic packet.
Quoted 8 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct · line 865
Quoted 3 lines
drivers/bluetooth/hci_qca.c +3 -0
+	 */
+	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
+	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
Skip the individual () since they are not needed. Also the skb->len check is not needed since the H4_RECV_ACL already ensures the proper length of the header.
And just use get_unaligned_le16(skb->data) here (or be16 if that is the byte order).
[Bala] : will update condition with _le16()
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/bluetooth/hci_qca.c static int qca_ibs_wake_ack(struct · line 866
Quoted 1 line
drivers/bluetooth/hci_qca.c +1 -0
+		return hci_recv_diag(hdev, skb);
Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or would it be better to be stripped off. Mainly asking are they any other magic handles that we might want to feed through the diag channel?
[Bala]: yes we need header in the stack, to differentiate between actual diagnostic packet and debug packet.
please explain that a bit more. There is just one debug handle or do you have more special handles?
[Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
this handler is dedicated for the debug logs sent by qcomm bt
chip and it is not used for any other connection handlers.
so here we are checking this handler and sending to stack as
an
a diagnostic packet.. so during the log capture we will read
this diagnostic packet and handler,to decode the debug packet.
that is the reason why we don't remove handler during
condition check, in future, if qualcomm chip sends diagnostic packet,
then this will create an problem to upper layers to decide
whether it is an diagnostic packet or the debug packet.
this is how we do it.
if( diagnostic packet && handler == 0x2edc)
treat them as an ddebug packet
else
treat as diagnostic packet.
Quoted 7 lines
Quoted 3 lines
let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.
No, 0x2edc is an dedicated handle for debug logs, i don't really think they we will have an new handle coming up. this is only the handle we will send via hci_recv_diag channel to the stack.
what i mean in my previous answer was,we don't to remove debug connection handle from the skb, because in the stack we look for a diagnostic packet and debug connection handler to decode the message.
Quoted 5 lines
if 0x2edc is the only magic connection handle, then lets strip the SKB of the ACL packet header and send only the payload to via hci_recv_diag. All the information is present and you would be just sending the ACL packet header for no reason. The diagnostic packets get a separate channel on the monitor interface.
Quoted 6 lines
[Bala]: even we the felt the same to remove ACL packet header before sending to stack via an diagnostic channel,
here is the case, let us assume that an in future qaulcomm bt
chip want to send diagnosis data as a diagnostic packet.
if we remove ACL header for ACL debug packet, how can the stack
differentiate between debug packet and diagnosis packet.

Quoted 5 lines

fair enough. Can we also do the same for the other Qualcomm / Atheros devices on USB and SMD bus? And what about implementing hdev->set_diag to enable / disable sending of debug logs. And of course the addition to btmon to decode the diagnostic channel.

[Bala]:
Does thing change need to be done for other Qualcomm/Atheros
device on USB and smd: For the legacy Qualcomm/Atheros Chips (Rome/AR30x2/AR30x3), they don’t have this ACL data debug log feature.

       hdev->set_diag implementation to enable or disable the debug logs

from controller: will study further and get back to you on this
Qualcomm specific file in Bluez stack to decode the bypassed
debug packets in btmon: soon we will be posting the parsing file.

Quoted 3 lines

Regards
Marcel

Signature · 2 lines

Regards
Balakrishna.