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@xxxxxxxxx>
>>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@xxxxxxxxx>
>>>> 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
