Hi Paul,
>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state
>handling
>
>Dear Kiran, dear Sai,
>
>
>Thank you for the patch.
>
>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.
>
>> 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.
>
>> 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.
>
>> ---
>> 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?
>
>> + 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
Regards,
Kiran
