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