lkml.org 
[lkml]   [2018]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs.
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.

\
 
 \ /
  Last update: 2018-10-24 16:57    [W:0.045 / U:9.732 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site