|WARNING| net/bnxt: remove VMDq support

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169867

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-21

# DPDK Patch Review

## Summary

This patch removes VMDq support from the bnxt driver. The removal is mostly clean, but there are a few issues that need attention.

---

## Errors

### 1. Incorrect dev_info fields left in place

**File:** `drivers/net/bnxt/bnxt_ethdev.c`

```c
dev_info->vmdq_pool_base = 0;
dev_info->vmdq_queue_base = 0;
```

These two VMDq-related dev_info fields are still being set to 0, but they should also be removed entirely when VMDq support is removed. Leaving them suggests partial VMDq support still exists.

**Fix:** Remove these two lines along with the `max_vmdq_pools` and `vmdq_queue_num` assignments, or document why they must remain.

---

## Warnings

### 1. Incomplete feature removal documentation

**File:** `doc/guides/rel_notes/release_26_11.rst`

The release notes mention that `max_vmdq_pools` and `vmdq_queue_num` are now 0, but don't mention:
- The removed multi-queue modes (`RTE_ETH_MQ_RX_VMDQ_DCB_RSS`, `RTE_ETH_MQ_RX_VMDQ_RSS`, etc.)
- The impact on applications using VMDq with this driver
- Whether the `RTE_ETH_VMDQ_ACCEPT_*` flags in `rte_pmd_bnxt_set_vf_rxmode()` still have an effect (the commit message says they remain, but the release notes don't clarify)

**Suggestion:** Expand the release notes to cover:
- Which multi-queue modes are no longer supported
- Clarify that `RTE_ETH_VMDQ_ACCEPT_*` flags remain functional (if that's the intended behavior per your commit message)

### 2. Logic change in bnxt_mq_rx_configure requires validation

**File:** `drivers/net/bnxt/bnxt_rxq.c`

**Before:**
```c
if (dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_DCB_RSS) {
    switch (dev_conf->rxmode.mq_mode) {
    case RTE_ETH_MQ_RX_VMDQ_RSS:
    case RTE_ETH_MQ_RX_VMDQ_ONLY:
    case RTE_ETH_MQ_RX_VMDQ_DCB_RSS:
        /* VMDq path */
        pools = conf->nb_queue_pools;
        ...
        break;
    case RTE_ETH_MQ_RX_RSS:
        pools = bp->rx_cosq_cnt ? bp->rx_cosq_cnt : 1;
        break;
    default:
        return -EINVAL;
    }
} else if (!dev_conf->rxmode.mq_mode) {
    pools = bp->rx_cosq_cnt ? bp->rx_cosq_cnt : pools;
}
```

**After:**
```c
if (!dev_conf->rxmode.mq_mode ||
    dev_conf->rxmode.mq_mode == RTE_ETH_MQ_RX_RSS)
    pools = bp->rx_cosq_cnt ? bp->rx_cosq_cnt : pools;
```

The new code treats `RTE_ETH_MQ_RX_RSS` the same as `!mq_mode` (both use `pools` as fallback when `rx_cosq_cnt` is 0). The original code set `pools = 1` for RSS mode but `pools` (uninitialized value, defaults to 1) for the no-mq-mode case. This appears correct but should be verified:
- When `mq_mode == RTE_ETH_MQ_RX_RSS` and `rx_cosq_cnt == 0`, the original code would set `pools = 1`; the new code uses the initialized value `pools = 1`.
- The behaviors match, but confirm that collapsing these cases is intentional and correct.

**Also note:** The original code returned `-EINVAL` for unsupported mq_modes. The new code silently ignores unsupported modes (e.g., `RTE_ETH_MQ_RX_DCB`, `RTE_ETH_MQ_RX_VMDQ_DCB`), falling through to use `pools = 1`. This is a behavior change.

**Suggestion:** Add validation to reject unsupported mq_modes explicitly, or document that unsupported modes now fall back to single-pool operation.

### 3. Removed error path handling may expose issues

**File:** `drivers/net/bnxt/bnxt_rxq.c`

The removed `switch` statement had a `default: return -EINVAL;` that prevented unsupported mq_modes from proceeding. With that removed, the function now accepts any mq_mode value and tries to configure queues. If an application passes an unsupported mode (e.g., `RTE_ETH_MQ_RX_DCB`), the driver will attempt configuration with `pools = 1` rather than failing.

**Suggestion:** Either:
1. Add explicit validation at the start of `bnxt_mq_rx_configure()` to reject unsupported modes, or
2. Document that unsupported modes are now silently treated as single-pool configuration.

---

## Info

### 1. Unused variable removal opportunity

**File:** `drivers/net/bnxt/bnxt_ethdev.c`

The variable declarations:
```c
uint16_t max_vnics, i, j, vpool, vrxq;
```

After your changes, `max_vnics`, `i`, `j`, `vpool`, and `vrxq` are no longer used in `bnxt_dev_info_get_op()`. Consider removing them in a follow-up cleanup or in this patch.

### 2. Comment removal suggests incomplete cleanup

**File:** `drivers/net/bnxt/bnxt_hwrm.c`

The removed comment:
```c
//TODO: Is there a better way to add VLANs to each VNIC in case of VMDQ
```

Used C++ style `//` comment, which is acceptable in C99+ but DPDK prefers `/* */` style. Not worth flagging on code being deleted, but note for future patches.

### 3. Coding style note on removed logic

**File:** `drivers/net/bnxt/bnxt_hwrm.c`

The removed code included:
```c
if ((dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) &&
    conf->pool_map[j].pools & (1UL << j)) {
```

The `1UL << j` pattern is acceptable here (32-bit shift on 32-bit mask), but if `pools` were a 64-bit field, this should have been `UINT64_C(1) << j` or `RTE_BIT64(j)`. Since the code is being removed, no action needed, but keep this in mind for similar patterns elsewhere.

---

## Conclusion

**Must fix:**
- Remove or justify the remaining `vmdq_pool_base` and `vmdq_queue_base` assignments

**Should fix:**
- Add explicit validation for unsupported mq_modes or document the fallback behavior
- Expand release notes to clarify the scope of the removal
- Clean up unused variables in `bnxt_dev_info_get_op()`

The core logic of the VMDq removal appears correct, but the error handling change and the leftover dev_info fields need attention.