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-hba...@codeaurora.org>
>>>>>>>>> Signed-off-by: Balakrishna Godavarthi <bgoda...@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.
Regards
Marcel
