|WARNING| [v1,1/1] net/nbl: allow MTU change when port is started

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: net/nbl: allow MTU change when port is started

---

## Summary

This patch removes a dev_started check from the MTU set callback to allow runtime MTU changes. The patch contains several correctness bugs and API compliance issues that must be fixed.

---

## Errors

### 1. Stale MTU value written to wrong field

**File:** `drivers/net/nbl/nbl_dev/nbl_dev.c`  
**Line:** `dev_data->dev_conf.rxmode.mtu = frame_size;`

The code writes `frame_size` (MTU + overhead) to `dev_conf.rxmode.mtu`, which should contain the MTU value, not the frame length. This is off by the overhead amount (appears to be `NBL_ETH_OVERHEAD` bytes).

Additionally, `dev_conf.rxmode.mtu` is the user's configure-time request and becomes stale after `rte_eth_dev_configure()` completes. PMDs must write to `dev_data->mtu` instead, which is the canonical post-configure MTU storage.

**Fix:**
```c
dev_data->mtu = mtu;
```

**Why it matters:** Writing frame_size instead of mtu produces an incorrect value. Writing to the wrong field means `rte_eth_dev_get_mtu()` and other DPDK core functions will read a stale/wrong value from `dev_data->mtu`.

---

### 2. Missing validation: MTU vs mbuf size

The `mtu_set` callback does not verify whether the new MTU is compatible with the currently configured mbuf size and scatter Rx setting. When the resulting frame length exceeds the mbuf data room size and scatter Rx is not enabled, packets will be silently truncated or dropped.

**Required check:**
```c
struct rte_eth_dev_info dev_info;
uint32_t max_pkt_len = mtu + NBL_ETH_OVERHEAD;
uint16_t mbuf_data_size;

/* Get current Rx queue mbuf size */
if (eth_dev->data->nb_rx_queues > 0) {
	struct nbl_rxq *rxq = eth_dev->data->rx_queues[0];
	if (rxq != NULL && rxq->mp != NULL) {
		mbuf_data_size = rte_pktmbuf_data_room_size(rxq->mp) - 
				 RTE_PKTMBUF_HEADROOM;
		
		/* Reject if scatter is not enabled and frame doesn't fit */
		if (!eth_dev->data->scattered_rx && 
		    max_pkt_len > mbuf_data_size) {
			NBL_LOG(ERR, "MTU %u requires scatter Rx (frame %u > mbuf %u)",
				mtu, max_pkt_len, mbuf_data_size);
			return -EINVAL;
		}
	}
}
```

**Why it matters:** Accepting an MTU that produces oversized frames without scatter Rx enabled causes silent packet loss/corruption at runtime.

---

### 3. Missing Rx function re-selection

After changing MTU, if the new frame size crosses the threshold between single-segment and multi-segment Rx (frame size vs mbuf size), the PMD must re-select the appropriate Rx burst function. The patch does not call the Rx function selection logic.

**Required action:**
After updating `dev_data->mtu`, call the function that selects between scalar and scattered Rx burst functions (typically named something like `nbl_set_rx_function()` or `nbl_select_rx_function()`).

**Why it matters:** If the port is started (which is now allowed by this patch), the Rx function must be updated to handle the new packet size. Using a scalar Rx function after increasing MTU beyond mbuf size will drop or corrupt packets.

---

## Warnings

### 1. Missing release notes

The patch claims to fix a bug (MTU could not be changed while port is started) but does not update release notes. Since this changes user-visible behavior (runtime MTU changes are now allowed), it should be documented.

**Suggested addition to `doc/guides/rel_notes/release_26_03.rst`** (or current release):
```rst
* **net/nbl: Enabled runtime MTU changes**

  The nbl PMD now allows MTU changes while the port is started, matching
  typical kernel driver behavior. Previously, MTU changes required stopping
  the port.
```

---

### 2. Incomplete fix rationale

The commit message states "Remove the dev_started check... to allow MTU configuration while the port is started" but does not explain:
- Why this check existed originally
- Whether the hardware supports runtime MTU changes
- What testing was done to verify runtime changes work correctly

**Suggested addition to commit message:**
Add a paragraph explaining: "The nbl hardware supports MTU changes without requiring port stop. The original check was overly restrictive. Verified with [describe testing]."

---

## Info

### 1. Patch removes safety check without adding replacement validation

The original code prevented MTU changes while started, which is a safe (if restrictive) policy. The patch removes this check but does not add any new validation of the MTU value. Consider whether the hardware has any MTU-related constraints that should be checked (e.g., maximum frame size, DMA buffer alignment).

---

## Compliance Summary

| Category | Status |
|----------|--------|
| Correctness bugs | **3 Errors** (wrong field written, wrong value, missing validation, missing function re-selection) |
| Coding style | OK |
| Documentation | **1 Warning** (missing release notes) |
| Testing | Unknown (not visible in patch) |

---

## Recommendation

**REJECT** - This patch cannot be merged as-is due to correctness bugs. The author must:

1. Fix the assignment to write `mtu` (not `frame_size`) to `dev_data->mtu` (not `dev_conf.rxmode.mtu`)
2. Add validation that the new MTU is compatible with mbuf size / scatter Rx setting
3. Add Rx function re-selection after MTU change when port is started
4. Update release notes
5. Describe hardware capabilities and testing in commit message