|SUCCESS| [4/4] net/bnxt: fix RSS hash mode configuration for VFs

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164909

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-04

Reviewing the patch series for DPDK coding standards and contribution guidelines.

commit 20260603175137.1990204-2-Mohammad-Shuab.Siddique at broadcom.com
Author: Ajit Khaparde <ajit.khaparde at broadcom.com>

net/bnxt: modify check for short Tx BDs

There is no need to use the long BDs for transmits
where only checksum offload is needed.
Modify the check for long BD and use long BDs only in cases
where TSO and other offloads are requested.

> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> index 27758898b0..7ef5b15ae8 100644
> --- a/drivers/net/bnxt/bnxt_txr.c
> +++ b/drivers/net/bnxt/bnxt_txr.c
> @@ -111,8 +111,7 @@ int bnxt_init_tx_ring_struct(struct bnxt_tx_queue *txq, unsigned int socket_id)
>  static bool
>  bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>  {
> -	if (tx_pkt->ol_flags & (RTE_MBUF_F_TX_TCP_SEG | RTE_MBUF_F_TX_TCP_CKSUM |
> -				RTE_MBUF_F_TX_UDP_CKSUM | RTE_MBUF_F_TX_IP_CKSUM |
> +	if (tx_pkt->ol_flags & (RTE_MBUF_F_TX_TCP_SEG |
>  				RTE_MBUF_F_TX_VLAN | RTE_MBUF_F_TX_OUTER_IP_CKSUM |
>  				RTE_MBUF_F_TX_TUNNEL_GRE | RTE_MBUF_F_TX_TUNNEL_VXLAN |
>  				RTE_MBUF_F_TX_TUNNEL_GENEVE | RTE_MBUF_F_TX_IEEE1588_TMST |

This looks reasonable for the stated goal of avoiding long BDs when only
basic checksums are needed.

---

commit 20260603175137.1990204-3-Mohammad-Shuab.Siddique at broadcom.com
Author: Ajit Khaparde <ajit.khaparde at broadcom.com>

net/bnxt: fix QP resource count in backing store config

The driver is not passing the QP1 count while
configuring backing store for QP type.
This can result in a smaller set of entries allocated by the
driver with the firmware.

The number of entries is provided by the firmware as a
part of backing store qcaps v2 HWRM command.

> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index b677f9491d..7d70d0f3ec 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -5404,11 +5404,11 @@ int bnxt_alloc_ctx_pg_tbls(struct bnxt *bp)
>  			if (ctxm->type == HWRM_FUNC_BACKING_STORE_CFG_V2_INPUT_TYPE_CQ)
>  				entries = ctxm->cq_l2_entries;
>  			else if (ctxm->type == HWRM_FUNC_BACKING_STORE_CFG_V2_INPUT_TYPE_QP)
> -				entries = ctxm->qp_l2_entries;
> +				entries = ctxm->qp_l2_entries + ctxm->qp_qp1_entries;
>  			else if (ctxm->type == HWRM_FUNC_BACKING_STORE_CFG_V2_INPUT_TYPE_MRAV)
>  				entries = ctxm->mrav_av_entries;
>  			else if (ctxm->type == HWRM_FUNC_BACKING_STORE_CFG_V2_INPUT_TYPE_TIM)
> -				entries = ctx2->qp_l2_entries;
> +				entries = ctx2->qp_l2_entries + ctx2->qp_qp1_entries;
>  			entries = clamp_t(uint32_t, entries, ctxm->min_entries,
>  					  ctxm->max_entries);
>  			ctx_pg[i].entries = entries;

Could the addition of qp_l2_entries and qp_qp1_entries overflow the
uint32_t type of the entries variable? If qp_l2_entries and
qp_qp1_entries are both close to UINT32_MAX, does this code overflow
before the clamp_t operation?

---

commit 20260603175137.1990204-4-Mohammad-Shuab.Siddique at broadcom.com
Author: Zoe Cheimets <zoe.cheimets at broadcom.com>

net/bnxt: remove implicit integer sign-extension

In bnxt_ring.c, the result on line 389 was auto-sign extended by
the compiler because the arithmetic result is an int, but the
dpi_offset is uint64_t. Fix by casting the result to uint64_t
before the multiplication forces extension. To ensure that a
negative integer is not being cast to uint64_t, add a check in
the if-statement.

> diff --git a/drivers/net/bnxt/bnxt_ring.c b/drivers/net/bnxt/bnxt_ring.c
> index ccca779b97..579b73d2ce 100644
> --- a/drivers/net/bnxt/bnxt_ring.c
> +++ b/drivers/net/bnxt/bnxt_ring.c
> @@ -385,9 +385,10 @@ void bnxt_set_db(struct bnxt *bp,
>  		db->doorbell = (char *)bp->doorbell_base + db_offset;
>  
>  		if (bp->fw_cap & BNXT_FW_CAP_MULTI_DB &&
> -				dpi != BNXT_PRIVILEGED_DPI) {
> -			dpi_offset = (dpi - bp->nq_dpi_start) *
> -					bp->db_page_size;
> +		    dpi != BNXT_PRIVILEGED_DPI &&
> +		    dpi >= bp->nq_dpi_start) {
> +			dpi_offset = (uint64_t)(dpi - bp->nq_dpi_start) *
> +							bp->db_page_size;

The indentation appears inconsistent here. In the modified code, the
multiplication line uses spaces for indentation that do not align with
the start of the expression on the previous line. According to DPDK
style, continuation lines should be indented to align with the opening
parenthesis or use consistent tab indentation.

Could the multiplication of the casted subtraction result with
bp->db_page_size overflow uint64_t? What are the expected ranges of
these values?

---

commit 20260603175137.1990204-5-Mohammad-Shuab.Siddique at broadcom.com
Author: Mohammad Shuab Siddique <mohammad-shuab.siddique at broadcom.com>

net/bnxt: fix RSS hash mode configuration for VFs

Fixed VFs attempting global RSS configuration which is not
permitted by firmware. VFs (including trusted VFs) must use
per-VNIC RSS configuration with actual vnic_id and rss_ctx_idx
values.

> diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
> index 0c82935de9..afc948ac29 100644
> --- a/drivers/net/bnxt/bnxt_hwrm.c
> +++ b/drivers/net/bnxt/bnxt_hwrm.c
> @@ -2970,8 +2970,22 @@ bnxt_hwrm_vnic_rss_cfg_hash_mode_p5(struct bnxt *bp, struct bnxt_vnic_info *vnic
>  		req.hash_mode_flags = BNXT_HASH_MODE_INNERMOST;
>  	else
>  		req.hash_mode_flags = vnic->hash_mode;
> -	req.vnic_id = rte_cpu_to_le_16(BNXT_DFLT_VNIC_ID_INVALID);
> -	req.rss_ctx_idx = rte_cpu_to_le_16(BNXT_RSS_CTX_IDX_INVALID);
> +
> +	/* VFs must use actual vnic_id for per-VNIC configuration.
> +	 * PFs can use INVALID vnic_id for global configuration.
> +	 * This is because VFs don't have permission to configure
> +	 * global hash mode, even if they're trusted.
> +	 */
> +	if (BNXT_VF(bp)) {
> +		req.vnic_id = rte_cpu_to_le_16(vnic->fw_vnic_id);
> +		req.rss_ctx_idx = rte_cpu_to_le_16(vnic->fw_grp_ids[0]);
> +		PMD_DRV_LOG_LINE(DEBUG, "VF using per-VNIC RSS config (vnic_id=%u)",
> +				vnic->fw_vnic_id);

Does this code validate that vnic->fw_grp_ids is non-NULL before
accessing fw_grp_ids[0]? Could this dereference fail if the VF
configuration is incomplete?

> +	} else {
> +		req.vnic_id = rte_cpu_to_le_16(BNXT_DFLT_VNIC_ID_INVALID);
> +		req.rss_ctx_idx = rte_cpu_to_le_16(BNXT_RSS_CTX_IDX_INVALID);
> +		PMD_DRV_LOG_LINE(DEBUG, "PF using global RSS config");
> +	}
>  
>  	PMD_DRV_LOG_LINE(DEBUG, "RSS CFG: Hash level %d", req.hash_mode_flags);
>  	rc = bnxt_hwrm_send_message(bp, &req, sizeof(req),