|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

---

## Errors

### 1. Stale MTU value assignment after configure

**Location:** `drivers/net/nbl/nbl_dev/nbl_dev.c:763`

```c
dev_data->dev_conf.rxmode.mtu = frame_size;
```

After `rte_eth_dev_configure()` completes, the canonical MTU is stored in `dev->data->mtu`, not in `dev_conf.rxmode.mtu`. The `rxmode.mtu` field is the user's initial request and becomes stale if `rte_eth_dev_set_mtu()` is called later. PMDs should update `dev->data->mtu` directly in the `mtu_set` callback.

Additionally, this line assigns `frame_size` (MTU + overhead) to `rxmode.mtu`, but MTU should be the L3 payload size only, not the frame size. This is a units error.

**Suggested fix:**
```c
/* Update the canonical MTU value */
dev_data->mtu = mtu;
```

---

### 2. Missing Rx function re-selection after MTU change

**Location:** `drivers/net/nbl/nbl_dev/nbl_dev.c:760-768`

When the MTU changes at runtime (port started), the maximum frame length may cross the threshold where scattered Rx becomes necessary. If the new frame size exceeds the Rx buffer size, the PMD must either:
1. Enable scattered Rx and re-select the Rx burst function, or
2. Return an error if scatter is not supported or buffers are insufficient

This patch removes the dev_started check, allowing MTU changes while the port is running, but does not re-select the Rx function. If the application increases the MTU beyond the single-buffer threshold, the non-scattered Rx function will silently drop or corrupt multi-segment packets.

**Suggested fix:**
```c
int nbl_mtu_set(struct rte_eth_dev *eth_dev, uint16_t mtu)
{
	struct rte_eth_dev_data *dev_data = eth_dev->data;
	/* ... existing declarations ... */
	uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
	uint32_t buf_size;
	int ret;

	/* Check if scatter is needed for new MTU */
	if (dev_data->nb_rx_queues > 0) {
		struct nbl_rxq *rxq = dev_data->rx_queues[0];
		buf_size = rte_pktmbuf_data_room_size(rxq->mb_pool) - RTE_PKTMBUF_HEADROOM;
		
		if (frame_size > buf_size) {
			if (!(dev_data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_SCATTER)) {
				NBL_LOG(ERR, "MTU %u requires scatter Rx but not enabled", mtu);
				return -EINVAL;
			}
			dev_data->scattered_rx = 1;
		}
	}

	ret = disp_ops->set_mtu(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt), dev_mgt->net_dev->vsi_id, mtu);
	if (ret != 0)
		return ret;

	dev_data->mtu = mtu;

	/* Re-select Rx functions if port is started */
	if (dev_data->dev_started) {
		for (uint16_t i = 0; i < dev_data->nb_rx_queues; i++) {
			ret = nbl_rx_queue_select_function(eth_dev, i);
			if (ret != 0) {
				NBL_LOG(ERR, "Failed to re-select Rx function for queue %u", i);
				return ret;
			}
		}
	}

	return 0;
}
```

Note: The exact function name `nbl_rx_queue_select_function` is hypothetical; adjust to match the driver's actual Rx function selection mechanism.

---

## Warnings

### 1. Missing release notes update

**Context:** This patch changes MTU behavior (removes restriction on started ports), which is a functional change visible to applications.

Release notes should document that MTU can now be changed dynamically on started ports. Add an entry to `doc/guides/rel_notes/release_XX_YY.rst` (where XX_YY is the target release):

```rst
* **Updated nbl driver MTU configuration.**

  * MTU can now be changed while the port is started, enabling dynamic
    reconfiguration without port restart.
```

---

### 2. Verify hardware MTU change atomicity

**Context:** The patch allows MTU changes on started ports.

When changing MTU on a running port, verify that the hardware register write (`disp_ops->set_mtu`) is atomic and does not cause packet corruption or drops during the transition. If the hardware requires stopping Rx queues before MTU change, the current approach may not be safe. Review the `set_mtu` implementation in the dispatch layer to confirm it handles live reconfiguration correctly.

This is flagged as a **Warning** because the review cannot determine from this patch alone whether the underlying hardware operation is safe for live updates. If unsafe, this becomes an **Error** (race condition).

---

## Summary

**Critical issues:**
1. Incorrect MTU storage (assigns frame_size to rxmode.mtu instead of storing MTU in dev->data->mtu)
2. Missing Rx function re-selection after MTU change can cause silent packet drops/corruption

**Additional concerns:**
1. Missing release notes for functional change
2. Hardware MTU change atomicity on started ports not verified

The patch's intent (allow MTU change on started ports) is reasonable, but the implementation is incomplete and will cause correctness bugs if the new MTU requires scatter Rx.