[PATCH 0/3] Fix SSR issues caused by BT_EN being pulled up by hardware

[PATCH 0/3] Fix SSR issues caused by BT_EN being pulled up by hardware

[Shuai Zhang]

This patch series addresses issues encountered during SSR when
the BT_EN pin is pulled up by hardware. The main issues fixed are:

1. Timeout when sending reset command.
2. IBS state of host and controller not being synchronized.
3. Multiple triggers of SSR generating only one coredump file.

Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>

Shuai Zhang (3):
  driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw
  driver: bluetooth: hci_qca: fix host IBS state after SSR
  driver: bluetooth: hci_qca: Multiple triggers of SSR only generate one
    coredump file

 drivers/bluetooth/hci_qca.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.34.1

[PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Shuai Zhang]

the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
This leads to reset command timeout.

Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
---
 drivers/bluetooth/hci_qca.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 4e56782b0..791f8d472 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
 		skb_queue_purge(&qca->rx_memdump_q);
 	}
 
+	/* If the SoC always enables the bt_en pin via hardware and the driver
+	 * cannot control the bt_en pin of the SoC chip, then during SSR,
+	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
+	 * This leads to a reset command timeout failure.
+	 * Also, add msleep delay to wait for controller to complete SSR.
+	 */
+	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
+		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
+		clear_bit(QCA_IBS_DISABLED, &qca->flags);
+		msleep(50);
+	}
+
 	clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
 }
 
-- 
2.34.1

[PATCH 2/3] driver: bluetooth: hci_qca: fix host IBS state after SSR

[Shuai Zhang]

After SSR, host will not download the firmware, causing
controller to remain in the IBS_WAKE state. Host needs
to synchronize with the controller to maintain proper operation.

Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
---
 drivers/bluetooth/hci_qca.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 791f8d472..a17d3f7ae 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1658,10 +1658,14 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
 	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
 	 * This leads to a reset command timeout failure.
 	 * Also, add msleep delay to wait for controller to complete SSR.
+	 *
+	 * Host will not download the firmware after SSR, controller to remain
+	 * in the IBS_WAKE state, and the host needs to synchronize with it
 	 */
 	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
 		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
 		clear_bit(QCA_IBS_DISABLED, &qca->flags);
+		qca->tx_ibs_state = HCI_IBS_TX_AWAKE;
 		msleep(50);
 	}
 
-- 
2.34.1

[PATCH 3/3] driver: bluetooth: hci_qca: Multiple triggers of SSR only generate one coredump file

[Shuai Zhang]

Multiple triggers of SSR only first generate coredump file,
duo to memcoredump_flag no clear.

add clear coredump flag when ssr completed.

Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
---
 drivers/bluetooth/hci_qca.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index a17d3f7ae..e836b2c29 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1661,11 +1661,14 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
 	 *
 	 * Host will not download the firmware after SSR, controller to remain
 	 * in the IBS_WAKE state, and the host needs to synchronize with it
+	 *
+	 * clear memcoredump_flag to ensure next submission of coredump date.
 	 */
 	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
 		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
 		clear_bit(QCA_IBS_DISABLED, &qca->flags);
 		qca->tx_ibs_state = HCI_IBS_TX_AWAKE;
+		qca->memdump_state = QCA_MEMDUMP_IDLE;
 		msleep(50);
 	}
 
-- 
2.34.1

RE: Fix SSR issues caused by BT_EN being pulled up by hardware

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 7679 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=982342

---Test result---

Test Summary:
CheckPatch                    PENDING   0.41 seconds
GitLint                       PENDING   0.38 seconds
SubjectPrefix                 FAIL      0.62 seconds
BuildKernel                   FAIL      18.51 seconds
CheckAllWarning               FAIL      20.49 seconds
CheckSparse                   FAIL      22.58 seconds
BuildKernel32                 FAIL      18.42 seconds
TestRunnerSetup               PASS      467.03 seconds
TestRunner_l2cap-tester       PASS      24.61 seconds
TestRunner_iso-tester         PASS      39.96 seconds
TestRunner_bnep-tester        PASS      5.84 seconds
TestRunner_mgmt-tester        FAIL      130.67 seconds
TestRunner_rfcomm-tester      PASS      9.27 seconds
TestRunner_sco-tester         PASS      14.93 seconds
TestRunner_ioctl-tester       PASS      9.92 seconds
TestRunner_mesh-tester        FAIL      11.38 seconds
TestRunner_smp-tester         PASS      8.49 seconds
TestRunner_userchan-tester    PASS      6.11 seconds
IncrementalBuild              PENDING   0.90 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: BuildKernel - FAIL
Desc: Build Kernel for Bluetooth
Output:

