|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

**Error: Index conversion may be incorrect**

The patch adds `pf->tm_conf.hidden_layers` to convert a relative layer index to an absolute one:

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

However, the subsequent line:
```c
uint8_t qg_lvl = q_lvl - 1;
```

is used to index `nodes_created_per_level[]` and `hw->layer_info[]` arrays. The patch does not verify that `q_lvl >= 1` before the subtraction, which could cause `qg_lvl` to underflow if `ice_get_leaf_level(pf) + pf->tm_conf.hidden_layers` equals 0.

While this may be prevented by design constraints elsewhere in the code, the lack of a bounds check on a value used as an array index is a potential out-of-bounds access.

**Suggested fix:**
Add a validation check:
```c
uint8_t q_lvl = ice_get_leaf_level(pf) + pf->tm_conf.hidden_layers;
if (q_lvl == 0) {
	PMD_DRV_LOG(ERR, "Invalid queue level calculation");
	return -EINVAL;
}
uint8_t qg_lvl = q_lvl - 1;
```

### Style

No style issues identified.

---

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

### Correctness

**Error: Missing rte_free on new error path**

The patch adds a new error case in `ice_tx_queue_start()`:

```c
} else if (pf->dcb_num_tcs > 1) {
	/* TM only manages the TC0 scheduler subtree. */
	PMD_DRV_LOG(ERR, "TM hierarchy is not supported together with multi-TC DCB");
	rte_free(txq_elem);
	return -EINVAL;
}
```

This correctly frees `txq_elem` on the error path, matching the existing pattern. No leak here.

**Warning: Initialization of dcb_num_tcs**

The patch initializes `pf->dcb_num_tcs = 1` in `ice_dev_configure()` before the DCB configuration block. However, `ice_dev_configure()` can be called multiple times during device reconfiguration. If DCB was previously configured with multiple TCs and then reconfigured without DCB, the initial assignment to 1 is correct. But if the DCB configuration block (`if (dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_DCB_FLAG)`) fails after updating `pf->dcb_num_tcs`, the field could be left in an inconsistent state.

The existing code does not show what happens on reconfiguration errors, so this may be acceptable if `ice_dev_configure()` always returns to a consistent state on failure. However, the pattern of setting `pf->dcb_num_tcs = 1` unconditionally at the start of configure suggests it's safe.

### Style

**Info: Comment formatting**

The comment "/* TM only manages the TC0 scheduler subtree. */" is formatted correctly but could be more specific about the consequence:

```c
/* TM hierarchy only supports TC0. Multi-TC DCB requires a different scheduler layout. */
```

This is a minor clarity improvement, not an error.

---

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

### Correctness

**Error: nb_tm_qps uninitialized in existing VSI setup paths**

The patch introduces `vsi->nb_tm_qps` and initializes it in `ice_setup_vsi()` for the `ICE_VSI_PF` case:

```c
vsi->nb_tm_qps = vsi->nb_qps;
```

However, `ice_setup_vsi()` handles multiple VSI types (`ICE_VSI_PF`, `ICE_VSI_CTRL`, `ICE_VSI_VF`). The `ICE_VSI_CTRL` and `ICE_VSI_VF` cases do not initialize `nb_tm_qps`, leaving it uninitialized (zero) for those VSI types.

Later, `ice_dev_info_get()` reads `vsi->nb_tm_qps`:

```c
dev_info->max_tx_queues = vsi->nb_tm_qps;
```

If `ice_dev_info_get()` is called on a control or VF VSI, this would report `max_tx_queues = 0`, which is incorrect.

**Suggested fix:**

Initialize `nb_tm_qps` for all VSI types in `ice_setup_vsi()`:

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

Or, if only PF VSIs use TM, guard the access in `ice_dev_info_get()`:

```c
dev_info->max_tx_queues = (vsi->type == ICE_VSI_PF) ? vsi->nb_tm_qps : vsi->nb_qps;
```

**Warning: nb_used_qps may not reflect actual queue count in all cases**

The patch changes `ice_vsi_disable_queues_intr()` to iterate over `nb_used_qps` instead of `nb_qps`:

```c
for (i = 0; i < vsi->nb_used_qps; i++) {
```

`nb_used_qps` tracks the number of queues actually configured by the application. If queues are stopped individually (not all at once), `nb_used_qps` may not match the number of queues that have interrupt state to clear. However, examining the code context, `ice_vsi_disable_queues_intr()` is called during port stop, when all queues are being torn down, so `nb_used_qps` should be correct at that point.

This is acceptable but worth noting in case queue lifecycle assumptions change.

### Style

**Info: Comment phrasing**

In `ice_tm.c`, the existing comment style uses concise phrases. The new code is consistent with this style. No issues.

---

## Summary

### Errors (must fix)

1. **Patch 1/3**: Potential underflow of `qg_lvl` if `q_lvl` equals 0, used as array index without bounds check.
2. **Patch 3/3**: `vsi->nb_tm_qps` uninitialized for `ICE_VSI_CTRL` and `ICE_VSI_VF` types, read in `ice_dev_info_get()`.

### Warnings (should fix)

1. **Patch 2/3**: `pf->dcb_num_tcs` initialization pattern may leave inconsistent state on configure failure (existing code may already handle this correctly; verify error paths).
2. **Patch 3/3**: `nb_used_qps` assumption in `ice_vsi_disable_queues_intr()` depends on queue lifecycle; verify correctness if queue stop sequence changes.

### Info (consider)

1. **Patch 2/3**: Comment clarity on TM/DCB conflict could be improved.