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.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham 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 56BC9C5ACCC for ; Thu, 18 Oct 2018 07:58:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 090DA208E4 for ; Thu, 18 Oct 2018 07:58:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 090DA208E4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=holtmann.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727711AbeJRP6a convert rfc822-to-8bit (ORCPT ); Thu, 18 Oct 2018 11:58:30 -0400 Received: from coyote.holtmann.net ([212.227.132.17]:48318 "EHLO mail.holtmann.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727363AbeJRP63 (ORCPT ); Thu, 18 Oct 2018 11:58:29 -0400 Received: from marcel-macbook.fritz.box (p4FF9F655.dip0.t-ipconnect.de [79.249.246.85]) by mail.holtmann.org (Postfix) with ESMTPSA id 087E9CEF8D; Thu, 18 Oct 2018 10:06:04 +0200 (CEST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs. From: Marcel Holtmann In-Reply-To: Date: Thu, 18 Oct 2018 09:58:41 +0200 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 Content-Transfer-Encoding: 8BIT Message-Id: 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> To: Balakrishna Godavarthi X-Mailer: Apple Mail (2.3445.100.39) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Regards Marcel