drivers/bluetooth/hci_ll.c: In function ‘ll_setup’:
drivers/bluetooth/hci_ll.c:652:46: error: ‘struct hci_dev’ has no member named ‘quirks’
  652 |   set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                              ^~
drivers/bluetooth/hci_ll.c:656:47: error: ‘struct hci_dev’ has no member named ‘quirks’
  656 |    set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                               ^~
make[4]: *** [scripts/Makefile.build:203: drivers/bluetooth/hci_ll.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:461: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:461: drivers] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/github/workspace/src/src/Makefile:2003: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
##############################
Test: CheckAllWarning - FAIL
Desc: Run linux kernel with all warning enabled
Output:

drivers/bluetooth/hci_ll.c: In function ‘ll_setup’:
drivers/bluetooth/hci_ll.c:652:46: error: ‘struct hci_dev’ has no member named ‘quirks’
  652 |   set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                              ^~
drivers/bluetooth/hci_ll.c:656:47: error: ‘struct hci_dev’ has no member named ‘quirks’
  656 |    set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                               ^~
make[4]: *** [scripts/Makefile.build:203: drivers/bluetooth/hci_ll.o] Error 1
make[3]: *** [scripts/Makefile.build:461: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:461: drivers] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/github/workspace/src/src/Makefile:2003: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
##############################
Test: CheckSparse - FAIL
Desc: Run sparse tool with linux kernel
Output:

net/bluetooth/af_bluetooth.c:248:25: warning: context imbalance in 'bt_accept_enqueue' - different lock contexts for basic block
drivers/bluetooth/hci_ll.c: In function ‘ll_setup’:
drivers/bluetooth/hci_ll.c:652:46: error: ‘struct hci_dev’ has no member named ‘quirks’
  652 |   set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                              ^~
drivers/bluetooth/hci_ll.c:656:47: error: ‘struct hci_dev’ has no member named ‘quirks’
  656 |    set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                               ^~
make[4]: *** [scripts/Makefile.build:203: drivers/bluetooth/hci_ll.o] Error 1
make[3]: *** [scripts/Makefile.build:461: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:461: drivers] Error 2
make[2]: *** Waiting for unfinished jobs....
net/bluetooth/hci_core.c:85:9: warning: context imbalance in '__hci_dev_get' - different lock contexts for basic block
net/bluetooth/hci_core.c: note: in included file (through include/linux/notifier.h, include/linux/memory_hotplug.h, include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, include/linux/radix-tree.h, ...):
./include/linux/srcu.h:400:9: warning: context imbalance in 'hci_dev_put_srcu' - unexpected unlock
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
./include/net/bluetooth/hci.h:2655:47: warning: array of flexible structures
./include/net/bluetooth/hci.h:2741:43: warning: array of flexible structures
net/bluetooth/hci_codec.c: note: in included file:
./include/net/bluetooth/hci_core.h:149:35: warning: array of flexible structures
net/bluetooth/sco.c: note: in included file:
./include/net/bluetooth/hci_core.h:149:35: warning: array of flexible structures
make[1]: *** [/github/workspace/src/src/Makefile:2003: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
##############################
Test: BuildKernel32 - FAIL
Desc: Build 32bit Kernel for Bluetooth
Output:

drivers/bluetooth/hci_ll.c: In function ‘ll_setup’:
drivers/bluetooth/hci_ll.c:652:46: error: ‘struct hci_dev’ has no member named ‘quirks’
  652 |   set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                              ^~
drivers/bluetooth/hci_ll.c:656:47: error: ‘struct hci_dev’ has no member named ‘quirks’
  656 |    set_bit(HCI_QUIRK_INVALID_BDADDR, &hu->hdev->quirks);
      |                                               ^~
make[4]: *** [scripts/Makefile.build:203: drivers/bluetooth/hci_ll.o] Error 1
make[3]: *** [scripts/Makefile.build:461: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:461: drivers] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/github/workspace/src/src/Makefile:2003: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 2 (2 Devices to AL)          Failed       0.189 seconds
LL Privacy - Add Device 3 (AL is full)               Failed       0.209 seconds
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.169 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.123 seconds
Mesh - Send cancel - 2                               Timed out    1.997 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Konrad Dybcio]

