|WARNING| [v2] net/ice: add per-queue Tx rate limit support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch Summary
This patch adds per-queue Tx rate limiting support to the ice PMD using `rte_eth_set_queue_rate_limit()` and `rte_eth_get_queue_rate_limit()` operations. The feature uses hardware packet pacing and is mutually exclusive with the Traffic Management API.

---

## Errors

### 1. Missing error check on `ice_get_lan_q_ctx()`
**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Function:** `ice_txq_rate_limit_kbps()`

The function `ice_get_lan_q_ctx()` can return NULL (as checked in the code), but the subsequent access to `q_ctx->q_teid` occurs without checking if `node` is NULL first. If `q_ctx` is NULL, the dereference `q_ctx->q_teid` will crash.

```c
q_ctx = ice_get_lan_q_ctx(hw, pf->main_vsi->idx, 0, queue_idx);
if (q_ctx == NULL)
	return 0;

node = ice_sched_find_node_by_teid(hw->port_info->root, q_ctx->q_teid);
/* q_ctx was already checked, but node needs checking before q_ctx->bw_t_info access */
```

The issue is that if `node == NULL`, the code continues to check the profile index using `node->info.data.eir_bw.bw_profile_idx`, but then if both conditions fail (node is NULL or profile matches default), it returns `q_ctx->bw_t_info.eir_bw.bw` which could be stale data since the node doesn't exist.

**Suggested fix:**
```c
uint32_t
ice_txq_rate_limit_kbps(struct ice_pf *pf, uint16_t queue_idx)
{
	struct ice_hw *hw = ICE_PF_TO_HW(pf);
	struct ice_sched_node *node;
	struct ice_q_ctx *q_ctx;

	q_ctx = ice_get_lan_q_ctx(hw, pf->main_vsi->idx, 0, queue_idx);
	if (q_ctx == NULL)
		return 0;

	node = ice_sched_find_node_by_teid(hw->port_info->root, q_ctx->q_teid);
	if (node == NULL)
		return 0;  /* Node doesn't exist, so no rate limit active */

	if (rte_le_to_cpu_16(node->info.data.eir_bw.bw_profile_idx) ==
	    ICE_SCHED_DFLT_RL_PROF_ID)
		return 0;

	return q_ctx->bw_t_info.eir_bw.bw;
}
```

---

## Warnings

### 1. Missing bounds check on `queue_idx` in `ice_get_queue_rate_limit()`
**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Function:** `ice_get_queue_rate_limit()`

The `ice_set_queue_rate_limit()` function checks that `queue_idx < dev->data->nb_tx_queues`, but `ice_get_queue_rate_limit()` does not perform this validation before calling `ice_txq_rate_limit_kbps()`.

**Suggested fix:**
```c
static int
ice_get_queue_rate_limit(struct rte_eth_dev *dev, uint16_t queue_idx,
			 uint32_t *tx_rate)
{
	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);

	if (queue_idx >= dev->data->nb_tx_queues) {
		PMD_DRV_LOG(ERR, "Tx queue %u is out of range (%u configured)",
			    queue_idx, dev->data->nb_tx_queues);
		return -EINVAL;
	}

	*tx_rate = ice_txq_rate_limit_kbps(pf, queue_idx) / 1000;

	return 0;
}
```

### 2. Integer division truncates sub-Mbps rates
**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Function:** `ice_get_queue_rate_limit()`

The conversion `ice_txq_rate_limit_kbps(pf, queue_idx) / 1000` truncates rates below 1 Mbps to zero. If a rate of 500 Kbps was set (perhaps through TM API or another path), the get function would report 0 (unlimited) instead of the actual rate.

This may be acceptable given the API specifies Mbps, but it's worth noting that the get function could return 0 for both "no limit" and "limit less than 1 Mbps". Consider documenting this behavior or rounding up instead of truncating.

### 3. Error message formatting inconsistency
**File:** `drivers/net/intel/ice/ice_tm.c`

Error messages in DPDK typically end without punctuation. The new error message ends with no period, which is correct, but the message could be clearer about which API to call to clear the rate limits.

Current:
```c
error->message =
	"queue rate limit already set via rte_eth_set_queue_rate_limit";
```

Suggested (more actionable):
```c
error->message =
	"queue rate limit set via rte_eth_set_queue_rate_limit must be cleared first";
```

### 4. Inconsistent `tx_rate` type checking
**File:** `drivers/net/intel/ice/ice_ethdev.c`

The `tx_rate` parameter is `uint32_t`, but the maximum check compares against `ICE_SCHED_MAX_BW / 1000` without ensuring the division result fits in `uint32_t`. If `ICE_SCHED_MAX_BW` is a 64-bit value, the comparison may not work as intended.

Verify that `ICE_SCHED_MAX_BW / 1000` does not overflow `uint32_t` range, or cast explicitly:
```c
if (tx_rate > (uint32_t)(ICE_SCHED_MAX_BW / 1000)) {
```

---

## Info

### 1. Documentation clarity for queue state requirement
**File:** `doc/guides/nics/ice.rst`

The documentation clearly states that "the queue must be started before its rate can be set", which matches the code check for `RTE_ETH_QUEUE_STATE_STARTED`. This is good.

However, it would be helpful to explicitly document that **calling `ice_get_queue_rate_limit()` on a stopped queue returns 0** (since `ice_txq_rate_limit_kbps()` returns 0 when the node doesn't exist). Currently, users might expect an error instead.

### 2. Consider adding testpmd command
**File:** N/A

While not required for the patch itself, adding a testpmd command to exercise `rte_eth_set_queue_rate_limit()` and `rte_eth_get_queue_rate_limit()` would demonstrate usage and provide a test path. This is not mandatory but would strengthen the feature validation.

---

## Summary

The patch correctly implements the per-queue rate limiting feature and properly documents the mutual exclusion with Traffic Management API. The main correctness issue is the potential NULL pointer dereference in `ice_txq_rate_limit_kbps()` when `node` is NULL but code continues to use `q_ctx` data. The missing bounds check in the get function should also be addressed for safety.