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

[netdev-bot+sashiko@kernel.org]

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?

In bnxt_queue_stop():

	bnxt_hwrm_rx_ring_free(bp, rxr, false);
	bnxt_hwrm_rx_agg_ring_free(bp, rxr, false);
	...
	memcpy(qmem, rxr, sizeof(*rxr));
	bnxt_init_rx_ring_struct(bp, qmem);

	return 0;

If the firmware rejects HWRM_RING_FREE or it times out, ndo_queue_stop
still returns 0. netdev_rx_queue_reconfig() then calls
ndo_queue_mem_free() on the old queue memory.

bnxt_queue_mem_free() then frees the RX buffers, destroys the page pools
and frees the RX and AGG descriptor rings. The firmware ring may still be
live at that point.

With tph_mode set, bnxt_tx_queue_stop() does the same thing for TX:

		bnxt_hwrm_tx_ring_free(bp, txr, true);
		bnxt_hwrm_cp_ring_free(bp, txr->tx_cpr);
		bnxt_free_one_tx_ring_skbs(bp, txr, txr->txq_index);

The helpers also clear ring->fw_ring_id even when rc is non-zero. After
that, the stale firmware ring is no longer visible to the stuck ring
accounting that the later patch "bnxt_en: stop DMA before releasing
rings the firmware did not free" adds to bnxt_hwrm_ring_free(). Would a
later close then skip bnxt_stop_dma() for that ring?

This path can be reached through netdev_rx_queue_restart(), either from
devmem TCP or io_uring zcrx memory provider binding, or from the bnxt
TPH IRQ affinity work.

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

Ring 0 of the CP/NQ set is also registered as the firmware async event
completion ring by bnxt_hwrm_set_async_event_cr() in
bnxt_hwrm_ring_alloc(). The close path does not move that registration.

It is not certain that firmware keeps writing to an unfreed CP/NQ ring
after the TX and RX rings are gone. Async event completions are the
likely source if it does.

On the per-queue paths, bnxt_tx_queue_stop() and bnxt_queue_stop() (with
tph_mode) call the converted helpers right next to
bnxt_hwrm_cp_ring_free(). The CP ring free status is not available to
those callers either.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106191423.4194081-1-joe%40dama.to