|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 Summary
- **File**: `drivers/net/nbl/nbl_dev/nbl_dev.c`
- **Change**: Removes redundant MTU assignment in `nbl_mtu_set()`
- **Reasoning**: ethdev layer already updates `data->mtu` on success

---

## Review Findings

### Errors

**1. Incorrect assignment being removed (MTU vs Frame Size confusion)**

The removed line assigns `frame_size` (MTU + overhead) to `dev_conf.rxmode.mtu`:
```c
dev_data->dev_conf.rxmode.mtu = frame_size;
```

This is wrong in two ways:
- **Assigns frame size to MTU field**: `frame_size = mtu + NBL_ETH_OVERHEAD` adds L2 overhead to the MTU, producing a frame length value. The `rxmode.mtu` field should contain the L3 MTU, not the frame size. This is a correctness bug (MTU confused with frame length).
- **Writes to stale field**: After `rte_eth_dev_configure()` completes, the canonical MTU is `dev->data->mtu`. The `dev_conf.rxmode.mtu` field is the user's configuration request and becomes stale if `rte_eth_dev_set_mtu()` is called. PMDs should not write to it.

However, the patch's reasoning is incomplete. The commit message states "ethdev layer already updates data->mtu on success" but does not mention that the removed assignment was incorrect (frame size instead of MTU).

**Recommendation**: The removal is correct but the commit message should clarify that the line was wrong (assigned frame size to MTU field) in addition to being redundant.

---

**2. Unused variable after removal**

The patch removes the only use of the `frame_size` variable but does not remove its declaration:
```c
uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
```

This will produce a compiler warning (unused variable) and should be removed along with the assignment.

**Fix**: Remove the `frame_size` variable declaration:
```c
-	uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
```

---

### Warnings

None.

---

### Info

**Comment removal context**

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

This comment is self-evident from the `if (dev_data->dev_started)` check immediately below. Its removal is acceptable (it restates what the code shows), but this is an optional style improvement rather than a requirement.

---

## Summary

The patch addresses a real issue (incorrect MTU vs frame size assignment) but:
1. **Leaves behind an unused variable** (`frame_size`) that will cause a compiler warning (Error)
2. **Commit message should clarify** that the removed line was incorrect (assigned frame size to MTU field), not just redundant

**Required changes:**
- Remove the unused `frame_size` variable declaration
- Optionally update commit message to mention the frame-size-vs-MTU error being fixed