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@xxxxxxxxx>
Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
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
