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

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:

quoted

quoted

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

quoted

quoted

Am 07.07.25 um 05:46 schrieb Kiran K:

quoted

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.

quoted

quoted

quoted

Signed-off-by: Sai Teja Aluvala <redacted>
Signed-off-by: Kiran K <redacted>
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.

quoted

quoted

quoted

---
   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)

quoted

quoted

quoted

   	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,

quoted

quoted

quoted

   			if (opcode == 0xfc01)
   				btintel_pcie_inject_cmd_complete(hdev,

opcode);

quoted

quoted

quoted

   		}
-		/* Firmware raises alive interrupt on HCI_OP_RESET */
-		if (opcode == HCI_OP_RESET)
+
+		/* Firmware raises alive interrupt on HCI_OP_RESET or

0xfc01*/

quoted

quoted

A space is missing before */.

Ack.

quoted

quoted

+		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.

quoted

quoted

quoted

   			data->gp0_received = false;

   		hdev->stat.cmd_tx++;

@@ -2025,17 +2028,15 @@ static int btintel_pcie_send_frame(struct

hci_dev *hdev,

quoted

quoted

quoted

   		bt_dev_dbg(data->hdev, "sent cmd: 0x%4.4x alive context

changed: %s  ->  %s",

quoted

quoted

quoted

   			   opcode, btintel_pcie_alivectxt_state2str(old_ctxt),
   			   btintel_pcie_alivectxt_state2str(data -

alive_intr_ctxt));

quoted

quoted

quoted

-		if (opcode == HCI_OP_RESET) {
-			ret = wait_event_timeout(data->gp0_wait_q,
-						 data->gp0_received,
-

msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

quoted

quoted

quoted

-			if (!ret) {
-				hdev->stat.err_tx++;
-				bt_dev_err(hdev, "No alive interrupt received

for %s",

quoted

quoted

quoted

-					   btintel_pcie_alivectxt_state2str(data-

alive_intr_ctxt));

quoted

quoted

-				ret = -ETIME;
-				goto exit_error;
-			}
+		ret = wait_event_timeout(data->gp0_wait_q,
+					 data->gp0_received,
+

msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

quoted

quoted

quoted

+		if (!ret) {
+			hdev->stat.err_tx++;
+			bt_dev_err(hdev, "No alive interrupt received for %s",
+				   btintel_pcie_alivectxt_state2str(data-

alive_intr_ctxt));

quoted

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

Ack.

quoted

quoted

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

Kind regards,

Paul

Regards,
Kiran