Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR
 command is sent out
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <086f2add931ff541c8a6349767ae2adc@codeaurora.org>
Date:   Thu, 19 Aug 2021 17:03:09 +0200
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        Matthias Kaehlcke <mka@chromium.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:BLUETOOTH SUBSYSTEM" <linux-bluetooth@vger.kernel.org>,
        Hemantg <hemantg@codeaurora.org>,
        MSM <linux-arm-msm@vger.kernel.org>, pharish@codeaurora.org,
        Rocky Liao <rjliao@codeaurora.org>, hbandi@codeaurora.org,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        mcchou@chromium.org
Content-Transfer-Encoding: 8BIT
Message-Id: <6238C7AB-32D7-4D12-A14E-24D12A862405@holtmann.org>
References: <1629091302-7893-1-git-send-email-bgodavar@codeaurora.org>
 <1CE27E9C-EABD-4B25-B255-8925297D11BD@holtmann.org>
 <086f2add931ff541c8a6349767ae2adc@codeaurora.org>
To:     Balakrishna Godavarthi <bgodavar@codeaurora.org>
Precedence: bulk

Hi Balakrishna,

>>> This change sets SSR triggered flags when QCA SSR command is sent to
>>> SoC. After the SSR command sent, driver discards the incoming data from
>>> the upper layers. This way will ensure to read full dumps from the
>>> BT SoC without any flow control issues due to excess of data receiving
>>> from the HOST in audio usecases.
>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>> ---
>>> drivers/bluetooth/hci_qca.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index 53deea2..5cbed6a 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -69,6 +69,8 @@
>>> #define QCA_LAST_SEQUENCE_NUM		0xFFFF
>>> #define QCA_CRASHBYTE_PACKET_LEN	1096
>>> #define QCA_MEMDUMP_BYTE		0xFB
>>> +#define QCA_SSR_OPCODE			0xFC0C
>>> +#define QCA_SSR_PKT_LEN		5
>>> enum qca_flags {
>>> 	QCA_IBS_DISABLED,
>>> @@ -871,6 +873,14 @@ static int qca_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>>> 	/* Prepend skb with frame type */
>>> 	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
>>> +	if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT &&
>>> +	    skb->len == QCA_SSR_PKT_LEN &&
>>> +	    hci_skb_opcode(skb) == QCA_SSR_OPCODE) {
>>> +		bt_dev_info(hu->hdev, "Triggering ssr");
>>> +		set_bit(QCA_SSR_TRIGGERED, &qca->flags);
>>> +		set_bit(QCA_MEMDUMP_COLLECTION, &qca->flags);
>>> +	}
>>> +
>> can we please stop hacking around by parsing opcodes in an enqueue
>> function. Sounds like someone is injecting raw HCI vendor commands and
>> then having a driver react to it.
> [Bala]: yes this opcode is injected via hcitool to test BT SoC dump procedure or
> to collect the dumps to debug the issue during issue cases. When audio usecases are running,
> HOST sends ACL packets to SoC, in meantime if this command is sent to SoC using hcitool
> to collect dumps at particular point,  With out this check HOST is pumping continues data to
> SoC and SoC RFR line goes high, sometimes SoC become unresponsive and driver starts logging
> command timeout error. Instead here, once a cmd with this opcode is sent, timer is started
> to ensure that SSR is in progress. If no response from SoC for 8 seconds. Driver will be restarted.

so why would I add a kernel work-around for this?

Design a proper interface for this and don’t rely on injecting HCI commands via HCI raw channel.

Regards

Marcel