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

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Balakrishna,

On 2018-10-18 13:28, Marcel Holtmann wrote:
> 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@codeaurora.org>
>>>>>>>>>> Signed-off-by: Balakrishna Godavarthi 
>>>>>>>>>> <bgodavar@codeaurora.org>
>>>>>>>>>> ---
>>>>>>>>>> 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.
> 
[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.


> Regards
> 
> Marcel

-- 
Regards
Balakrishna.