|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

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

### Summary
This patch removes an incorrect assignment to `dev_data->dev_conf.rxmode.mtu` in the NBL PMD's `mtu_set` callback. The commit message states that the ethdev layer already updates `data->mtu` on success, making the assignment unnecessary.

---

## Findings

### Errors

**1. Incorrect value assigned to rxmode.mtu before removal**

The removed line was:
```c
dev_data->dev_conf.rxmode.mtu = frame_size;
```

Where `frame_size = mtu + NBL_ETH_OVERHEAD`.

This is doubly wrong:
1. After `rte_eth_dev_configure()` completes, the canonical MTU is stored in `dev->data->mtu`. The `dev->data->dev_conf.rxmode.mtu` field is the user's *request* and must not be written after configure -- it becomes stale if `rte_eth_dev_set_mtu()` is called later, which is exactly this code path.
2. Even if writing to `rxmode.mtu` were correct, the value should be `mtu`, not `frame_size`. MTU is the L3 payload size; frame size is MTU plus L2 overhead. Storing frame size in the MTU field confuses the two concepts.

The patch correctly removes this line. However, the commit message should explain *both* issues:
- Writing to `rxmode.mtu` after configure is wrong (stale value problem)
- The value being written (`frame_size`) is wrong (MTU vs frame size confusion)

**Suggested commit message addition:**
```
The ethdev layer already updates dev->data->mtu on success.
After rte_eth_dev_configure(), dev_conf.rxmode.mtu becomes stale
and should not be modified by mtu_set.

Additionally, the original code assigned frame_size (MTU + overhead)
instead of MTU, confusing frame length with MTU.

Remove the incorrect assignment; the ethdev layer handles mtu updates.
```

---

### Warnings

**1. Missing verification of scatter Rx handling**

When MTU is changed, the PMD should verify whether the new frame size (MTU + overhead) fits in a single mbuf or requires scatter Rx. The patch does not show any logic for:
- Re-validating frame size vs mbuf size after the MTU change
- Re-selecting the Rx burst function if scatter is now needed (or no longer needed)
- Returning an error if the new MTU requires scatter but scatter is not enabled

This is not introduced by the patch (the original code also lacks this), but since this is a fix being backported to stable, it's worth noting. The PMD should check:

```c
mbuf_size = rte_pktmbuf_data_room_size(rxq->mp) - RTE_PKTMBUF_HEADROOM;
max_frame = mtu + NBL_ETH_OVERHEAD;
if (!dev_data->scattered_rx && max_frame > mbuf_size) {
    if (!(dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER))
        return -EINVAL;
    /* Or auto-enable scatter and re-select Rx function */
}
```

**Recommendation:** File a follow-up bug to verify scatter Rx handling in `nbl_mtu_set()`.

---

**2. Unused variable after patch**

The patch removes the only use of `frame_size`:
```c
-	uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
```

After the assignment to `rxmode.mtu` is removed, `frame_size` is declared but never used. The patch should also remove the `frame_size` declaration to avoid a compiler warning (unused variable).

**Suggested fix:**
```diff
-	uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
 	int ret;
```

---

### Info

**1. Comment removed**

The patch removes the comment:
```c
-	/* mtu setting is forbidden if port is start */
```

While the comment was grammatically incorrect ("port is start" should be "port is started"), it did document the logic. Consider replacing it with a corrected version:

```c
/* MTU setting is forbidden if port is started */
```

However, since the check immediately follows with a clear error message, the comment may be considered redundant. This is a minor style preference.

---

## Positive Aspects

1. **Correct identification of the bug:** The patch correctly identifies that writing to `rxmode.mtu` after configure is wrong.

2. **Proper stable backport:** The patch includes `Cc: stable at dpdk.org` and a `Fixes:` tag, following DPDK backport policy.

3. **Targeted fix:** The change is minimal and focused on the specific issue.

---

## Summary

The patch fixes a real bug (stale write to `rxmode.mtu`), but:
- The commit message should explain that the value being written was also wrong (frame size vs MTU)
- The unused `frame_size` variable should be removed
- A follow-up bug should be filed for missing scatter Rx validation in `nbl_mtu_set()`

The core fix is correct and should be merged after addressing the unused variable.