|WARNING| [v1,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/170127

_AI Code Review_

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

# DPDK Patch Review: net/nbl: fix incorrect MTU stored in rxmode

## Summary

This patch removes code that incorrectly stores MTU values in `dev_conf.rxmode.mtu` and an unused `frame_size` calculation. The patch author's explanation is that the ethdev layer already updates `dev->data->mtu` on success, making the driver assignment unnecessary.

---

## Errors

### 1. MTU confused with frame length

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

The removed code was storing a **frame length** (`mtu + NBL_ETH_OVERHEAD`) into a field intended for **MTU**.
This is backwards and would cause off-by-overhead-bytes errors throughout the driver.
MTU is the L3 payload size (e.g., 1500); frame length includes L2 overhead (14-18+ bytes).
Writing frame_size into rxmode.mtu would make subsequent reads of that field return a value 18 bytes too large.

The patch correctly removes this line, as `rxmode.mtu` should never be written by the driver --
it is the user's configuration *request* consumed during `rte_eth_dev_configure()`.
After configure completes, the canonical MTU is in `dev->data->mtu`, not `dev_conf.rxmode.mtu`.

However, the removed assignment's use of `frame_size` instead of `mtu` indicates
the original code confused the two concepts.

**Verdict:** The patch fixes the bug by removing the incorrect assignment. No further action needed on this item.

### 2. Missing scatter Rx validation

**File:** drivers/net/nbl/nbl_dev/nbl_dev.c  
**Function:** `nbl_mtu_set()`

The function accepts an MTU value but does not verify that the resulting frame length
(MTU + `NBL_ETH_OVERHEAD`) will fit within the Rx buffer size when scatter Rx is disabled.
If the configured MTU produces frames larger than a single mbuf can hold
and scatter Rx is not enabled, packets will be silently truncated or dropped.

The function should check:
```c
uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
uint32_t mbuf_size = /* get from rxq mempool or dev_data->min_rx_buf_size */;

if (!dev_data->scattered_rx && frame_size > mbuf_size) {
    if (!(dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER))
        return -EINVAL;
    /* Enable scatter Rx and re-select Rx burst function */
    dev_data->dev_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_SCATTER;
    dev_data->scattered_rx = 1;
}
```

Alternatively, if the driver does not support scatter Rx or cannot enable it at this point,
return `-EINVAL` when the MTU is too large.

**Suggested fix:**
Add validation logic as shown above, or ensure that the driver's `dev_info.max_mtu`
already enforces a limit consistent with the mbuf size and that scatter Rx is automatically
selected during queue setup based on `dev->data->mtu`.
Review the driver's Rx queue setup code to confirm this is handled there.

---

## Warnings

None.

---

## Info

### Comment removal

The patch removes the comment `/* mtu setting is forbidden if port is start */`.
This comment was accurate (the check `if (dev_data->dev_started)` immediately follows)
and removing it reduces code clarity. Consider restoring it or replacing it with:

```c
/* MTU changes require port to be stopped */
if (dev_data->dev_started) {
    ...
}
```

---

## Positive Observations

- **Correct fix for stale rxmode.mtu:** The patch correctly removes the driver's write to `dev_conf.rxmode.mtu`, which is stale after `rte_eth_dev_configure()` completes. Drivers should read `dev->data->mtu`, not `rxmode.mtu`.

- **Removes unused variable:** The `frame_size` variable becomes unused after removing the assignment and is correctly removed.

- **Appropriate error handling:** The function returns the error code from `disp_ops->set_mtu()` without modification, which is correct.

---

## Final Recommendation

**Conditional Accept** pending verification:

1. **Verify scatter Rx handling** (Error #2): Confirm that either:
   - The driver's `dev_info.max_mtu` is set to enforce a safe limit
     (e.g., `max_rx_pktlen - NBL_ETH_OVERHEAD` where `max_rx_pktlen` matches
     the single-mbuf buffer size when scatter is disabled), OR
   - The driver's Rx queue setup code automatically selects scatter Rx
     when `dev->data->mtu + NBL_ETH_OVERHEAD > mbuf_size`, OR
   - Add the validation shown above to `nbl_mtu_set()`.

2. **Consider restoring the comment** (Info): The removed comment accurately described the check and should be kept or reworded.

The patch's core change (removing the incorrect `rxmode.mtu` assignment) is correct and fixes a real bug.
The scatter Rx validation issue is a pre-existing gap that this patch does not introduce,
but should be addressed to prevent silent packet truncation.