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?
>>>>> [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.
>>>> 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.
>> 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.

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