|SUCCESS| [v2,4/4] net/ice: split VSI queue count into Rx/Tx

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/4: net/ice: fix Tx queue capacity sizing after TM commit

### Errors

None.

### Warnings

None.

### Info

**Patch Summary**: Fixes an index calculation bug where `ice_get_leaf_level()` returned a relative layer index but was used to index an array requiring an absolute layer index. The fix adds `pf->tm_conf.hidden_layers` to convert the relative index to absolute, correcting the `nb_qps` calculation that determines usable Tx queue count.

**Code Review**:
- The fix is straightforward and correct: converting relative layer index to absolute by adding the hidden layer offset.
- The commit message clearly explains the root cause and impact.
- Tagged with `Fixes:` and `Cc: stable at dpdk.org` appropriately.

---

## PATCH 2/4: net/ice: skip TC validation if hierarchy committed

### Errors

None.

### Warnings

None.

### Info

**Patch Summary**: Fixes a bug where `ice_tx_queue_start()` incorrectly rejected queues after a TM hierarchy was committed. Since TM hierarchies only use TC0, the patch skips the traffic class lookup when a hierarchy is committed (domain is known to be 0) and adds explicit rejection when DCB configures multiple TCs alongside a committed TM hierarchy.

**Code Review**:

1. **Resource leak on error path** (line 856 in ice_rxtx.c):
   The existing code already frees `txq_elem` on both error paths (line 847 and 855), so the new error path at line 861 also correctly frees it before returning. No leak introduced.

2. **Logic correctness**:
   - The `if (!pf->tm_conf.committed)` block moves the TC lookup inside a conditional, which is correct.
   - When TM is committed, `cgd_idx` stays 0 (initialized on line 837), which matches the commit message statement that domain is always 0 for TM.
   - The DCB multi-TC check is appropriate since TM only manages TC0.

3. **Style**:
   - Line wrapping at 80 characters is consistent with DPDK style in this file.
   - Proper indentation maintained.

4. **New field `dcb_num_tcs`**:
   - Initialized to 1 in `ice_dev_configure()` (line 3966).
   - Set to `nb_tc_used` when DCB is configured (line 3997).
   - This tracks the number of TCs correctly for the lifetime of the port.

---

## PATCH 3/4: net/ice: fix Rx queue count reporting after TM commit

### Errors

None.

### Warnings

None.

### Info

**Patch Summary**: Fixes two related bugs caused by `vsi->nb_qps` growing after TM hierarchy commit:
1. `ice_dev_info_get()` reported inflated `max_rx_queues` because it used `nb_qps` which grew to reflect Tx capacity.
2. `ice_vsi_disable_queues_intr()` cleared interrupt registers beyond the VSI's actual queue window, potentially affecting other VSIs.

The fix introduces `vsi->nb_tm_txqs` to track Tx scheduler capacity separately from the fixed VSI queue allocation (`nb_qps`).

**Code Review**:

1. **New field `nb_tm_txqs`**:
   - Initialized to `vsi->nb_qps` during VSI setup (line 1819).
   - Updated in TM commit logic (ice_tm.c lines 862, 865, 892, 898).
   - Used in `ice_dev_info_get()` for `max_tx_queues` (line 4632).

2. **`ice_vsi_disable_queues_intr()` fix**:
   - Changed loop bound from `vsi->nb_qps` to `vsi->nb_used_qps` (line 2928).
   - `nb_used_qps` correctly tracks configured queues, preventing out-of-bounds register writes.
   - This is the correct fix for the interrupt register issue.

3. **Split of concerns**:
   - `nb_qps` remains the fixed VSI allocation.
   - `nb_tm_txqs` tracks Tx capacity that can grow.
   - `nb_used_qps` tracks actually configured queues.
   - This separation is logical and correct.

4. **All callers of `nb_qps` reviewed**:
   - ice_ethdev.c line 4628: `max_rx_queues = vsi->nb_qps` is correct (Rx capacity doesn't grow).
   - ice_ethdev.c line 4632: Now uses `nb_tm_txqs`, correct.
   - ice_tm.c updates: All correctly updated to use `nb_tm_txqs`.

---

## PATCH 4/4: net/ice: split VSI queue count into Rx/Tx

### Errors

None.

### Warnings

None.

### Info

**Patch Summary**: Refactors the queue tracking to use separate `nb_rxqs` and `nb_txqs` variables instead of the symmetric `nb_qps`, completing the separation started in patch 3. Since Rx and Tx queue counts can diverge after TM hierarchy commit, tracking them separately is clearer.

**Code Review**:

1. **Mechanical rename**:
   - `vsi->nb_qps` replaced with `vsi->nb_rxqs` for Rx context (ice_ethdev.c lines 1005-1047).
   - `vsi->nb_tm_txqs` (from patch 3) renamed to `vsi->nb_txqs`.
   - Initial allocation sets both equal: `vsi->nb_txqs = vsi->nb_rxqs` (lines 1819, 1844).

2. **All usages updated**:
   - VSI setup code (lines 1756, 1823, etc.): Uses `nb_rxqs`.
   - `ice_vsi_config_tc_queue_mapping()`: Uses `nb_rxqs` for queue mapping (lines 1005-1047), which is correct since this configures Rx queue to TC association.
   - `ice_dev_info_get()`: Uses `nb_rxqs` for `max_rx_queues` and `nb_txqs` for `max_tx_queues` (lines 4628-4629).
   - TM code (ice_tm.c lines 862, 865, 892, 898): Uses `nb_txqs`.

3. **Consistency**:
   - Both `nb_rxqs` and `nb_txqs` initialized at VSI setup.
   - `nb_txqs` grows with TM hierarchy (ice_tm.c).
   - `nb_rxqs` stays fixed for life of port.
   - This matches the intended semantics.

4. **Comment update**:
   - ice_ethdev.h line 332: Comment updated to reflect split into Rx/Tx.
   - ice_ethdev.c lines 1901-1904: Comment removed as it no longer applies (max Tx queues now explicitly `nb_txqs`).

---

## Summary

**No errors or warnings found across all four patches.**

All patches are correctly implemented and address the stated bugs:

1. **Patch 1**: Fixes layer index calculation for Tx queue capacity.
2. **Patch 2**: Fixes queue start validation when TM hierarchy is committed.
3. **Patch 3**: Fixes Rx queue count reporting and interrupt register clearing.
4. **Patch 4**: Refactors for clarity by splitting symmetric `nb_qps` into separate Rx/Tx counters.

The series is well-structured, with each patch building on the previous one logically. Resource management is correct on all paths. The code follows DPDK style. Documentation in comments and commit messages is clear.