[PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[Kiran K]

Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
reset) command. This change fixes the driver to properly wait for the
alive interrupt.

Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
Signed-off-by: Kiran K <kiran.k@intel.com>
Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
---
 drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index 1113a6310bd0..f893ad6fc87a 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct btintel_pcie_data *data)
 	case BTINTEL_PCIE_INTEL_HCI_RESET1:
 		if (btintel_pcie_in_op(data)) {
 			submit_rx = true;
+			signal_waitq = true;
 			break;
 		}
 
 		if (btintel_pcie_in_iml(data)) {
 			submit_rx = true;
+			signal_waitq = true;
 			data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
 			break;
 		}
@@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
 			if (opcode == 0xfc01)
 				btintel_pcie_inject_cmd_complete(hdev, opcode);
 		}
-		/* Firmware raises alive interrupt on HCI_OP_RESET */
-		if (opcode == HCI_OP_RESET)
+
+		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
+		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
 			data->gp0_received = false;
 
 		hdev->stat.cmd_tx++;
@@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
 		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
 			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
 			   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
-		if (opcode == HCI_OP_RESET) {
-			ret = wait_event_timeout(data->gp0_wait_q,
-						 data->gp0_received,
-						 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
-			if (!ret) {
-				hdev->stat.err_tx++;
-				bt_dev_err(hdev, "No alive interrupt received for %s",
-					   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
-				ret = -ETIME;
-				goto exit_error;
-			}
+		ret = wait_event_timeout(data->gp0_wait_q,
+					 data->gp0_received,
+					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
+		if (!ret) {
+			hdev->stat.err_tx++;
+			bt_dev_err(hdev, "No alive interrupt received for %s",
+				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
+			ret = -ETIME;
+			goto exit_error;
 		}
 	}
 	hdev->stat.byte_tx += skb->len;
-- 
2.43.0

[PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[Kiran K]

Firmware raises alive interrpt on sending 0xfc01 command. Alive context
maintained in driver needs to be updated before sending 0xfc01 (Intel
Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition where
the context is also updated in threaded irq.

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
---
 drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index f893ad6fc87a..d29103b102e4 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
 				btintel_pcie_inject_cmd_complete(hdev, opcode);
 		}
 
-		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
-		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
-			data->gp0_received = false;
-
 		hdev->stat.cmd_tx++;
 		break;
 	case HCI_ACLDATA_PKT:
@@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
 	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
 	       BTINTEL_PCIE_HCI_TYPE_LEN);
 
+	if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
+		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
+		if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
+			data->gp0_received = false;
+			old_ctxt = data->alive_intr_ctxt;
+			data->alive_intr_ctxt =
+				(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
+					BTINTEL_PCIE_HCI_RESET);
+			bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive context changed: %s  ->  %s",
+				   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
+				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
+		}
+	}
+
 	ret = btintel_pcie_send_sync(data, skb);
 	if (ret) {
 		hdev->stat.err_tx++;
@@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
 
 	if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
 	    (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
-		old_ctxt = data->alive_intr_ctxt;
-		data->alive_intr_ctxt =
-			(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
-				BTINTEL_PCIE_HCI_RESET);
-		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
-			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
-			   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
 		ret = wait_event_timeout(data->gp0_wait_q,
 					 data->gp0_received,
 					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
-- 
2.43.0

RE: [v1,1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.36 seconds
GitLint                       PENDING   0.23 seconds
SubjectPrefix                 PASS      0.18 seconds
BuildKernel                   PASS      24.10 seconds
CheckAllWarning               PASS      26.41 seconds
CheckSparse                   PASS      29.76 seconds
BuildKernel32                 PASS      23.98 seconds
TestRunnerSetup               PASS      466.76 seconds
TestRunner_l2cap-tester       PASS      24.93 seconds
TestRunner_iso-tester         PASS      33.97 seconds
TestRunner_bnep-tester        PASS      5.93 seconds
TestRunner_mgmt-tester        FAIL      130.72 seconds
TestRunner_rfcomm-tester      PASS      9.22 seconds
TestRunner_sco-tester         PASS      14.60 seconds
TestRunner_ioctl-tester       PASS      9.92 seconds
TestRunner_mesh-tester        FAIL      11.32 seconds
TestRunner_smp-tester         PASS      8.43 seconds
TestRunner_userchan-tester    PASS      6.11 seconds
IncrementalBuild              PENDING   0.70 seconds

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

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

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 482 (98.4%), Failed: 4, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 2 (2 Devices to AL)          Failed       0.194 seconds
LL Privacy - Set Flags 1 (Add to RL)                 Failed       0.160 seconds
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.164 seconds
LL Privacy - Start Discovery 2 (Disable RL)          Failed       0.200 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.170 seconds
Mesh - Send cancel - 2                               Timed out    2.004 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[Paul Menzel]

Dear Sai, dear Kiran,


Thank you for your patch.

Am 07.07.25 um 05:46 schrieb Kiran K:
> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
> reset) command. This change fixes the driver to properly wait for the
> alive interrupt.

What is the consequence of not waiting?

> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
> ---
>   drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 1113a6310bd0..f893ad6fc87a 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct btintel_pcie_data *data)
>   	case BTINTEL_PCIE_INTEL_HCI_RESET1:
>   		if (btintel_pcie_in_op(data)) {
>   			submit_rx = true;
> +			signal_waitq = true;
>   			break;
>   		}
>   
>   		if (btintel_pcie_in_iml(data)) {
>   			submit_rx = true;
> +			signal_waitq = true;
>   			data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>   			break;
>   		}
> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>   			if (opcode == 0xfc01)
>   				btintel_pcie_inject_cmd_complete(hdev, opcode);
>   		}
> -		/* Firmware raises alive interrupt on HCI_OP_RESET */
> -		if (opcode == HCI_OP_RESET)
> +
> +		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/

A space is missing before */.

> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01)

Please define a macro for the magic number.

>   			data->gp0_received = false;
>   
>   		hdev->stat.cmd_tx++;
> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>   		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
>   			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>   			   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> -		if (opcode == HCI_OP_RESET) {
> -			ret = wait_event_timeout(data->gp0_wait_q,
> -						 data->gp0_received,
> -						 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> -			if (!ret) {
> -				hdev->stat.err_tx++;
> -				bt_dev_err(hdev, "No alive interrupt received for %s",
> -					   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> -				ret = -ETIME;
> -				goto exit_error;
> -			}
> +		ret = wait_event_timeout(data->gp0_wait_q,
> +					 data->gp0_received,
> +					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> +		if (!ret) {
> +			hdev->stat.err_tx++;
> +			bt_dev_err(hdev, "No alive interrupt received for %s",
> +				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));

In a follow-up patch, the log message could be improved by also adding 
the timeout value to it.

> +			ret = -ETIME;
> +			goto exit_error;
>   		}
>   	}
>   	hdev->stat.byte_tx += skb->len;


Kind regards,

Paul

Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[Paul Menzel]

Dear Kiran, dear Sai,


Thank you for the patch.

Am 07.07.25 um 05:46 schrieb Kiran K:
> Firmware raises alive interrpt on sending 0xfc01 command. Alive context

interr*u*pt

(I would have hoped, two developers would spot such a typo, a spell 
checker also highlights.)

> maintained in driver needs to be updated before sending 0xfc01 (Intel
> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition where
> the context is also updated in threaded irq.

Do you have a reproducer for the issue?

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")

I’d add the Fixes tag before the Signed-off-by lines.

> ---
>   drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index f893ad6fc87a..d29103b102e4 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>   				btintel_pcie_inject_cmd_complete(hdev, opcode);
>   		}
>   
> -		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
> -		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
> -			data->gp0_received = false;
> -
>   		hdev->stat.cmd_tx++;
>   		break;
>   	case HCI_ACLDATA_PKT:
> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>   	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>   	       BTINTEL_PCIE_HCI_TYPE_LEN);
>   
> +	if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
> +		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01) {

Why not keep the form of just one if statement with && in the condition? 
as below?

> +			data->gp0_received = false;
> +			old_ctxt = data->alive_intr_ctxt;
> +			data->alive_intr_ctxt =
> +				(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
> +					BTINTEL_PCIE_HCI_RESET);
> +			bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive context changed: %s  ->  %s",
> +				   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
> +				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> +		}
> +	}
> +
>   	ret = btintel_pcie_send_sync(data, skb);
>   	if (ret) {
>   		hdev->stat.err_tx++;
> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>   
>   	if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>   	    (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
> -		old_ctxt = data->alive_intr_ctxt;
> -		data->alive_intr_ctxt =
> -			(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
> -				BTINTEL_PCIE_HCI_RESET);
> -		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
> -			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
> -			   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>   		ret = wait_event_timeout(data->gp0_wait_q,
>   					 data->gp0_received,
>   					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));


Kind regards,

Paul

RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[K, Kiran]

Hi Paul,

Thanks for the comments.

>Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive
>interrupt
>
>Dear Sai, dear Kiran,
>
>
>Thank you for your patch.
>
>Am 07.07.25 um 05:46 schrieb Kiran K:
>> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
>> reset) command. This change fixes the driver to properly wait for the
>> alive interrupt.
>
>What is the consequence of not waiting?

This is an alignment between driver and firmware. If driver doesn’t wait for alive interrupt, then there is chance of stack sending commands before the firmware is ready to accept.
>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>> driver and firmware")
>> ---
>>   drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 1113a6310bd0..f893ad6fc87a 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct
>btintel_pcie_data *data)
>>   	case BTINTEL_PCIE_INTEL_HCI_RESET1:
>>   		if (btintel_pcie_in_op(data)) {
>>   			submit_rx = true;
>> +			signal_waitq = true;
>>   			break;
>>   		}
>>
>>   		if (btintel_pcie_in_iml(data)) {
>>   			submit_rx = true;
>> +			signal_waitq = true;
>>   			data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>>   			break;
>>   		}
>> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>   			if (opcode == 0xfc01)
>>   				btintel_pcie_inject_cmd_complete(hdev,
>opcode);
>>   		}
>> -		/* Firmware raises alive interrupt on HCI_OP_RESET */
>> -		if (opcode == HCI_OP_RESET)
>> +
>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or
>0xfc01*/
>
>A space is missing before */.
Ack.

