Re: [PATCH net v5 2/4] bnxt_en: check HWRM response if completion never arrives

[Joe Damato <joe@dama.to>]

On Sat, Nov 07, 2026 at 07:58:51PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The new fallback for a missing completion is in the generic
>   completion-ring wait branch of __hwrm_send()…
> - [Medium] bnxt_hwrm_rx_ring_reset()
>   (drivers/net/ethernet/broadcom/bnxt/bnxt.c:14467) sends…
> - [Medium] In the new fallback for a missing completion in __hwrm_send(),
>   dma_rmb() runs once, before the loads of seq_id, resp_len and the…
> - [Low] The commit message says the valid byte need not be polled because
>   'Several seconds are spent waiting for the completion, so a response…
> - [Low] BNXT_HWRM_CTX_SILENT is documented in bnxt_hwrm.h as 'squelch
>   firmware errors', and hwrm_err() turns messages into netdev_dbg when…
> 
> --
> 
> bnxt_en: check HWRM response if completion never arrives
> 
> This changes __hwrm_send() so that when a command sent over a completion
> ring never has its completion consumed by NAPI, the DMA response is
> checked before giving up. If resp_len is non-zero, the sequence id
> matches and the valid byte is set, the command is treated as completed
> and falls through to the normal error_code handling.
> 
> > Several seconds are spent waiting for the completion, so a response that
> > was written at all is complete by the time the wait gives up. There is no
> > need to poll for the valid byte here the way the polling path below has
> > to, where the poll is for a non-zero length and the valid byte at the end
> > of the message may still be on its way.
> 
> [Severity: Low]
> Is the "several seconds" part accurate? __hwrm_req_init() sets the
> default timeout like this:

I can fix the commit message if I respin (I'm still going through the feedback
so idk yet). I don't think it's worth a respin if this is the only real issue,
but I suspect there are real issues that will require a respin.

> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> > index 5bfabdca7d0e2..4feba90f0bf68 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> > @@ -582,11 +582,36 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
> >  		}
> >  
> >  		if (READ_ONCE(token->state) != BNXT_HWRM_COMPLETE) {
> > -			hwrm_err(bp, ctx, "Resp cmpl intr err msg: 0x%x\n",
> > -				 req_type);
> > -			goto exit;
> > +			__le16 resp_seq_id;
> > +			u8 valid_byte = 0;
> > +
> > +			/* The completion ring entry was not delivered for
> > +			 * some reason. It might be possible that the command
> > +			 * was carried out even without a completion being
> > +			 * posted. Check the response before giving up and log
> > +			 * the state.
> > +			 */
> > +			dma_rmb();
> > +			resp_seq_id = READ_ONCE(ctx->resp->seq_id);
> > +			len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len));
> > +			if (len && resp_seq_id == ctx->req->seq_id)
> > +				valid_byte = *((u8 *)ctx->resp + len - 1);
> 
> [Severity: Medium]
> Is the dma_rmb() in the right place here? It runs once, before the
> seq_id, resp_len and valid byte loads. Nothing orders the loads of the
> rest of the response after the valid byte is seen.
> 
> Those later reads include ctx->resp->error_code in the netdev_warn()
> below. They also include the common tail of __hwrm_send():
> 
>     *valid = 0;
>     rc = le16_to_cpu(ctx->resp->error_code);
> 
> and any response fields that callers read after hwrm_req_send() returns.
> 
> Firmware writes the valid byte last. The usual consumer pattern is to
> see the valid byte, then dma_rmb(), then read the payload. The address of
> the valid byte load depends on len, but the error_code load depends on
> nothing.
> 
> On weakly ordered CPUs such as arm64 or POWER, could the error_code load
> complete before the valid byte load while firmware is still writing the
> response at the timeout boundary? The response buffer starts zeroed, so a
> stale error_code would read as 0 and a failed command would look like a
> success.
> 
> For HWRM_RING_FREE, would that hide a failure from the error handling
> added later in the series, which stops DMA only when ring free fails?

I think this is true; the dma_rmb needs to come later so I'm going to mark
this as CR, but will reply to the rest of the feedback to make sure I figure
out what will come in the next revision.

> The polling path has the same gap after its valid loop. This patch adds
> a new consumer that relies on the completeness argument in the commit
> message.

This existed before my series, so I feel like fixing this for polling maybe is
code for net-next (?)

