Re: [PATCH net v5 1/4] bnxt_en: return the RING_FREE status to callers

[Joe Damato]

On Sat, Nov 07, 2026 at 07:58:49PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
> 
> 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).
> 
> - [High] The patch makes bnxt_hwrm_rx_ring_free(),
>   bnxt_hwrm_rx_agg_ring_free() and bnxt_hwrm_tx_ring_free() return the…
> - [Medium] The commit message says "All three ring free helpers that send
>   the command discard the value", but four helpers send HWRM_RING_FREE,…
> 
> --
> 
> bnxt_en: return the RING_FREE status to callers
> 
> This commit changes bnxt_hwrm_tx_ring_free(), bnxt_hwrm_rx_ring_free()
> and bnxt_hwrm_rx_agg_ring_free() so they return the status from
> hwrm_ring_free_send_msg() instead of discarding it. No caller acts on
> the returned value yet.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 33c4cacc8ffe3..ea8529c789947 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -7679,50 +7679,55 @@ static int hwrm_ring_free_send_msg(struct bnxt *bp,
> 
> [ ... ]
> 
> > -static void bnxt_hwrm_rx_ring_free(struct bnxt *bp,
> > -				   struct bnxt_rx_ring_info *rxr,
> > -				   bool close_path)
> > +static int bnxt_hwrm_rx_ring_free(struct bnxt *bp,
> > +				  struct bnxt_rx_ring_info *rxr,
> > +				  bool close_path)
> >  {
> >  	struct bnxt_ring_struct *ring = &rxr->rx_ring_struct;
> >  	u32 grp_idx = rxr->bnapi->index;
> >  	u32 cmpl_ring_id;
> > +	int rc;
> >  
> >  	if (ring->fw_ring_id == INVALID_HW_RING_ID)
> > -		return;
> > +		return 0;
> >  
> >  	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
> > -	hwrm_ring_free_send_msg(bp, ring,
> > -				RING_FREE_REQ_RING_TYPE_RX,
> > -				close_path ? cmpl_ring_id :
> > -				INVALID_HW_RING_ID);
> > +	rc = hwrm_ring_free_send_msg(bp, ring,
> > +				     RING_FREE_REQ_RING_TYPE_RX,
> > +				     close_path ? cmpl_ring_id :
> > +				     INVALID_HW_RING_ID);
> >  	ring->fw_ring_id = INVALID_HW_RING_ID;
> >  	bp->grp_info[grp_idx].rx_fw_ring_id = INVALID_HW_RING_ID;
> > +	return rc;
> >  }
> 
> [Severity: High]
> The per-queue callers of these helpers still ignore the returned status,
> and none of the later patches in the series seems to change them. Can
> the queue restart path still release rings that the firmware did not
> free?

The per-queue callers should be fixed in a followup to net-next, IMHO, because
this series addresses a real failure seen in the field on production hosts.

The per-queue callers probably would fail in the same way, but that's not a
failure I've observed so I think that would be a separate followup.

So, I disagree with Sashiko, but if the maintainers feel differently on this
bit, let me know.

> > -static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
> > -				       struct bnxt_rx_ring_info *rxr,
> > -				       bool close_path)
> > +static int bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
> > +				      struct bnxt_rx_ring_info *rxr,
> > +				      bool close_path)
> 
> [ ... ]
> 
> > @@ -7730,14 +7735,15 @@ static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp,
> >  		type = RING_FREE_REQ_RING_TYPE_RX;
> >  
> >  	if (ring->fw_ring_id == INVALID_HW_RING_ID)
> > -		return;
> > +		return 0;
> >  
> >  	cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr);
> > -	hwrm_ring_free_send_msg(bp, ring, type,
> > -				close_path ? cmpl_ring_id :
> > -				INVALID_HW_RING_ID);
> > +	rc = hwrm_ring_free_send_msg(bp, ring, type,
> > +				     close_path ? cmpl_ring_id :
> > +				     INVALID_HW_RING_ID);
> >  	ring->fw_ring_id = INVALID_HW_RING_ID;
> >  	bp->grp_info[grp_idx].agg_fw_ring_id = INVALID_HW_RING_ID;
> > +	return rc;
> >  }
> >  
> >  static void bnxt_hwrm_cp_ring_free(struct bnxt *bp,
> 
> [Severity: Medium]
> The commit message says:
> 
>     All three ring free helpers that send the command discard the value.
> 
> Is that count right? bnxt_hwrm_cp_ring_free() also sends HWRM_RING_FREE,
> and it still returns void:
> 
> 	hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_L2_CMPL,
> 				INVALID_HW_RING_ID);
> 	ring->fw_ring_id = INVALID_HW_RING_ID;
> 
> The inline NQ/L2_CMPL free in bnxt_hwrm_ring_free() also drops its
> status:
> 
> 		if (ring->fw_ring_id != INVALID_HW_RING_ID) {
> 			hwrm_ring_free_send_msg(bp, ring, type,
> 						INVALID_HW_RING_ID);
> 
> By the end of the series, bnxt_hwrm_ring_free() counts stuck rings only
> for TX, RX and AGG failures. If a CP or NQ RING_FREE fails, would
> bnxt_stop_dma() be skipped, with __bnxt_close_nic() going on to free
> that ring's memory in bnxt_free_mem()?

This also seems possible, but this is not what I observed in the field. On
three hosts, the NQ rings failed alongside the RX/TX ring free failures and so
if this series were applied DMA would be stopped.

I suppose it is possible that only the NQ or completion rings fail and this
series do not handle that case.

As with the above feedback from Sashiko: if I understand the requirements for
net correctly (and I'm probably wrong), this series addresses actual bugs seen
on prod workloads.

I'd be happy to send a followup to net-next addressing both points above after
this series is merged.

If the maintainers prefer that these other failure cases (which I did not
observe in real life) are handled in this series, please let me know.