On Sat, Nov 07, 2026 at 07:58:49PM +0000, netdev-bot+sashiko@kernel.org 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.
quoted
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,
[ ... ]
quoted
-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.
quoted
-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)
[ ... ]
quoted
@@ -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.