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