On 7/15/25 7:16 AM, Shuai Zhang wrote:
> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
> This leads to reset command timeout.

This is a description of what goes wrong in terms of the code of
this driver, and it doesn't explain why you gate the code addition
with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
what you're doing, and more importantly, why.

> 
> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
> ---
>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 4e56782b0..791f8d472 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>  		skb_queue_purge(&qca->rx_memdump_q);
>  	}
>  
> +	/* If the SoC always enables the bt_en pin via hardware and the driver
> +	 * cannot control the bt_en pin of the SoC chip, then during SSR,

What is the "SoC" here? Bluetooth chip? MSM?

What does "enabling the pin via hardware" refer to? Do we ever expect
that a proper platform description skips the bt_en pin?

Also:

/*
 * If the..

> +	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
> +	 * This leads to a reset command timeout failure.
> +	 * Also, add msleep delay to wait for controller to complete SSR.

Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
you to explain that acronym.. it's used a number of times across the
driver, so perhaps a comment somewhere at the top in a separate commit
would be good as well. I'm guessing "subsystem reset"?

Konrad

> +	 */
> +	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> +		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
> +		clear_bit(QCA_IBS_DISABLED, &qca->flags);
> +		msleep(50);
> +	}
> +
>  	clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
>  }
>  

Re: [PATCH 2/3] driver: bluetooth: hci_qca: fix host IBS state after SSR

[Konrad Dybcio]

On 7/15/25 7:16 AM, Shuai Zhang wrote:
> After SSR, host will not download the firmware, causing
> controller to remain in the IBS_WAKE state. Host needs
> to synchronize with the controller to maintain proper operation.
> 
> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
> ---
>  drivers/bluetooth/hci_qca.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 791f8d472..a17d3f7ae 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1658,10 +1658,14 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>  	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>  	 * This leads to a reset command timeout failure.
>  	 * Also, add msleep delay to wait for controller to complete SSR.
> +	 *
> +	 * Host will not download the firmware after SSR, controller to remain
> +	 * in the IBS_WAKE state, and the host needs to synchronize with it
>  	 */
>  	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>  		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
> +		qca->tx_ibs_state = HCI_IBS_TX_AWAKE;
>  		msleep(50);

This touches upon the code introduced in the previous patch.

Any reason they should be separate?

Konrad

Re: [PATCH 3/3] driver: bluetooth: hci_qca: Multiple triggers of SSR only generate one coredump file

[Konrad Dybcio]

On 7/15/25 7:16 AM, Shuai Zhang wrote:
> Multiple triggers of SSR only first generate coredump file,
> duo to memcoredump_flag no clear.
> 
> add clear coredump flag when ssr completed.
> 
> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
> ---
>  drivers/bluetooth/hci_qca.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index a17d3f7ae..e836b2c29 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1661,11 +1661,14 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>  	 *
>  	 * Host will not download the firmware after SSR, controller to remain
>  	 * in the IBS_WAKE state, and the host needs to synchronize with it
> +	 *
> +	 * clear memcoredump_flag to ensure next submission of coredump date.
>  	 */
>  	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>  		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>  		qca->tx_ibs_state = HCI_IBS_TX_AWAKE;
> +		qca->memdump_state = QCA_MEMDUMP_IDLE;
>  		msleep(50);

Same comment as patch 2

Konrad

Re: [PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Shuai Zhang]

Hi Konrad 

On 7/15/2025 5:11 PM, Konrad Dybcio wrote:
> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>> This leads to reset command timeout.
> 
> This is a description of what goes wrong in terms of the code of
> this driver, and it doesn't explain why you gate the code addition
> with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
> what you're doing, and more importantly, why.
> 

The problem encountered is that when the host actively triggers ssr 
and collects the coredump data, the bt will send a reset command to 
the controller. However, due to the aforementioned flag not being set, 
the reset command times out.

I'm not clear whether you want to ask about the function of 
HCI_QUIRK_NON_PERSISTENT_SETUP or why the changes are placed 
under if(!HCI_QUIRK_NON_PERSISTENT_SETUP).

Regarding the purpose of HCI_QUIRK_NON_PERSISTENT_SETUP, 
you can refer to this commit. 740011cfe94859df8d05f5400d589a8693b095e7

