[PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[Balakrishna Godavarthi]

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);
+	}
+
 	spin_lock_irqsave(&qca->hci_ibs_lock, flags);
 
 	/* Don't go to sleep in middle of patch download or
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=531819

---Test result---

Test Summary:
CheckPatch                    PASS      0.43 seconds
GitLint                       PASS      0.10 seconds
BuildKernel                   PASS      505.70 seconds
TestRunner: Setup             PASS      334.42 seconds
TestRunner: l2cap-tester      PASS      2.53 seconds
TestRunner: bnep-tester       PASS      1.88 seconds
TestRunner: mgmt-tester       PASS      30.51 seconds
TestRunner: rfcomm-tester     PASS      2.07 seconds
TestRunner: sco-tester        PASS      2.01 seconds
TestRunner: smp-tester        FAIL      2.05 seconds
TestRunner: userchan-tester   PASS      1.92 seconds

Details
##############################
Test: CheckPatch - PASS - 0.43 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - PASS - 0.10 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 505.70 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 334.42 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.53 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.88 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 30.51 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.07 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.01 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.05 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.021 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.92 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44386 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3593 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616863 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11712 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9948 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5490 bytes --]

Re: [PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[Marcel Holtmann]

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.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[bgodavar]

Hi Marcel,

On 2021-08-16 21:37, Marcel Holtmann wrote:
> 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.

> Regards
> 
> Marcel

Re: [PATCH] Bluetooth: hci_qca: Set SSR triggered flags when SSR command is sent out

[Marcel Holtmann]

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