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

["Margolin, Michael" <mrgolin@amazon.com>]

On 7/7/2025 9:28 AM, Leon Romanovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Sun, Jul 06, 2025 at 07:32:05PM +0300, Margolin, Michael wrote:
>> On 7/6/2025 10:25 AM, Leon Romanovsky wrote:
>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> On Thu, Jul 03, 2025 at 06:23:14PM +0000, Michael Margolin wrote:
>>>>         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.
>> What do you mean by that?!?
> If you take a close look on the prints, you will see the reasons.
> For example, you print comp_ctx->status which can be only 0 or 1,
> while it is already clear what its value from the print itself.
>
>> These errors are extremely rare and are not manually reproducible, that's
>> why we want to collect as much information as we can when it happens.
> Do it out-of-tree, there is no need in upstream code for internal debug
> sessions.

It's not for internal debug, it is used in production. Why would I 
upstream internal debug prints?

>> I'm less bothered by the format as long as we have the info we need.
> Like I said, it is clear that you never actually relied on this information.
> Better if you completely delete these prints and keep them out-of-tree.

Not sure that I follow, can you please elaborate on what "relying" means 
from your POV?

Michael