As for why it's placed in if(!HCI_QUIRK_NON_PERSISTENT_SETUP), 
since HCI_QUIRK_NON_PERSISTENT_SETUP is related to BT_EN, it can be 
used to determine if BT_EN exists in the DTS.

>>
>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>> ---
>>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 4e56782b0..791f8d472 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>  		skb_queue_purge(&qca->rx_memdump_q);
>>  	}
>>  
>> +	/* If the SoC always enables the bt_en pin via hardware and the driver
>> +	 * cannot control the bt_en pin of the SoC chip, then during SSR,
> 
> What is the "SoC" here? Bluetooth chip? MSM?

yes, Bluetooth chip on qcs9075-evk platform

> 
> What does "enabling the pin via hardware" refer to? Do we ever expect
> that a proper platform description skips the bt_en pin?
> 
> Also:
> 
> /*
>  * If the..
> 

Sorry, I’m not quite sure I follow—could you clarify what you meant?
Here is my understanding.

Enabling pins through hardware refers to "the pin is  pulled up by hardware".
qcs9075-evk platform use the m.2 connective card, the bt_en always pull up.


>> +	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>> +	 * This leads to a reset command timeout failure.
>> +	 * Also, add msleep delay to wait for controller to complete SSR.
> 
> Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
> you to explain that acronym.. it's used a number of times across the
> driver, so perhaps a comment somewhere at the top in a separate commit
> would be good as well. I'm guessing "subsystem reset"?

Just to clarify, SSR is short for Subsystem Restart

> 
> Konrad
> 
>> +	 */
>> +	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>> +		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>> +		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>> +		msleep(50);
>> +	}
>> +
>>  	clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
>>  }
>>  

Shuai

Re: [PATCH 2/3] driver: bluetooth: hci_qca: fix host IBS state after SSR

[Shuai Zhang]

On 7/15/2025 5:12 PM, Konrad Dybcio wrote:
> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>> After SSR, host will not download the firmware, causing
>> controller to remain in the IBS_WAKE state. Host needs
>> to synchronize with the controller to maintain proper operation.
>>
>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>> ---
>>  drivers/bluetooth/hci_qca.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 791f8d472..a17d3f7ae 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1658,10 +1658,14 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>  	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>  	 * This leads to a reset command timeout failure.
>>  	 * Also, add msleep delay to wait for controller to complete SSR.
>> +	 *
>> +	 * Host will not download the firmware after SSR, controller to remain
>> +	 * in the IBS_WAKE state, and the host needs to synchronize with it
>>  	 */
>>  	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>  		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>> +		qca->tx_ibs_state = HCI_IBS_TX_AWAKE;
>>  		msleep(50);
> 
> This touches upon the code introduced in the previous patch.
> 
> Any reason they should be separate?
> 

Since this is a different issue, I separated it. The same reason applies to patch 3/3.

> Konrad

Shuai

Re: [PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Konrad Dybcio]

On 7/19/25 1:32 AM, Shuai Zhang wrote:
> Hi Konrad 
> 
> On 7/15/2025 5:11 PM, Konrad Dybcio wrote:
>> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>>> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>> This leads to reset command timeout.
>>
>> This is a description of what goes wrong in terms of the code of
>> this driver, and it doesn't explain why you gate the code addition
>> with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
>> what you're doing, and more importantly, why.
>>
> 
> The problem encountered is that when the host actively triggers ssr 
> and collects the coredump data, the bt will send a reset command to 
> the controller. However, due to the aforementioned flag not being set, 
> the reset command times out.
> 
> I'm not clear whether you want to ask about the function of 
> HCI_QUIRK_NON_PERSISTENT_SETUP or why the changes are placed 
> under if(!HCI_QUIRK_NON_PERSISTENT_SETUP).
> 
> Regarding the purpose of HCI_QUIRK_NON_PERSISTENT_SETUP, 
> you can refer to this commit. 740011cfe94859df8d05f5400d589a8693b095e7
> 
> As for why it's placed in if(!HCI_QUIRK_NON_PERSISTENT_SETUP), 
> since HCI_QUIRK_NON_PERSISTENT_SETUP is related to BT_EN, it can be 
> used to determine if BT_EN exists in the DTS.

What I'm saying is that you should put this information in the
commit message

