|WARNING| [3/3] net/ice: fix Rx queue count reporting after TM commit

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: net/ice: fix Tx queue capacity sizing after TM commit

### Correctness Issues

**Error: Off-by-one array indexing after fix**

The patch adds `pf->tm_conf.hidden_layers` to convert `ice_get_leaf_level()` from a relative to an absolute layer index:

```c
uint8_t q_lvl = ice_get_leaf_level(pf) + pf->tm_conf.hidden_layers;
uint8_t qg_lvl = q_lvl - 1;
```

However, `qg_lvl` is then used to index `nodes_created_per_level[]` and `hw->layer_info[]`:

```c
pf->main_vsi->nb_qps =
    RTE_MIN(nodes_created_per_level[qg_lvl] * hw->max_children[qg_lvl],
            hw->layer_info[q_lvl].max_device_nodes);
```

If `q_lvl` is now an absolute layer index, then `qg_lvl = q_lvl - 1` is also an absolute index. But the array `nodes_created_per_level[]` is populated by iterating through the TM node list, which uses node->level values. Without seeing the full context of how `nodes_created_per_level[]` is populated, verify that its indices match the absolute layer numbering after this change. If the array is still indexed by relative layers, this creates a mismatch.

**Suggested fix:** Verify that all uses of `qg_lvl` and `q_lvl` after this change consistently use absolute layer indices. The loop that populates `nodes_created_per_level[]` may need to convert node->level to absolute layers as well, or the indexing logic needs adjustment.

---

## Patch 2/3: net/ice: skip TC validation if hierarchy committed

### Correctness Issues

**Error: Missing `pf->dcb_num_tcs` initialization on device close/reconfigure**

The patch initializes `pf->dcb_num_tcs = 1` in `ice_dev_configure()`:

```c
pf->dcb_num_tcs = 1;
```

This field is then conditionally overwritten if DCB is enabled:

```c
if (dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG)
    /* ... */
    pf->dcb_num_tcs = nb_tc_used;
```

However, if the device is closed and reconfigured without DCB in the second configure, the `dcb_num_tcs` value from the first configure could persist. While the code does initialize it to 1 at the start of each configure, this is correct. No issue here.

**Error: TM hierarchy committed flag not cleared on device close**

In `ice_tx_queue_start()`, the code now checks:

```c
if (!pf->tm_conf.committed) {
    /* tc_mapping lookup */
} else if (pf->dcb_num_tcs > 1) {
    PMD_DRV_LOG(ERR, "TM hierarchy is not supported together with multi-TC DCB");
    return -EINVAL;
}
```

The `pf->tm_conf.committed` flag is set in patch 1's context (via `commit_new_hierarchy()`), but it's unclear from this patch alone whether the flag is cleared on device close/reset. If an application configures a device with TM, closes it, reconfigures with DCB (multi-TC), and starts queues, the stale `committed` flag would bypass the tc_mapping lookup and potentially hit the multi-TC rejection even though no TM hierarchy is active.

**Suggested fix:** Verify that `pf->tm_conf.committed` is cleared in `ice_dev_close()` or `ice_dev_reset()`. If not, add the clear operation.

---

**Error: Resource leak on early return path**

In `ice_tx_queue_start()`, when the multi-TC DCB check fails:

```c
} else if (pf->dcb_num_tcs > 1) {
    PMD_DRV_LOG(ERR, "TM hierarchy is not supported together with multi-TC DCB");
    rte_free(txq_elem);
    return -EINVAL;
}
```

This path correctly frees `txq_elem`. However, trace through the earlier code before this addition. The new conditional block:

```c
if (!pf->tm_conf.committed) {
    /* Mirror RXQ<->CGD association to TXQ<->CGD */
    for (int i = 0; i < ICE_MAX_TRAFFIC_CLASS; i++) {
        /* ... */
        if (q_base <= tx_queue_id && tx_queue_id < q_base + q_range)
            break;
        cgd_idx++;
    }
    if (cgd_idx >= ICE_MAX_TRAFFIC_CLASS) {
        PMD_DRV_LOG(ERR, "Bad queue mapping configuration");
        rte_free(txq_elem);
        return -EINVAL;
    }
}
```

