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

[Leon Romanovsky <leon@kernel.org>]

On Thu, Jul 03, 2025 at 06:23:14PM +0000, Michael Margolin wrote:
> Add command id 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 | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Please don't forget to add changelogs.

> 
> diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c
> index bafd210dd43e..e6377602a9c4 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.
>   */

<...>

>  	reinit_completion(&comp_ctx->wait_event);
>  
> @@ -557,17 +559,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 (id: %d, 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->cmd_id, 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 (id: %d, 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->cmd_id, aq->sq.pc, aq->sq.cc,
> +				aq->cq.cc);

I have very strong feeling that you don't really use these prints in real life.

For example, comp_ctx->cmd_id is printed with %d, while code and comment
around cmd_id in __efa_com_submit_admin_cmd() suggests that it needs to be 0x%X.

It has a lot of information separated to LSB and MSB bits which are not readable
while printing with %d.

You are also printing comp_ctx->status, which is clear from if/else section.

So no, I don't buy this claim for "additional debug information", while
existing is not used.

Thanks

>  
>  		clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
>  		err = -ETIME;
> -- 
> 2.47.1
>