Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs.

[Marcel Holtmann]

Hi Balakrishna,

>>> 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 <c-hbandi@xxxxxxxxxxxxxx>
>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
>>> ---
>>> v2:
>>> * Addressed reviewer comments.
>>> v1:
>>> * initial patch
>>> ---
>>> drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
>>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index d98ed0442201..63ac3c6b4149 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -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.
> 
>>> /* 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))
>> 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()
> 
>>> +		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