> 
>>>
>>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>>> ---
>>>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index 4e56782b0..791f8d472 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>>  		skb_queue_purge(&qca->rx_memdump_q);
>>>  	}
>>>  
>>> +	/* If the SoC always enables the bt_en pin via hardware and the driver
>>> +	 * cannot control the bt_en pin of the SoC chip, then during SSR,
>>
>> What is the "SoC" here? Bluetooth chip? MSM?
> 
> yes, Bluetooth chip on qcs9075-evk platform
> 
>>
>> What does "enabling the pin via hardware" refer to? Do we ever expect
>> that a proper platform description skips the bt_en pin?
>>
>> Also:
>>
>> /*
>>  * If the..
>>
> 
> Sorry, I’m not quite sure I follow—could you clarify what you meant?
> Here is my understanding.

The comment style.

> 
> Enabling pins through hardware refers to "the pin is  pulled up by hardware".
> qcs9075-evk platform use the m.2 connective card, the bt_en always pull up.

This is not conclusive either. Does the firmware of the bluetooth chip
configure the pin, or is the reset pin connected to an always-on PCIe
supply or similar?

> 
> 
>>> +	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.

At a glance, they're only cleared in qca_setup(), regardless of their state

>>> +	 * This leads to a reset command timeout failure.
>>> +	 * Also, add msleep delay to wait for controller to complete SSR.
>>
>> Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
>> you to explain that acronym.. it's used a number of times across the
>> driver, so perhaps a comment somewhere at the top in a separate commit
>> would be good as well. I'm guessing "subsystem reset"?
> 
> Just to clarify, SSR is short for Subsystem Restart

Please write it down somewhere

Konrad

Re: [PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Shuai Zhang]

Hi Konrad

On 7/19/2025 7:32 AM, Shuai Zhang wrote:
> Hi Konrad 
> 
> On 7/15/2025 5:11 PM, Konrad Dybcio wrote:
>> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>>> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>> This leads to reset command timeout.
>>
>> This is a description of what goes wrong in terms of the code of
>> this driver, and it doesn't explain why you gate the code addition
>> with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
>> what you're doing, and more importantly, why.
>>
> 
> The problem encountered is that when the host actively triggers ssr 
> and collects the coredump data, the bt will send a reset command to 
> the controller. However, due to the aforementioned flag not being set, 
> the reset command times out.
> 
> I'm not clear whether you want to ask about the function of 
> HCI_QUIRK_NON_PERSISTENT_SETUP or why the changes are placed 
> under if(!HCI_QUIRK_NON_PERSISTENT_SETUP).
> 
> Regarding the purpose of HCI_QUIRK_NON_PERSISTENT_SETUP, 
> you can refer to this commit. 740011cfe94859df8d05f5400d589a8693b095e7
> 
> As for why it's placed in if(!HCI_QUIRK_NON_PERSISTENT_SETUP), 
> since HCI_QUIRK_NON_PERSISTENT_SETUP is related to BT_EN, it can be 
> used to determine if BT_EN exists in the DTS.
> 
>>>
>>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>>> ---
>>>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index 4e56782b0..791f8d472 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>>  		skb_queue_purge(&qca->rx_memdump_q);
>>>  	}
>>>  
>>> +	/* If the SoC always enables the bt_en pin via hardware and the driver
>>> +	 * cannot control the bt_en pin of the SoC chip, then during SSR,
>>
>> What is the "SoC" here? Bluetooth chip? MSM?
> 
> yes, Bluetooth chip on qcs9075-evk platform
> 
>>
>> What does "enabling the pin via hardware" refer to? Do we ever expect
>> that a proper platform description skips the bt_en pin?
>>
>> Also:
>>
>> /*
>>  * If the..
>>
> 
> Sorry, I’m not quite sure I follow—could you clarify what you meant?
> Here is my understanding.
> 
> Enabling pins through hardware refers to "the pin is  pulled up by hardware".
> qcs9075-evk platform use the m.2 connective card, the bt_en always pull up.
> 
> 
>>> +	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>> +	 * This leads to a reset command timeout failure.
>>> +	 * Also, add msleep delay to wait for controller to complete SSR.
>>
>> Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
>> you to explain that acronym.. it's used a number of times across the
>> driver, so perhaps a comment somewhere at the top in a separate commit
>> would be good as well. I'm guessing "subsystem reset"?
> 
> Just to clarify, SSR is short for Subsystem Restart
> 
>>
>> Konrad
>>
>>> +	 */
>>> +	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>> +		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>>> +		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>> +		msleep(50);
>>> +	}
>>> +
>>>  	clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
>>>  }
>>>  
> 
> Shuai
> 