Both error paths correctly free `txq_elem` before returning. No leak introduced by this patch.

---

### Code Style

**Info: Loop variable declaration style**

The loop uses C89 style:

```c
for (int i = 0; i < ICE_MAX_TRAFFIC_CLASS; i++) {
```

This is C99 declaration-at-use, which is acceptable per the guidelines. No issue.

---

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

### Correctness Issues

**Error: Potential uninitialized `vsi->nb_tm_qps` on non-main VSI types**

The patch adds initialization in `ice_setup_vsi()`:

```c
case ICE_VSI_PF:
    /* ... */
    vsi->nb_tm_qps = vsi->nb_qps;
    break;
case ICE_VSI_CTRL:
    /* ... */
```

The initialization `vsi->nb_tm_qps = vsi->nb_qps` is only added for `ICE_VSI_PF`. If `ice_dev_info_get()` can be called on other VSI types, `vsi->nb_tm_qps` will be uninitialized (or zero if the struct was zero-allocated).

Looking at `ice_dev_info_get()`:

```c
struct ice_vsi *vsi = pf->main_vsi;
/* ... */
dev_info->max_tx_queues = vsi->nb_tm_qps;
```

The `main_vsi` is typically the PF VSI, so this is likely safe. However, verify that `main_vsi` is always of type `ICE_VSI_PF`. If other VSI types can be main_vsi, they need `nb_tm_qps` initialized as well.

**Suggested fix:** Either initialize `vsi->nb_tm_qps = vsi->nb_qps` for all VSI types in `ice_setup_vsi()`, or document that `nb_tm_qps` is only valid for PF VSIs and add a defensive check in `ice_dev_info_get()`.

---

**Info: Struct field initialization order**

The new field `nb_tm_qps` is initialized after the VSI queue setup in `ice_setup_vsi()`. This is acceptable, but placing it immediately after the assignment of `vsi->nb_qps` (likely earlier in the function) would make the relationship clearer.

---

### Documentation

**Warning: Missing release notes entry for API change**

The patch changes the behavior of `ice_dev_info_get()`, which fills `rte_eth_dev_info`. While the signature doesn't change, the semantics do: `max_tx_queues` may now differ from its previous value after a TM hierarchy commit. This is an observable behavior change for applications querying device capabilities.

**Suggested fix:** Add a release note in `doc/guides/rel_notes/release_XX_XX.rst` (current release) under "Fixed Issues" describing that `max_tx_queues` now correctly reflects Tx scheduler capacity after TM commit.

---

**Warning: Missing documentation of `nb_tm_qps` field**

The new `nb_tm_qps` field in `struct ice_vsi` lacks a comment explaining its purpose and relationship to `nb_qps`.

**Suggested fix:** Add a comment:

```c
uint16_t nb_qps;         /* Number of queue pairs VSI can occupy */
uint16_t nb_tm_qps;      /* Tx queue capacity after TM hierarchy commit (may exceed nb_qps) */
uint16_t nb_used_qps;    /* Number of queue pairs VSI uses */
```

---

## Summary

### Errors (must fix)
1. **Patch 1**: Verify `qg_lvl` indexing is consistent with absolute layer numbering after adding `hidden_layers` offset
2. **Patch 2**: Verify `pf->tm_conf.committed` is cleared on device close/reset to avoid stale flag issues
3. **Patch 3**: Verify `vsi->nb_tm_qps` is initialized for all VSI types that can be `main_vsi`, or add defensive checks

### Warnings (should fix)
1. **Patch 3**: Add release notes entry for observable `max_tx_queues` behavior change
2. **Patch 3**: Add comment documenting `nb_tm_qps` field purpose

### Info
1. **Patch 3**: Consider moving `nb_tm_qps` initialization closer to `nb_qps` assignment for clarity