>
>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>
>Please define a macro for the magic number.
>
This is vendor specific opcode and is also shared across btintel.c, btusb.c and hci_intel.c. Would it be acceptable to submit a separate patch for this change alone?

>>   			data->gp0_received = false;
>>
>>   		hdev->stat.cmd_tx++;
>> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>   		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context
>changed: %s  ->  %s",
>>   			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>   			   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>> -		if (opcode == HCI_OP_RESET) {
>> -			ret = wait_event_timeout(data->gp0_wait_q,
>> -						 data->gp0_received,
>> -
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> -			if (!ret) {
>> -				hdev->stat.err_tx++;
>> -				bt_dev_err(hdev, "No alive interrupt received
>for %s",
>> -					   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>> -				ret = -ETIME;
>> -				goto exit_error;
>> -			}
>> +		ret = wait_event_timeout(data->gp0_wait_q,
>> +					 data->gp0_received,
>> +
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> +		if (!ret) {
>> +			hdev->stat.err_tx++;
>> +			bt_dev_err(hdev, "No alive interrupt received for %s",
>> +				   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>
>In a follow-up patch, the log message could be improved by also adding the
>timeout value to it.
Ack.

>
>> +			ret = -ETIME;
>> +			goto exit_error;
>>   		}
>>   	}
>>   	hdev->stat.byte_tx += skb->len;
>
>
>Kind regards,
>
>Paul

Thanks,
Kiran

RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[K, Kiran]

Hi Paul,

>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state
>handling
>
>Dear Kiran, dear Sai,
>
>
>Thank you for the patch.
>
>Am 07.07.25 um 05:46 schrieb Kiran K:
>> Firmware raises alive interrpt on sending 0xfc01 command. Alive
>> context
>
>interr*u*pt
>
>(I would have hoped, two developers would spot such a typo, a spell checker
>also highlights.)

Ack. Unfortunately  'interrpt' was missing in my codespell dictionary. I have updated the same. Thanks.
>
>> maintained in driver needs to be updated before sending 0xfc01 (Intel
>> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition
>> where the context is also updated in threaded irq.
>
>Do you have a reproducer for the issue?
Yes. Issue was reproduced in stress reboot scenario although reproduction rate is less - 1/25. 
>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>> driver and firmware")
>
>I’d add the Fixes tag before the Signed-off-by lines.
Ack.

>
>> ---
>>   drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>>   1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index f893ad6fc87a..d29103b102e4 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>   				btintel_pcie_inject_cmd_complete(hdev,
>opcode);
>>   		}
>>
>> -		/* Firmware raises alive interrupt on HCI_OP_RESET or
>0xfc01*/
>> -		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>> -			data->gp0_received = false;
>> -
>>   		hdev->stat.cmd_tx++;
>>   		break;
>>   	case HCI_ACLDATA_PKT:
>> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>   	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>>   	       BTINTEL_PCIE_HCI_TYPE_LEN);
>>
>> +	if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or
>0xfc01*/
>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
>
>Why not keep the form of just one if statement with && in the condition?
>as below?
>
>> +			data->gp0_received = false;
>> +			old_ctxt = data->alive_intr_ctxt;
>> +			data->alive_intr_ctxt =
>> +				(opcode == 0xfc01 ?
>BTINTEL_PCIE_INTEL_HCI_RESET1 :
>> +					BTINTEL_PCIE_HCI_RESET);
>> +			bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive
>context changed: %s  ->  %s",
>> +				   opcode,
>btintel_pcie_alivectxt_state2str(old_ctxt),
>> +				   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>> +		}
>> +	}
>> +
>>   	ret = btintel_pcie_send_sync(data, skb);
>>   	if (ret) {
>>   		hdev->stat.err_tx++;
>> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct
>> hci_dev *hdev,
>>
>>   	if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>>   	    (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
>> -		old_ctxt = data->alive_intr_ctxt;
>> -		data->alive_intr_ctxt =
>> -			(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1
>:
>> -				BTINTEL_PCIE_HCI_RESET);
>> -		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context
>changed: %s  ->  %s",
>> -			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>> -			   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>>   		ret = wait_event_timeout(data->gp0_wait_q,
>>   					 data->gp0_received,
>>
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>
>
>Kind regards,
>
>Paul

Regards,
Kiran

Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[Paul Menzel]

Dear Kiran,


Am 08.07.25 um 14:23 schrieb K, Kiran:

>> Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

>> Am 07.07.25 um 05:46 schrieb Kiran K:
>>> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
>>> reset) command. This change fixes the driver to properly wait for the
>>> alive interrupt.
>>
>> What is the consequence of not waiting?
> 
> This is an alignment between driver and firmware. If driver doesn’t
> wait for alive interrupt, then there is chance of stack sending
> commands before the firmware is ready to accept.

Thank you for elaborating. It’d be great if you added it to the commit 
message, when you resend.

>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")

I would also put the Fixes: tag above the Signed-off-by line.

>>> ---
>>>    drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>>>    1 file changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/btintel_pcie.c
>>> b/drivers/bluetooth/btintel_pcie.c
>>> index 1113a6310bd0..f893ad6fc87a 100644
>>> --- a/drivers/bluetooth/btintel_pcie.c
>>> +++ b/drivers/bluetooth/btintel_pcie.c
>>> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct btintel_pcie_data *data)
>>>    	case BTINTEL_PCIE_INTEL_HCI_RESET1:
>>>    		if (btintel_pcie_in_op(data)) {
>>>    			submit_rx = true;
>>> +			signal_waitq = true;
>>>    			break;
>>>    		}
>>>
>>>    		if (btintel_pcie_in_iml(data)) {
>>>    			submit_rx = true;
>>> +			signal_waitq = true;
>>>    			data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>>>    			break;
>>>    		}
>>> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>>>    			if (opcode == 0xfc01)
>>>    				btintel_pcie_inject_cmd_complete(hdev, opcode);
>>>    		}
>>> -		/* Firmware raises alive interrupt on HCI_OP_RESET */
>>> -		if (opcode == HCI_OP_RESET)
>>> +
>>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>>
>> A space is missing before */.
> Ack.
> 
>>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>>
>> Please define a macro for the magic number.
> 
> This is vendor specific opcode and is also shared across btintel.c,
> btusb.c and hci_intel.c. Would it be acceptable to submit a separate
> patch for this change alone?

Sure. Fine by me.

>>>    			data->gp0_received = false;
>>>
>>>    		hdev->stat.cmd_tx++;
>>> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>>>    		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
>>>    			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>>    			   btintel_pcie_alivectxt_state2str(data - alive_intr_ctxt));
>>> -		if (opcode == HCI_OP_RESET) {
>>> -			ret = wait_event_timeout(data->gp0_wait_q,
>>> -						 data->gp0_received,
>>> -						 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>>> -			if (!ret) {
>>> -				hdev->stat.err_tx++;
>>> -				bt_dev_err(hdev, "No alive interrupt received for %s",
>>> -					   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>>> -				ret = -ETIME;
>>> -				goto exit_error;
>>> -			}
>>> +		ret = wait_event_timeout(data->gp0_wait_q,
>>> +					 data->gp0_received,
>>> +					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>>> +		if (!ret) {
>>> +			hdev->stat.err_tx++;
>>> +			bt_dev_err(hdev, "No alive interrupt received for %s",
>>> +				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>>
>> In a follow-up patch, the log message could be improved by also adding the
>> timeout value to it.
> Ack.
> 
>>> +			ret = -ETIME;
>>> +			goto exit_error;
>>>    		}
>>>    	}
>>>    	hdev->stat.byte_tx += skb->len;

Kind regards,

Paul

Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[Luiz Augusto von Dentz]

Hi Kiran,

On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>
> Firmware raises alive interrpt on sending 0xfc01 command. Alive context
> maintained in driver needs to be updated before sending 0xfc01 (Intel
> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition where
> the context is also updated in threaded irq.

This should be a little more specific, like explaining what is alive
interrupt supposed to mean or if we cannot do any communication wheel
these are pending?

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
> ---
>  drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index f893ad6fc87a..d29103b102e4 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>                                 btintel_pcie_inject_cmd_complete(hdev, opcode);
>                 }
>
> -               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
> -               if (opcode == HCI_OP_RESET || opcode == 0xfc01)
> -                       data->gp0_received = false;
> -
>                 hdev->stat.cmd_tx++;
>                 break;
>         case HCI_ACLDATA_PKT:
> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>         memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>                BTINTEL_PCIE_HCI_TYPE_LEN);
>
> +       if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
> +               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
> +               if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
> +                       data->gp0_received = false;

This type of flags should really be made into atomic flags so they can
be checked atomically, anyway this shouldn't block these fixes but
something I very much look forward to be changed.

> +                       old_ctxt = data->alive_intr_ctxt;
> +                       data->alive_intr_ctxt =
> +                               (opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
> +                                       BTINTEL_PCIE_HCI_RESET);
> +                       bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive context changed: %s  ->  %s",
> +                                  opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
> +                                  btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> +               }
> +       }
> +
>         ret = btintel_pcie_send_sync(data, skb);
>         if (ret) {
>                 hdev->stat.err_tx++;
> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>
>         if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>             (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
> -               old_ctxt = data->alive_intr_ctxt;
> -               data->alive_intr_ctxt =
> -                       (opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
> -                               BTINTEL_PCIE_HCI_RESET);
> -               bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
> -                          opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
> -                          btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>                 ret = wait_event_timeout(data->gp0_wait_q,
>                                          data->gp0_received,
>                                          msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> --
> 2.43.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[Luiz Augusto von Dentz]

Hi Kiran,

On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>
> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
> reset) command. This change fixes the driver to properly wait for the
> alive interrupt.
>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
> ---
>  drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 1113a6310bd0..f893ad6fc87a 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct btintel_pcie_data *data)
>         case BTINTEL_PCIE_INTEL_HCI_RESET1:
>                 if (btintel_pcie_in_op(data)) {
>                         submit_rx = true;
> +                       signal_waitq = true;
>                         break;
>                 }
>
>                 if (btintel_pcie_in_iml(data)) {
>                         submit_rx = true;
> +                       signal_waitq = true;
>                         data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>                         break;
>                 }
> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>                         if (opcode == 0xfc01)
>                                 btintel_pcie_inject_cmd_complete(hdev, opcode);
>                 }
> -               /* Firmware raises alive interrupt on HCI_OP_RESET */
> -               if (opcode == HCI_OP_RESET)
> +
> +               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
> +               if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>                         data->gp0_received = false;
>
>                 hdev->stat.cmd_tx++;
> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>                 bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
>                            opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>                            btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> -               if (opcode == HCI_OP_RESET) {
> -                       ret = wait_event_timeout(data->gp0_wait_q,
> -                                                data->gp0_received,
> -                                                msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> -                       if (!ret) {
> -                               hdev->stat.err_tx++;
> -                               bt_dev_err(hdev, "No alive interrupt received for %s",
> -                                          btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> -                               ret = -ETIME;
> -                               goto exit_error;
> -                       }
> +               ret = wait_event_timeout(data->gp0_wait_q,
> +                                        data->gp0_received,
> +                                        msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> +               if (!ret) {
> +                       hdev->stat.err_tx++;
> +                       bt_dev_err(hdev, "No alive interrupt received for %s",
> +                                  btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
> +                       ret = -ETIME;
> +                       goto exit_error;

This should probably go into btintel_pcie_send_sync instead of doing a
post handling as above, also if I read this right then we have to wait
on 2 interrupts when it comes to HCI_Reset and 0xfc01?
btintel_pcie_send_sync already waits on URBD0 this also adds GP0,
having 2 interrupts means proper ordering needs to be enforced,
otherwise if GP0 happens before URBD0 this will probably timeout, if
this is done on purpose let document why we think it is ok to do it.

>                 }
>         }
>         hdev->stat.byte_tx += skb->len;
> --
> 2.43.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[Paul Menzel]

Dear Kiran,


Thank you for your reply.

Am 08.07.25 um 14:30 schrieb K, Kiran:

>> Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

>> Am 07.07.25 um 05:46 schrieb Kiran K:
>>> Firmware raises alive interrpt on sending 0xfc01 command. Alive context
>>
>> interr*u*pt
>>
>> (I would have hoped, two developers would spot such a typo, a spell checker
>> also highlights.)
> 
> Ack. Unfortunately  'interrpt' was missing in my codespell
> dictionary. I have updated the same. Thanks.

hunspell or a/ispell should find it by defautl.

>>> maintained in driver needs to be updated before sending 0xfc01 (Intel
>>> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition
>>> where the context is also updated in threaded irq.
>>
>> Do you have a reproducer for the issue?
> Yes. Issue was reproduced in stress reboot scenario although reproduction rate is less - 1/25.

It’d be great if you added that to the commit message, and what 
scripts/commands you run for this stress reboot scenario.

>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
>>
>> I’d add the Fixes tag before the Signed-off-by lines.
> Ack.

There is one more comment below.

>>> ---
>>>    drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/btintel_pcie.c
>>> b/drivers/bluetooth/btintel_pcie.c
>>> index f893ad6fc87a..d29103b102e4 100644
>>> --- a/drivers/bluetooth/btintel_pcie.c
>>> +++ b/drivers/bluetooth/btintel_pcie.c
>>> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>>>    				btintel_pcie_inject_cmd_complete(hdev, opcode);
>>>    		}
>>>
>>> -		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>>> -		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>>> -			data->gp0_received = false;
>>> -
>>>    		hdev->stat.cmd_tx++;
>>>    		break;
>>>    	case HCI_ACLDATA_PKT:
>>> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>>>    	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>>>    	       BTINTEL_PCIE_HCI_TYPE_LEN);
>>>
>>> +	if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
>>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
>>
>> Why not keep the form of just one if statement with && in the condition?
>> as below?
>>
>>> +			data->gp0_received = false;
>>> +			old_ctxt = data->alive_intr_ctxt;
>>> +			data->alive_intr_ctxt =
>>> +				(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
>>> +					BTINTEL_PCIE_HCI_RESET);
>>> +			bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive context changed: %s  ->  %s",
>>> +				   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>> +				   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>>> +		}
>>> +	}
>>> +
>>>    	ret = btintel_pcie_send_sync(data, skb);
>>>    	if (ret) {
>>>    		hdev->stat.err_tx++;
>>> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct hci_dev *hdev,
>>>
>>>    	if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>>>    	    (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
>>> -		old_ctxt = data->alive_intr_ctxt;
>>> -		data->alive_intr_ctxt =
>>> -			(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1
>> :
>>> -				BTINTEL_PCIE_HCI_RESET);
>>> -		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed: %s  ->  %s",
>>> -			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>> -			   btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>>>    		ret = wait_event_timeout(data->gp0_wait_q,
>>>    					 data->gp0_received,
>>>					 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

Kind regards,

Paul

RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[K, Kiran]

Hi Paul,

>Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive
>interrupt
>
>Dear Kiran,
>
>
>Am 08.07.25 um 14:23 schrieb K, Kiran:
>
>>> Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait
>>> for alive interrupt
>
>>> Am 07.07.25 um 05:46 schrieb Kiran K:
>>>> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
>>>> reset) command. This change fixes the driver to properly wait for
>>>> the alive interrupt.
>>>
>>> What is the consequence of not waiting?
>>
>> This is an alignment between driver and firmware. If driver doesn’t
>> wait for alive interrupt, then there is chance of stack sending
>> commands before the firmware is ready to accept.
>
>Thank you for elaborating. It’d be great if you added it to the commit message,
>when you resend.
Ack.

>
>>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>>>> driver and firmware")
>
>I would also put the Fixes: tag above the Signed-off-by line.
Ack.

>
>>>> ---
>>>>    drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>>>>    1 file changed, 14 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btintel_pcie.c
>>>> b/drivers/bluetooth/btintel_pcie.c
>>>> index 1113a6310bd0..f893ad6fc87a 100644
>>>> --- a/drivers/bluetooth/btintel_pcie.c
>>>> +++ b/drivers/bluetooth/btintel_pcie.c
>>>> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct
>btintel_pcie_data *data)
>>>>    	case BTINTEL_PCIE_INTEL_HCI_RESET1:
>>>>    		if (btintel_pcie_in_op(data)) {
>>>>    			submit_rx = true;
>>>> +			signal_waitq = true;
>>>>    			break;
>>>>    		}
>>>>
>>>>    		if (btintel_pcie_in_iml(data)) {
>>>>    			submit_rx = true;
>>>> +			signal_waitq = true;
>>>>    			data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>>>>    			break;
>>>>    		}
>>>> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>>>    			if (opcode == 0xfc01)
>>>>    				btintel_pcie_inject_cmd_complete(hdev,
>opcode);
>>>>    		}
>>>> -		/* Firmware raises alive interrupt on HCI_OP_RESET */
>>>> -		if (opcode == HCI_OP_RESET)
>>>> +
>>>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or
>0xfc01*/
>>>
>>> A space is missing before */.
>> Ack.
>>
>>>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>>>
>>> Please define a macro for the magic number.
>>
>> This is vendor specific opcode and is also shared across btintel.c,
>> btusb.c and hci_intel.c. Would it be acceptable to submit a separate
>> patch for this change alone?
>
>Sure. Fine by me.
Thanks.

>
>>>>    			data->gp0_received = false;
>>>>
>>>>    		hdev->stat.cmd_tx++;
>>>> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct
>hci_dev *hdev,
>>>>    		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context
>changed: %s  ->  %s",
>>>>    			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>>>    			   btintel_pcie_alivectxt_state2str(data -
>alive_intr_ctxt));
>>>> -		if (opcode == HCI_OP_RESET) {
>>>> -			ret = wait_event_timeout(data->gp0_wait_q,
>>>> -						 data->gp0_received,
>>>> -
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>>>> -			if (!ret) {
>>>> -				hdev->stat.err_tx++;
>>>> -				bt_dev_err(hdev, "No alive interrupt received
>for %s",
>>>> -					   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>>>> -				ret = -ETIME;
>>>> -				goto exit_error;
>>>> -			}
>>>> +		ret = wait_event_timeout(data->gp0_wait_q,
>>>> +					 data->gp0_received,
>>>> +
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>>>> +		if (!ret) {
>>>> +			hdev->stat.err_tx++;
>>>> +			bt_dev_err(hdev, "No alive interrupt received for %s",
>>>> +				   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>>>
>>> In a follow-up patch, the log message could be improved by also
>>> adding the timeout value to it.
>> Ack.
>>
>>>> +			ret = -ETIME;
>>>> +			goto exit_error;
>>>>    		}
>>>>    	}
>>>>    	hdev->stat.byte_tx += skb->len;
>
>Kind regards,
>
>Paul

Regards,
Kiran

RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[K, Kiran]

Hi Luiz,

>Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive
>interrupt
>
>Hi Kiran,
>
>On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>>
>> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
>> reset) command. This change fixes the driver to properly wait for the
>> alive interrupt.
>>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>> driver and firmware")
>> ---
>>  drivers/bluetooth/btintel_pcie.c | 27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 1113a6310bd0..f893ad6fc87a 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -947,11 +947,13 @@ static void btintel_pcie_msix_gp0_handler(struct
>btintel_pcie_data *data)
>>         case BTINTEL_PCIE_INTEL_HCI_RESET1:
>>                 if (btintel_pcie_in_op(data)) {
>>                         submit_rx = true;
>> +                       signal_waitq = true;
>>                         break;
>>                 }
>>
>>                 if (btintel_pcie_in_iml(data)) {
>>                         submit_rx = true;
>> +                       signal_waitq = true;
>>                         data->alive_intr_ctxt = BTINTEL_PCIE_FW_DL;
>>                         break;
>>                 }
>> @@ -1985,8 +1987,9 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>                         if (opcode == 0xfc01)
>>                                 btintel_pcie_inject_cmd_complete(hdev, opcode);
>>                 }
>> -               /* Firmware raises alive interrupt on HCI_OP_RESET */
>> -               if (opcode == HCI_OP_RESET)
>> +
>> +               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>> +               if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>>                         data->gp0_received = false;
>>
>>                 hdev->stat.cmd_tx++;
>> @@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>                 bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed:
>%s  ->  %s",
>>                            opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>                            btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>> -               if (opcode == HCI_OP_RESET) {
>> -                       ret = wait_event_timeout(data->gp0_wait_q,
>> -                                                data->gp0_received,
>> -
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> -                       if (!ret) {
>> -                               hdev->stat.err_tx++;
>> -                               bt_dev_err(hdev, "No alive interrupt received for %s",
>> -                                          btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>> -                               ret = -ETIME;
>> -                               goto exit_error;
>> -                       }
>> +               ret = wait_event_timeout(data->gp0_wait_q,
>> +                                        data->gp0_received,
>> +
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> +               if (!ret) {
>> +                       hdev->stat.err_tx++;
>> +                       bt_dev_err(hdev, "No alive interrupt received for %s",
>> +                                  btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>> +                       ret = -ETIME;
>> +                       goto exit_error;
>
>This should probably go into btintel_pcie_send_sync instead of doing a post
>handling as above, also if I read this right then we have to wait on 2 interrupts
>when it comes to HCI_Reset and 0xfc01?
>btintel_pcie_send_sync already waits on URBD0 this also adds GP0, having 2
>interrupts means proper ordering needs to be enforced, otherwise if GP0
>happens before URBD0 this will probably timeout, if this is done on purpose
>let document why we think it is ok to do it.

The URBD0 interrupt is raised immediately after packet reception, while the Alive interrupt is triggered after the firmware initializes and enables the data path for receiving TX packets. This order is guaranteed by firmware.

>
>>                 }
>>         }
>>         hdev->stat.byte_tx += skb->len;
>> --
>> 2.43.0
>>
>>
>
>
>--
>Luiz Augusto von Dentz

RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[K, Kiran]

Hi Luiz,

>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state
>handling
>
>Hi Kiran,
>
>On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>>
>> Firmware raises alive interrpt on sending 0xfc01 command. Alive
>> context maintained in driver needs to be updated before sending 0xfc01
>> (Intel
>> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition
>> where the context is also updated in threaded irq.
>
>This should be a little more specific, like explaining what is alive interrupt
>supposed to mean or if we cannot do any communication wheel these are
>pending?
Ack.  I will update the commit message.

>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>> driver and firmware")
>> ---
>>  drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>>  1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index f893ad6fc87a..d29103b102e4 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>                                 btintel_pcie_inject_cmd_complete(hdev, opcode);
>>                 }
>>
>> -               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>> -               if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>> -                       data->gp0_received = false;
>> -
>>                 hdev->stat.cmd_tx++;
>>                 break;
>>         case HCI_ACLDATA_PKT:
>> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct hci_dev
>*hdev,
>>         memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>>                BTINTEL_PCIE_HCI_TYPE_LEN);
>>
>> +       if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
>> +               /* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>> +               if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
>> +                       data->gp0_received = false;
>
>This type of flags should really be made into atomic flags so they can be
>checked atomically, anyway this shouldn't block these fixes but something I
>very much look forward to be changed.
Ack.
>
>> +                       old_ctxt = data->alive_intr_ctxt;
>> +                       data->alive_intr_ctxt =
>> +                               (opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
>> +                                       BTINTEL_PCIE_HCI_RESET);
>> +                       bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive context
>changed: %s  ->  %s",
>> +                                  opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>> +                                  btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>> +               }
>> +       }
>> +
>>         ret = btintel_pcie_send_sync(data, skb);
>>         if (ret) {
>>                 hdev->stat.err_tx++;
>> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct
>> hci_dev *hdev,
>>
>>         if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>>             (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
>> -               old_ctxt = data->alive_intr_ctxt;
>> -               data->alive_intr_ctxt =
>> -                       (opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1 :
>> -                               BTINTEL_PCIE_HCI_RESET);
>> -               bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context changed:
>%s  ->  %s",
>> -                          opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>> -                          btintel_pcie_alivectxt_state2str(data->alive_intr_ctxt));
>>                 ret = wait_event_timeout(data->gp0_wait_q,
>>                                          data->gp0_received,
>>
>> msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> --
>> 2.43.0
>>
>>
>
>
>--
>Luiz Augusto von Dentz

Thanks,
Kiran

RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling

[K, Kiran]

Hi Paul,

>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state
>handling
>
>Dear Kiran,
>
>
>Thank you for your reply.
>
>Am 08.07.25 um 14:30 schrieb K, Kiran:
>
>>> Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive
>>> context state handling
>
>>> Am 07.07.25 um 05:46 schrieb Kiran K:
>>>> Firmware raises alive interrpt on sending 0xfc01 command. Alive
>>>> context
>>>
>>> interr*u*pt
>>>
>>> (I would have hoped, two developers would spot such a typo, a spell
>>> checker also highlights.)
>>
>> Ack. Unfortunately  'interrpt' was missing in my codespell dictionary.
>> I have updated the same. Thanks.
>
>hunspell or a/ispell should find it by defautl.
>
Ack.

>>>> maintained in driver needs to be updated before sending 0xfc01
>>>> (Intel
>>>> Reset) or 0x03c0 (HCI Reset) to avoid the potential race condition
>>>> where the context is also updated in threaded irq.
>>>
>>> Do you have a reproducer for the issue?
>> Yes. Issue was reproduced in stress reboot scenario although reproduction
>rate is less - 1/25.
>
>It’d be great if you added that to the commit message, and what
>scripts/commands you run for this stress reboot scenario.

Ack. I will update the commit message.

>
>>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>>>> driver and firmware")
>>>
>>> I’d add the Fixes tag before the Signed-off-by lines.
>> Ack.
>
>There is one more comment below.
>
Sorry. I missed this one. I will fix it in the v2 version of the patch.

>>>> ---
>>>>    drivers/bluetooth/btintel_pcie.c | 25 ++++++++++++++-----------
>>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btintel_pcie.c
>>>> b/drivers/bluetooth/btintel_pcie.c
>>>> index f893ad6fc87a..d29103b102e4 100644
>>>> --- a/drivers/bluetooth/btintel_pcie.c
>>>> +++ b/drivers/bluetooth/btintel_pcie.c
>>>> @@ -1988,10 +1988,6 @@ static int btintel_pcie_send_frame(struct
>hci_dev *hdev,
>>>>    				btintel_pcie_inject_cmd_complete(hdev,
>opcode);
>>>>    		}
>>>>
>>>> -		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>>>> -		if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>>>> -			data->gp0_received = false;
>>>> -
>>>>    		hdev->stat.cmd_tx++;
>>>>    		break;
>>>>    	case HCI_ACLDATA_PKT:
>>>> @@ -2012,6 +2008,20 @@ static int btintel_pcie_send_frame(struct
>hci_dev *hdev,
>>>>    	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
>>>>    	       BTINTEL_PCIE_HCI_TYPE_LEN);
>>>>
>>>> +	if (type == BTINTEL_PCIE_HCI_CMD_PKT) {
>>>> +		/* Firmware raises alive interrupt on HCI_OP_RESET or 0xfc01*/
>>>> +		if (opcode == HCI_OP_RESET || opcode == 0xfc01) {
>>>
>>> Why not keep the form of just one if statement with && in the condition?
>>> as below?
Ack.
>>>
>>>> +			data->gp0_received = false;
>>>> +			old_ctxt = data->alive_intr_ctxt;
>>>> +			data->alive_intr_ctxt =
>>>> +				(opcode == 0xfc01 ?
>BTINTEL_PCIE_INTEL_HCI_RESET1 :
>>>> +					BTINTEL_PCIE_HCI_RESET);
>>>> +			bt_dev_dbg(data->hdev, "sending cmd: 0x%4.4x alive
>context changed: %s  ->  %s",
>>>> +				   opcode,
>btintel_pcie_alivectxt_state2str(old_ctxt),
>>>> +				   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>>>> +		}
>>>> +	}
>>>> +
>>>>    	ret = btintel_pcie_send_sync(data, skb);
>>>>    	if (ret) {
>>>>    		hdev->stat.err_tx++;
>>>> @@ -2021,13 +2031,6 @@ static int btintel_pcie_send_frame(struct
>>>> hci_dev *hdev,
>>>>
>>>>    	if (type == BTINTEL_PCIE_HCI_CMD_PKT &&
>>>>    	    (opcode == HCI_OP_RESET || opcode == 0xfc01)) {
>>>> -		old_ctxt = data->alive_intr_ctxt;
>>>> -		data->alive_intr_ctxt =
>>>> -			(opcode == 0xfc01 ? BTINTEL_PCIE_INTEL_HCI_RESET1
>>> :
>>>> -				BTINTEL_PCIE_HCI_RESET);
>>>> -		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context
>changed: %s  ->  %s",
>>>> -			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
>>>> -			   btintel_pcie_alivectxt_state2str(data-
>>alive_intr_ctxt));
>>>>    		ret = wait_event_timeout(data->gp0_wait_q,
>>>>    					 data->gp0_received,
>>>>
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>
>Kind regards,
>
>Paul

Thanks,
Kiran