Messages in this thread | | | Date | Wed, 24 Oct 2018 20:25:52 +0530 | From | Balakrishna Godavarthi <> | Subject | Re: [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.
| |