lkml.org 
[lkml]   [2018]   [Oct]   [16]   [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 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.

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