From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.5 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F064C67863 for ; Wed, 24 Oct 2018 14:56:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0A6D32082F for ; Wed, 24 Oct 2018 14:56:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="JmAUZPze"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="JmAUZPze" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0A6D32082F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-bluetooth-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726850AbeJXXYU (ORCPT ); Wed, 24 Oct 2018 19:24:20 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:51144 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726457AbeJXXYU (ORCPT ); Wed, 24 Oct 2018 19:24:20 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 90F4560C5F; Wed, 24 Oct 2018 14:55:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1540392953; bh=5qxJ1ECucaLRVbkAyvJ/6cvvyESt93lQXolXbKd1ddk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=JmAUZPzepi0UZdMgs10RmQbJPvXGDSfbvaPHcV0RRs1Pjt+7UMCQGiIEs5ZEq+Pnc hwnEtFCI+L6+cYa7PpdRFO98/AzRjeKLScOIDQHJZr+7nO2TMjDhZHW8BfsALtWLPX /GGyZxWtg5XLWMk0hygPn7GVKcwBHFT0MEFzNyBE= Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 060DF60246; Wed, 24 Oct 2018 14:55:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1540392953; bh=5qxJ1ECucaLRVbkAyvJ/6cvvyESt93lQXolXbKd1ddk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=JmAUZPzepi0UZdMgs10RmQbJPvXGDSfbvaPHcV0RRs1Pjt+7UMCQGiIEs5ZEq+Pnc hwnEtFCI+L6+cYa7PpdRFO98/AzRjeKLScOIDQHJZr+7nO2TMjDhZHW8BfsALtWLPX /GGyZxWtg5XLWMk0hygPn7GVKcwBHFT0MEFzNyBE= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 24 Oct 2018 20:25:52 +0530 From: Balakrishna Godavarthi To: Marcel Holtmann Cc: Johan Hedberg , Matthias Kaehlcke , Linux Kernel Mailing List , linux-bluetooth@vger.kernel.org, hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org, Harish Bandi Subject: Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs. In-Reply-To: References: <20181003151144.10537-1-bgodavar@codeaurora.org> <2206464E-FA01-4A5A-8BC4-5CF70A0B9765@holtmann.org> <01616379-E0FF-49AC-8C69-DA498DA9F51F@holtmann.org> <540262ec6c6f1c5f3a9baf18b6f40d47@codeaurora.org> <9261133E-226E-42E7-B2F6-15489CCC7CF1@holtmann.org> Message-ID: <9cd7bbd5c6d620399f4562ec4f406725@codeaurora.org> X-Sender: bgodavar@codeaurora.org User-Agent: Roundcube Webmail/1.2.5 Sender: linux-bluetooth-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org 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 >>>>>>>>>> Signed-off-by: Balakrishna Godavarthi >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> 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.