Re: [PATCH for-next] RDMA/efa: Extend admin timeout error print

[Gal Pressman <gal.pressman@linux.dev>]

On 02/07/2025 18:20, Michael Margolin wrote:
> Add command context index to the printed message for additional debug
> information.
> 
> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa_com.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c
> index bafd210dd43e..f1e88ee89bb8 100644
> --- a/drivers/infiniband/hw/efa/efa_com.c
> +++ b/drivers/infiniband/hw/efa/efa_com.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>  /*
> - * Copyright 2018-2024 Amazon.com, Inc. or its affiliates. All rights reserved.
> + * Copyright 2018-2025 Amazon.com, Inc. or its affiliates. All rights reserved.
>   */
>  
>  #include "efa_com.h"
> @@ -557,17 +557,19 @@ static int efa_com_wait_and_process_admin_cq_interrupts(struct efa_comp_ctx *com
>  		if (comp_ctx->status == EFA_CMD_COMPLETED)
>  			ibdev_err_ratelimited(
>  				aq->efa_dev,
> -				"The device sent a completion but the driver didn't receive any MSI-X interrupt for admin cmd %s(%d) status %d (ctx: 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
> +				"The device sent a completion but the driver didn't receive any MSI-X interrupt for admin cmd %s(%d) status %d (ctx[%d]: 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
>  				efa_com_cmd_str(comp_ctx->cmd_opcode),
>  				comp_ctx->cmd_opcode, comp_ctx->status,
> -				comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
> +				comp_ctx - aq->comp_ctx, comp_ctx, aq->sq.pc,
> +				aq->sq.cc, aq->cq.cc);
>  		else
>  			ibdev_err_ratelimited(
>  				aq->efa_dev,
> -				"The device didn't send any completion for admin cmd %s(%d) status %d (ctx 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
> +				"The device didn't send any completion for admin cmd %s(%d) status %d (ctx[%d]: 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
>  				efa_com_cmd_str(comp_ctx->cmd_opcode),
>  				comp_ctx->cmd_opcode, comp_ctx->status,
> -				comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
> +				comp_ctx - aq->comp_ctx, comp_ctx, aq->sq.pc,
> +				aq->sq.cc, aq->cq.cc);
>  
>  		clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
>  		err = -ETIME;

Arguably, there is no point in keeping the comp_ctx pointer print, as
you have nothing to compare the hashed pointer to. It could've been
useful if the pointer was also printed when the command is submitted,
but it isn't, so it's probably better to just remove it and keep the
index you added.

A better alternative might be storing the cmd_id inside the comp_ctx and
printing it instead, it contains more information.