* [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-07 3:46 [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt Kiran K
@ 2025-07-07 3:46 ` Kiran K
2025-07-07 6:15 ` Paul Menzel
2025-07-08 20:30 ` Luiz Augusto von Dentz
2025-07-07 4:27 ` [v1,1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt bluez.test.bot
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Kiran K @ 2025-07-07 3:46 UTC (permalink / raw)
To: linux-bluetooth
Cc: ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, aluvala.sai.teja, Kiran K
Firmware raises alive interrpt on sending 0xfc01 command. Alive context
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.
Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
---
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) {
+ 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));
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-07 3:46 ` [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling Kiran K
@ 2025-07-07 6:15 ` Paul Menzel
2025-07-08 12:30 ` K, Kiran
2025-07-08 20:30 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2025-07-07 6:15 UTC (permalink / raw)
To: Kiran K, Sai Teja Aluvala
Cc: ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, linux-bluetooth
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.)
> 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?
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
I’d add the Fixes tag before the Signed-off-by lines.
> ---
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-07 6:15 ` Paul Menzel
@ 2025-07-08 12:30 ` K, Kiran
2025-07-09 6:02 ` Paul Menzel
0 siblings, 1 reply; 15+ messages in thread
From: K, Kiran @ 2025-07-08 12:30 UTC (permalink / raw)
To: Paul Menzel, Aluvala Sai Teja
Cc: Srivatsa, Ravishankar, Tumkur Narayan, Chethan,
Devegowda, Chandrashekar, linux-bluetooth@vger.kernel.org
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@intel.com>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-08 12:30 ` K, Kiran
@ 2025-07-09 6:02 ` Paul Menzel
2025-07-15 2:16 ` K, Kiran
0 siblings, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2025-07-09 6:02 UTC (permalink / raw)
To: Kiran K, Aluvala Sai Teja
Cc: Ravishankar Srivatsa, Chethan Tumkur Narayan,
Chandrashekar Devegowda, linux-bluetooth
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.
>>> 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.
>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>> 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.
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-09 6:02 ` Paul Menzel
@ 2025-07-15 2:16 ` K, Kiran
0 siblings, 0 replies; 15+ messages in thread
From: K, Kiran @ 2025-07-15 2:16 UTC (permalink / raw)
To: Paul Menzel, Aluvala Sai Teja
Cc: Srivatsa, Ravishankar, Tumkur Narayan, Chethan,
Devegowda, Chandrashekar, linux-bluetooth@vger.kernel.org
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@intel.com>
>>>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>>>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-07 3:46 ` [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling Kiran K
2025-07-07 6:15 ` Paul Menzel
@ 2025-07-08 20:30 ` Luiz Augusto von Dentz
2025-07-15 2:11 ` K, Kiran
1 sibling, 1 reply; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2025-07-08 20:30 UTC (permalink / raw)
To: Kiran K
Cc: linux-bluetooth, ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, aluvala.sai.teja
Hi Kiran,
On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>
> Firmware raises alive interrpt on sending 0xfc01 command. Alive context
> 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.
This should be a little more specific, like explaining what is alive
interrupt supposed to mean or if we cannot do any communication wheel
these are pending?
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between driver and firmware")
> ---
> 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) {
> + data->gp0_received = false;
This type of flags should really be made into atomic flags so they can
be checked atomically, anyway this shouldn't block these fixes but
something I very much look forward to be changed.
> + 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));
> --
> 2.43.0
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling
2025-07-08 20:30 ` Luiz Augusto von Dentz
@ 2025-07-15 2:11 ` K, Kiran
0 siblings, 0 replies; 15+ messages in thread
From: K, Kiran @ 2025-07-15 2:11 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth@vger.kernel.org, Srivatsa, Ravishankar,
Tumkur Narayan, Chethan, Devegowda, Chandrashekar,
Aluvala Sai Teja
Hi Luiz,
>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state
>handling
>
>Hi Kiran,
>
>On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>>
>> Firmware raises alive interrpt on sending 0xfc01 command. Alive
>> context 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.
>
>This should be a little more specific, like explaining what is alive interrupt
>supposed to mean or if we cannot do any communication wheel these are
>pending?
Ack. I will update the commit message.
>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Fixes: 05c200c8f029 ("Bluetooth: btintel_pcie: Add handshake between
>> driver and firmware")
>> ---
>> 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) {
>> + data->gp0_received = false;
>
>This type of flags should really be made into atomic flags so they can be
>checked atomically, anyway this shouldn't block these fixes but something I
>very much look forward to be changed.
Ack.
>
>> + 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));
>> --
>> 2.43.0
>>
>>
>
>
>--
>Luiz Augusto von Dentz
Thanks,
Kiran
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [v1,1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-07 3:46 [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt Kiran K
2025-07-07 3:46 ` [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling Kiran K
@ 2025-07-07 4:27 ` bluez.test.bot
2025-07-07 6:07 ` [PATCH v1 1/2] " Paul Menzel
2025-07-08 20:51 ` Luiz Augusto von Dentz
3 siblings, 0 replies; 15+ messages in thread
From: bluez.test.bot @ 2025-07-07 4:27 UTC (permalink / raw)
To: linux-bluetooth, kiran.k
[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=979530
---Test result---
Test Summary:
CheckPatch PENDING 0.36 seconds
GitLint PENDING 0.23 seconds
SubjectPrefix PASS 0.18 seconds
BuildKernel PASS 24.10 seconds
CheckAllWarning PASS 26.41 seconds
CheckSparse PASS 29.76 seconds
BuildKernel32 PASS 23.98 seconds
TestRunnerSetup PASS 466.76 seconds
TestRunner_l2cap-tester PASS 24.93 seconds
TestRunner_iso-tester PASS 33.97 seconds
TestRunner_bnep-tester PASS 5.93 seconds
TestRunner_mgmt-tester FAIL 130.72 seconds
TestRunner_rfcomm-tester PASS 9.22 seconds
TestRunner_sco-tester PASS 14.60 seconds
TestRunner_ioctl-tester PASS 9.92 seconds
TestRunner_mesh-tester FAIL 11.32 seconds
TestRunner_smp-tester PASS 8.43 seconds
TestRunner_userchan-tester PASS 6.11 seconds
IncrementalBuild PENDING 0.70 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 482 (98.4%), Failed: 4, Not Run: 4
Failed Test Cases
LL Privacy - Add Device 2 (2 Devices to AL) Failed 0.194 seconds
LL Privacy - Set Flags 1 (Add to RL) Failed 0.160 seconds
LL Privacy - Set Flags 2 (Enable RL) Failed 0.164 seconds
LL Privacy - Start Discovery 2 (Disable RL) Failed 0.200 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
Failed Test Cases
Mesh - Send cancel - 1 Timed out 2.170 seconds
Mesh - Send cancel - 2 Timed out 2.004 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-07 3:46 [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt Kiran K
2025-07-07 3:46 ` [PATCH v1 2/2] Bluetooth: btintel_pcie: Fix alive context state handling Kiran K
2025-07-07 4:27 ` [v1,1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt bluez.test.bot
@ 2025-07-07 6:07 ` Paul Menzel
2025-07-08 12:23 ` K, Kiran
2025-07-08 20:51 ` Luiz Augusto von Dentz
3 siblings, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2025-07-07 6:07 UTC (permalink / raw)
To: Kiran K, Sai Teja Aluvala
Cc: ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, linux-bluetooth
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?
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> 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 */.
> + if (opcode == HCI_OP_RESET || opcode == 0xfc01)
Please define a macro for the magic number.
> 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.
> + ret = -ETIME;
> + goto exit_error;
> }
> }
> hdev->stat.byte_tx += skb->len;
Kind regards,
Paul
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-07 6:07 ` [PATCH v1 1/2] " Paul Menzel
@ 2025-07-08 12:23 ` K, Kiran
2025-07-08 18:27 ` Paul Menzel
0 siblings, 1 reply; 15+ messages in thread
From: K, Kiran @ 2025-07-08 12:23 UTC (permalink / raw)
To: Paul Menzel, Aluvala Sai Teja
Cc: Srivatsa, Ravishankar, Tumkur Narayan, Chethan,
Devegowda, Chandrashekar, linux-bluetooth@vger.kernel.org
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@intel.com>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-08 12:23 ` K, Kiran
@ 2025-07-08 18:27 ` Paul Menzel
2025-07-10 11:05 ` K, Kiran
0 siblings, 1 reply; 15+ messages in thread
From: Paul Menzel @ 2025-07-08 18:27 UTC (permalink / raw)
To: Kiran K, Aluvala Sai Teja
Cc: Ravishankar Srivatsa, Chethan Tumkur Narayan,
Chandrashekar Devegowda, linux-bluetooth
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@intel.com>
>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-08 18:27 ` Paul Menzel
@ 2025-07-10 11:05 ` K, Kiran
0 siblings, 0 replies; 15+ messages in thread
From: K, Kiran @ 2025-07-10 11:05 UTC (permalink / raw)
To: Paul Menzel, Aluvala Sai Teja
Cc: Srivatsa, Ravishankar, Tumkur Narayan, Chethan,
Devegowda, Chandrashekar, linux-bluetooth@vger.kernel.org
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@intel.com>
>>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-07 3:46 [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt Kiran K
` (2 preceding siblings ...)
2025-07-07 6:07 ` [PATCH v1 1/2] " Paul Menzel
@ 2025-07-08 20:51 ` Luiz Augusto von Dentz
2025-07-15 1:14 ` K, Kiran
3 siblings, 1 reply; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2025-07-08 20:51 UTC (permalink / raw)
To: Kiran K
Cc: linux-bluetooth, ravishankar.srivatsa, chethan.tumkur.narayan,
chandrashekar.devegowda, aluvala.sai.teja
Hi Kiran,
On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>
> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
> reset) command. This change fixes the driver to properly wait for the
> alive interrupt.
>
> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
> Signed-off-by: Kiran K <kiran.k@intel.com>
> 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*/
> + if (opcode == HCI_OP_RESET || opcode == 0xfc01)
> 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));
> + ret = -ETIME;
> + goto exit_error;
This should probably go into btintel_pcie_send_sync instead of doing a
post handling as above, also if I read this right then we have to wait
on 2 interrupts when it comes to HCI_Reset and 0xfc01?
btintel_pcie_send_sync already waits on URBD0 this also adds GP0,
having 2 interrupts means proper ordering needs to be enforced,
otherwise if GP0 happens before URBD0 this will probably timeout, if
this is done on purpose let document why we think it is ok to do it.
> }
> }
> hdev->stat.byte_tx += skb->len;
> --
> 2.43.0
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt
2025-07-08 20:51 ` Luiz Augusto von Dentz
@ 2025-07-15 1:14 ` K, Kiran
0 siblings, 0 replies; 15+ messages in thread
From: K, Kiran @ 2025-07-15 1:14 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth@vger.kernel.org, Srivatsa, Ravishankar,
Tumkur Narayan, Chethan, Devegowda, Chandrashekar,
Aluvala Sai Teja
Hi Luiz,
>Subject: Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive
>interrupt
>
>Hi Kiran,
>
>On Sun, Jul 6, 2025 at 11:30 PM Kiran K <kiran.k@intel.com> wrote:
>>
>> Firmware raises an alive interrupt upon receiving the 0xfc01 (Intel
>> reset) command. This change fixes the driver to properly wait for the
>> alive interrupt.
>>
>> Signed-off-by: Sai Teja Aluvala <aluvala.sai.teja@intel.com>
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> 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*/
>> + if (opcode == HCI_OP_RESET || opcode == 0xfc01)
>> 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));
>> + ret = -ETIME;
>> + goto exit_error;
>
>This should probably go into btintel_pcie_send_sync instead of doing a post
>handling as above, also if I read this right then we have to wait on 2 interrupts
>when it comes to HCI_Reset and 0xfc01?
>btintel_pcie_send_sync already waits on URBD0 this also adds GP0, having 2
>interrupts means proper ordering needs to be enforced, otherwise if GP0
>happens before URBD0 this will probably timeout, if this is done on purpose
>let document why we think it is ok to do it.
The URBD0 interrupt is raised immediately after packet reception, while the Alive interrupt is triggered after the firmware initializes and enables the data path for receiving TX packets. This order is guaranteed by firmware.
>
>> }
>> }
>> hdev->stat.byte_tx += skb->len;
>> --
>> 2.43.0
>>
>>
>
>
>--
>Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread