|WARNING| [v2,1/1] net/nbl: fix incorrect MTU stored in rxmode

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch fixes an MTU handling bug in the nbl driver. It removes incorrect modification of `dev_conf.rxmode.mtu` and corrects confusion between MTU and frame length.

---

## Errors

### 1. Missing scatter Rx re-evaluation after MTU change

**Issue:** After changing the MTU, the driver must re-evaluate whether scattered Rx is needed and update the Rx burst function selection accordingly. The new MTU may require scatter mode if the resulting frame size exceeds the mbuf data buffer size, or may allow disabling scatter if the new MTU fits within a single mbuf.

**Why it matters:** When MTU increases such that frames no longer fit in a single mbuf, continuing to use a non-scattered Rx function will cause packet truncation or drops. When MTU decreases to fit in a single mbuf, continuing to use scattered Rx unnecessarily impacts performance.

**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;
	struct nbl_adapter *adapter = ETH_DEV_TO_NBL_DEV_PF_PRIV(eth_dev);
	struct nbl_dev_mgt *dev_mgt = NBL_ADAPTER_TO_DEV_MGT(adapter);
	struct nbl_dispatch_ops *disp_ops = NBL_DEV_MGT_TO_DISP_OPS(dev_mgt);
	uint32_t frame_size;
	uint32_t mbuf_data_size;
	int ret;

	/* mtu setting is forbidden if port is started */
	if (dev_data->dev_started) {
		NBL_LOG(ERR, "port %d must be stopped before configuration", dev_data->port_id);
		return -EBUSY;
	}

	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;

	/* Re-select Rx function based on new frame size vs mbuf size */
	frame_size = mtu + NBL_ETH_OVERHEAD;
	mbuf_data_size = rte_pktmbuf_data_room_size(dev_data->rx_queues[0]->mp) -
			 RTE_PKTMBUF_HEADROOM;

	if (frame_size > mbuf_data_size) {
		/* Need scatter Rx for this MTU */
		dev_data->dev_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_SCATTER;
		dev_data->scattered_rx = 1;
	} else {
		/* Can use single-segment Rx */
		dev_data->dev_conf.rxmode.offloads &= ~RTE_ETH_RX_OFFLOAD_SCATTER;
		dev_data->scattered_rx = 0;
	}

	/* Update Rx burst function selection */
	nbl_select_rx_function(eth_dev);

	return 0;
}
```

Note: Verify that `nbl_select_rx_function()` exists or add the equivalent logic to select between scattered and non-scattered Rx burst functions. Also verify that accessing `rx_queues[0]->mp` at this point is safe (queue must be configured).

---

## Warnings

### 1. Missing validation against scatter Rx capability

**Issue:** The driver should verify whether scatter Rx is supported before accepting an MTU that requires it. If the hardware cannot do multi-segment receive and the new MTU produces frames larger than the mbuf data buffer, the MTU change should be rejected.

**Suggested fix:** Before setting the MTU in hardware, check:
```c
if (frame_size > mbuf_data_size &&
    !(dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER)) {
	NBL_LOG(ERR, "MTU %u requires scatter Rx which is not supported", mtu);
	return -EINVAL;
}
```

This assumes `dev_info` is available or can be obtained. If scatter is always supported, this check is unnecessary.

---

## Info

### 1. Comment typo fix is good

The change from "port is start" to "port is started" improves readability.

### 2. Patch correctly addresses the stated issues

- Removes stale `rxmode.mtu` write (correct per AGENTS.md MTU guidelines)
- Removes `frame_size` assignment that confused MTU with frame length (correct)
- The fix aligns with DPDK's MTU handling model where `dev->data->mtu` is canonical after configure

---

## Final Check

**Correctness bugs:** Missing scatter Rx re-evaluation after MTU change is a **potential correctness issue** -- oversized packets could be silently dropped or truncated if the Rx function is not updated.

**Style/process items flagged:** Only items where an actual problem exists (missing scatter re-evaluation, missing capability check).

**No false positives:** All flagged items represent real gaps in MTU change handling.