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

[Leon Romanovsky <leon@kernel.org>]

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.

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

Thanks

> 
> 
> Michael
> 
> > 
> > Thanks
> >