Please let me know if there are any updates. Thank you.

BR,
Shuai

Re: [PATCH 1/3] driver: bluetooth: hci_qca: fix ssr fail when BT_EN is pulled up by hw

[Konrad Dybcio]

On 8/12/25 10:03 AM, Shuai Zhang wrote:
> Hi Konrad
> 
> On 7/19/2025 7:32 AM, Shuai Zhang wrote:
>> Hi Konrad 
>>
>> On 7/15/2025 5:11 PM, Konrad Dybcio wrote:
>>> On 7/15/25 7:16 AM, Shuai Zhang wrote:
>>>> the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>>> This leads to reset command timeout.
>>>
>>> This is a description of what goes wrong in terms of the code of
>>> this driver, and it doesn't explain why you gate the code addition
>>> with HCI_QUIRK_NON_PERSISTENT_SETUP, please share more details about
>>> what you're doing, and more importantly, why.
>>>
>>
>> The problem encountered is that when the host actively triggers ssr 
>> and collects the coredump data, the bt will send a reset command to 
>> the controller. However, due to the aforementioned flag not being set, 
>> the reset command times out.
>>
>> I'm not clear whether you want to ask about the function of 
>> HCI_QUIRK_NON_PERSISTENT_SETUP or why the changes are placed 
>> under if(!HCI_QUIRK_NON_PERSISTENT_SETUP).
>>
>> Regarding the purpose of HCI_QUIRK_NON_PERSISTENT_SETUP, 
>> you can refer to this commit. 740011cfe94859df8d05f5400d589a8693b095e7
>>
>> As for why it's placed in if(!HCI_QUIRK_NON_PERSISTENT_SETUP), 
>> since HCI_QUIRK_NON_PERSISTENT_SETUP is related to BT_EN, it can be 
>> used to determine if BT_EN exists in the DTS.
>>
>>>>
>>>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>>>> ---
>>>>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index 4e56782b0..791f8d472 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -1653,6 +1653,18 @@ static void qca_hw_error(struct hci_dev *hdev, u8 code)
>>>>  		skb_queue_purge(&qca->rx_memdump_q);
>>>>  	}
>>>>  
>>>> +	/* If the SoC always enables the bt_en pin via hardware and the driver
>>>> +	 * cannot control the bt_en pin of the SoC chip, then during SSR,
>>>
>>> What is the "SoC" here? Bluetooth chip? MSM?
>>
>> yes, Bluetooth chip on qcs9075-evk platform
>>
>>>
>>> What does "enabling the pin via hardware" refer to? Do we ever expect
>>> that a proper platform description skips the bt_en pin?
>>>
>>> Also:
>>>
>>> /*
>>>  * If the..
>>>
>>
>> Sorry, I’m not quite sure I follow—could you clarify what you meant?
>> Here is my understanding.
>>
>> Enabling pins through hardware refers to "the pin is  pulled up by hardware".
>> qcs9075-evk platform use the m.2 connective card, the bt_en always pull up.
>>
>>
>>>> +	 * the QCA_SSR_TRIGGERED and QCA_IBS_DISABLED bits cannot be cleared.
>>>> +	 * This leads to a reset command timeout failure.
>>>> +	 * Also, add msleep delay to wait for controller to complete SSR.
>>>
>>> Googling "bluetooth SSR" yields nothing, so it's fair for me to ask
>>> you to explain that acronym.. it's used a number of times across the
>>> driver, so perhaps a comment somewhere at the top in a separate commit
>>> would be good as well. I'm guessing "subsystem reset"?
>>
>> Just to clarify, SSR is short for Subsystem Restart
>>
>>>
>>> Konrad
>>>
>>>> +	 */
>>>> +	if (!test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>> +		clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>>>> +		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>>> +		msleep(50);
>>>> +	}
>>>> +
>>>>  	clear_bit(QCA_HW_ERROR_EVENT, &qca->flags);
>>>>  }
>>>>  
>>
>> Shuai
>>
> 
> Please let me know if there are any updates. Thank you.

You're expected to address the review comments in a subsequent patchset
revision, in this case please put the answers to the questions I asked
in the commit message, or in the comments, so that someone else can
make sense of the change

Konrad