|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. Use of stale `dev_conf.rxmode.mtu` after configure

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

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

**Problem:** After `rte_eth_dev_configure()` completes, the canonical MTU is stored in `dev->data->mtu`, not `dev->data->dev_conf.rxmode.mtu`. The `rxmode.mtu` field is the user's configuration request and becomes stale if `rte_eth_dev_set_mtu()` is called later. PMDs must read and write `dev->data->mtu` instead.

Additionally, the code is writing `frame_size` (MTU + overhead) to what should be an MTU field. This confuses MTU with frame length.

**Fix:**
```c
/* Update the canonical MTU field */
dev_data->mtu = mtu;

/* Call hardware configuration with the MTU value */
ret = disp_ops->set_mtu(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt), 
                        dev_mgt->net_dev->vsi_id, mtu);
```

---

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

**Problem:** When MTU is changed while the port is running, the maximum frame size may now exceed what the currently selected Rx burst function can handle. If the new MTU requires multi-segment receive but the port was using the single-segment Rx function, packets will be silently truncated or dropped.

The `mtu_set` callback must:
1. Check if the new frame size exceeds the mbuf data buffer size
2. Enable scatter Rx if needed (or return error if scatter is not available)
3. Re-select the appropriate Rx burst function

**Current code path:** The patch removes the `dev_started` check, allowing runtime MTU changes, but does not update `dev->data->scattered_rx` or call any function to reconfigure the Rx data path.

**Fix:** After the hardware MTU is configured successfully, add:
```c
uint32_t mbuf_size;
uint32_t max_frame_len = mtu + NBL_ETH_OVERHEAD;

/* Determine if scatter Rx is needed for the new MTU */
if (dev_data->nb_rx_queues > 0) {
    struct nbl_rx_queue *rxq = dev_data->rx_queues[0];
    mbuf_size = rte_pktmbuf_data_room_size(rxq->mp) - 
                RTE_PKTMBUF_HEADROOM;
    
    if (max_frame_len > mbuf_size) {
        if (!(dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER))
            return -EINVAL;
        
        dev_data->dev_conf.rxmode.offloads |= 
            RTE_ETH_RX_OFFLOAD_SCATTER;
        dev_data->scattered_rx = 1;
    }
}

/* Re-select Rx burst function for new MTU/scatter settings */
nbl_set_rx_function(eth_dev);
```

Note: Without seeing the full driver code, I cannot verify if `nbl_set_rx_function()` exists or what the correct function name is. The PMD must have a mechanism to select between scalar and scattered Rx functions based on `dev->data->scattered_rx` and frame size.

---

### 3. Missing error path resource cleanup (pre-existing, but now more critical)

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

**Problem:** If `disp_ops->set_mtu()` fails, the function returns the error but has already modified `dev_data->dev_conf.rxmode.mtu` (or should have modified `dev_data->mtu`). This leaves the software state inconsistent with the hardware state.

With the removal of the `dev_started` check, this inconsistency is now more serious because the port is running and the mismatch can cause immediate packet handling errors.

**Fix:** Only update the software MTU after the hardware operation succeeds:
```c
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;

/* Hardware succeeded, now update software state */
dev_data->mtu = mtu;

/* Re-evaluate scatter Rx and burst function as shown in error #2 */
```

---

## WARNINGS

### 1. Hardcoded overhead constant

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

```c
uint32_t frame_size = mtu + NBL_ETH_OVERHEAD;
```

**Issue:** The overhead is hardcoded to `NBL_ETH_OVERHEAD`. If this driver supports VLAN or QinQ offloads, the overhead calculation should account for the enabled encapsulations. Using a device-specific overhead constant is acceptable if the value matches the advertised capabilities in `dev_info`, but verify that `NBL_ETH_OVERHEAD` correctly represents `dev_info.max_rx_pktlen - dev_info.max_mtu`.

**Suggestion:** If VLAN/QinQ support exists, calculate overhead based on enabled offloads:
```c
uint32_t overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
if (dev_data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP)
    overhead += RTE_VLAN_HLEN;
if (dev_data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP)
    overhead += RTE_VLAN_HLEN;
uint32_t frame_size = mtu + overhead;
```

If the driver does not support VLAN offloads and `NBL_ETH_OVERHEAD` is `18` (standard Ethernet header + CRC), this is acceptable.

---

### 2. Missing release notes update

**Issue:** This patch changes user-visible behavior (MTU can now be changed on a started port, where it previously returned `-EBUSY`). This is a feature enhancement that should be documented in the release notes under the "New Features" or "net/nbl" PMD section.

**Required:** Add an entry to `doc/guides/rel_notes/release_26_03.rst` (or the appropriate current release file):
```rst
* **Updated nbl driver.**

  * Enabled MTU configuration on started ports.
```

---

## SUMMARY

**Critical issues requiring fix before merge:**
1. Incorrect write to `dev_conf.rxmode.mtu` instead of `dev->data->mtu` (Error #1)
2. Confusion between MTU and frame length in the write operation (Error #1)
3. Missing re-evaluation of scatter Rx requirement after MTU change (Error #2)
4. Software state updated before hardware operation, causing inconsistency on error (Error #3)

**Additional issues:**
5. Hardcoded Ethernet overhead (Warning #1 - acceptable if driver doesn't support VLAN)
6. Missing release notes entry (Warning #2)

The patch concept is reasonable (removing the arbitrary restriction on MTU changes for started ports), but the implementation is incomplete. The primary concern is that changing MTU on a running port requires re-configuring the Rx data path to handle the new maximum frame size, which this patch does not do.