Re: [PATCH v1 1/2] Bluetooth: btintel_pcie: Make driver wait for alive interrupt

[Paul Menzel <pmenzel@xxxxxxxxxxxxx>]

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

> +		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