> > +
> > +			if (!valid_byte) {
> > +				hwrm_err(bp, ctx,
> > +					 "Resp cmpl intr err msg: 0x%x len:%d seq:0x%x/0x%x\n",
> > +					 req_type, len,
> > +					 le16_to_cpu(resp_seq_id),
> > +					 le16_to_cpu(ctx->req->seq_id));
> > +				goto exit;
> > +			}
> 
> [Severity: Medium]
> This fallback is in the generic completion ring branch, so it covers
> every request that sets cmpl_ring, not only HWRM_RING_FREE. Does it change
> the result of the ethtool interrupt self test?
> 
> bnxt_hwrm_selftest_irq() sends HWRM_SELFTEST_IRQ with the completion ring
> set:
> 
>     req->cmpl_ring = cpu_to_le16(cmpl_ring);
>     return hwrm_req_send(bp, req);
> 
> The test catches a broken interrupt path only because hwrm_req_send()
> fails when NAPI never consumes the HWRM_DONE.
> 
> Firmware still writes the DMA response for the command. If the interrupt
> or completion never reaches NAPI, this path now falls through and returns
> error_code, which is 0:
> 
> ethtool -t <dev> offline
>   bnxt_self_test()
>     bnxt_test_irq()
>       bnxt_hwrm_selftest_irq()
>         __hwrm_send()
> 
> bnxt_self_test() marks the test failed only here:
> 
>     if (rc || bnxt_test_irq(bp)) {
>         buf[BNXT_IRQ_TEST_IDX] = 1;
>         etest->flags |= ETH_TEST_FL_FAILED;
>     }
> 
> Would Interrupt_test then pass on a device whose interrupt delivery is
> broken, with only a netdev_warn in dmesg? The commit message mentions
> only the ring free case.

Yea this is legit. I will fix this in the next revision.

> [Severity: Medium]
> bnxt_hwrm_rx_ring_reset() raises a similar question. It sends
> HWRM_RING_RESET with cmpl_ring set to the RX ring's own completion ring,
> using hwrm_req_send_silent().
> 
> Before this patch, a lost completion returned -EBUSY. bnxt_rx_ring_reset()
> then fell back to bnxt_reset_task(), which also re-initializes NAPI and
> IRQs:
> 
>     rc = bnxt_hwrm_rx_ring_reset(bp, i);
>     if (rc) {
>         ...
>         bnxt_reset_task(bp, true);
>         break;
>     }
>     bnxt_free_one_rx_ring_skbs(bp, rxr);
>     ...
>     rxr->bnapi->in_reset = false;
> 
> Now the same case can return 0 and take the in-place recovery path.
> 
> Commit 74608fc98d28 sent the response through the ring's own completion
> ring for ordering: "The response will be the last entry in the ring and
> we can free the ring after getting the response." When NAPI consumes
> HWRM_DONE, that shows the earlier RX completions on the ring were
> discarded while in_reset was set.
> 
> Without that, can unconsumed completions stay on the cp ring and be
> processed by bnxt_rx_pkt() after in_reset is cleared and the buffers are
> replaced?
> 
> If the completion was lost because the ring's interrupt or NAPI is stuck,
> would the in-place path leave the ring broken, where the global reset
> would have recovered it?
> 
> What happens once stale entries are processed depends on runtime state,
> so this one is less certain.

I think this is legit, too.
 
> > +			netdev_warn(bp->dev,
> > +				    "Resp cmpl intr not delivered, msg: 0x%x completed anyway (len:%d valid:0x%x err:0x%x)\n",
> > +				    req_type, len, valid_byte,
> > +				    le16_to_cpu(ctx->resp->error_code));
> 
> [Severity: Low]
> Should this respect BNXT_HWRM_CTX_SILENT? The flag is documented as
> "squelch firmware errors". The failure side of this branch uses
> hwrm_err(), which drops to netdev_dbg for silent contexts.
> 
> This netdev_warn() always prints the firmware error_code. The common tail
> of __hwrm_send() suppresses the same error_code for silent callers:
> 
>     if (rc == HWRM_ERR_CODE_BUSY && !(ctx->flags & BNXT_HWRM_CTX_SILENT))
>         netdev_warn(bp->dev, "FW returned busy, hwrm req_type 0x%x\n",
>                     req_type);
>     else if (rc && rc != HWRM_ERR_CODE_PF_UNAVAILABLE)
>         hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n",
>                  req_type, le16_to_cpu(ctx->req->seq_id), rc);
> 
> bnxt_hwrm_rx_ring_reset() uses hwrm_req_send_silent() and goes through
> this path. A lost completion there would now log a warning that includes
> the firmware error code.

OK, I can make the warning respect silent callers, so I'll fix this in the
next revision.

pw-bot: cr