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

    Regards

    Marcel

    \
     
     \ /
      Last update: 2018-10-18 09:59    [W:4.534 / U:0.240 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site