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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jul 07, 2025 at 12:51:40PM +0300, Margolin, Michael wrote:
> 
> 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?

It is used in internal cloud for the NICs not available to the rest of
the world. So yes, it is your internal debug print.

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

"relied" == "used"

> 
> Michael
>