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

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

Hi Marcel,

On 2018-10-16 20:39, 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.

> Regards
> 
> Marcel

-- 
Regards
Balakrishna.