|WARNING| [10/10] net/enetc4: add cacheable BD ring support with SW cache maintenance

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-19

# DPDK Patch Review

Reviewing patch series for DPDK net/enetc driver (10 patches) against AGENTS.md guidelines.

---

## PATCH 01/10: net/enetc: fix TX BD structure

### Errors

**Use-after-free risk in descriptor reuse**  
The patch removes the `ENETC_TXBD_FLAGS_IE` flag macro but does not add any code to clear the flags field when recycling TX descriptors. After `enetc_clean_tx_ring()` reclaims a BD, the `flags` field retains the `FLAGS_F` bit from the previous packet. When the driver reuses that BD index for a new packet, the old stale `FLAGS_F` could cause the hardware to treat an incomplete descriptor as frame-final, resulting in partial packet transmission or DMA reading uninitialized `addr`/`buf_len`.

Patch 02/10 addresses this by zeroing `flags` before writing the new descriptor, but that fix should be part of this patch or this patch should not be applied alone. As written, patch 01 creates a window where the removed flag handling is not replaced.

### Warnings

None.

---

## PATCH 02/10: net/enetc: fix TX BDs flag overwrite issue

### Errors

None (this patch correctly fixes the issue introduced in 01/10 by zeroing flags before setting offload bits).

### Warnings

**Bitwise OR on byte-size flags with 16-bit constant**  
```c
txbd->flags |= rte_cpu_to_le_16(ENETC4_TXBD_FLAGS_F);
```
`txbd->flags` is now `uint8_t` (per patch 01/10) but the code uses `rte_cpu_to_le_16()`. On a little-endian system this works (the upper byte of the 16-bit constant is zero), but it is type-mismatched. Should be:
```c
txbd->flags |= ENETC4_TXBD_FLAGS_F;
```
Since `ENETC4_TXBD_FLAGS_F` is `BIT(7)` (0x80), no endian conversion is needed for an 8-bit value.

Similarly, line 175 zeros `flags` (correct) but then the `|=` on line 185 treats it as 16-bit. The type inconsistency should be fixed.

---

## PATCH 03/10: net/enetc: fix queue initialization

### Errors

None.

### Warnings

None.

---

## PATCH 04/10: net/enetc: support ESP packet type in packet parsing

### Errors

None.

### Warnings

**Missing release notes**  
This patch adds a new feature (ESP packet type support) but does not update the release notes. Adding `RTE_PTYPE_TUNNEL_ESP` to the supported ptypes and handling it in the RX parse path is a user-visible capability change and should be documented in `doc/guides/rel_notes/release_*.rst`.

---

## PATCH 05/10: net/enetc: update random MAC generation code

### Errors

None.

### Warnings

None.

---

## PATCH 06/10: net/enetc: support scatter-gather

### Errors

**Missing scatter Rx offload advertisement on PF**  
Patch adds `RTE_ETH_RX_OFFLOAD_SCATTER` to `dev_rx_offloads_sup` in `enetc4_vf.c` but the PF driver (`enetc_ethdev.c` or `enetc4_ethdev.c`) does not advertise this capability. If scatter-gather is supported on both PF and VF (as the RX/TX code changes in `enetc_rxtx.c` suggest), then `dev_infos_get` for the PF must also include `RTE_ETH_RX_OFFLOAD_SCATTER` in `rx_offload_capa`. As written, applications using the PF cannot discover or enable scatter Rx.

**TX multi-segment without enabling scatter offload**  
The TX path (`enetc_xmit_pkts_nc`) now handles `nb_segs > 1` and walks the segment chain, but there is no check that the application has enabled `RTE_ETH_TX_OFFLOAD_MULTI_SEGS` before attempting to transmit a multi-segment packet. The driver should either:
- Verify the offload is enabled and reject/drop multi-segment packets when it is not, or
- Advertise multi-segment support unconditionally if the hardware always supports it.

Currently the code will silently accept multi-segment packets even if the application did not request the offload, which violates the DPDK ethdev contract.

### Warnings

**Incomplete `dev_info` fields for PF**  
Patch updates `enetc4_vf_dev_infos_get()` to fill `nb_seg_max`, `nb_mtu_seg_max`, etc., but does not make the equivalent changes to the PF `dev_infos_get` function. If scatter-gather is supported on the PF, the PF info function must also report the segment limits.

**Missing release notes**  
Scatter-gather support is a new feature and requires a release notes entry.

---

## PATCH 07/10: net/enetc: add option to disable VSI messaging

### Errors

None.

### Warnings

**Missing release notes**  
New devarg `enetc4_vsi_disable` should be documented in release notes.

**Devarg value ignored**  
The code checks `rte_kvargs_count(kvlist, ENETC4_VSI_DISABLE) != 0` but does not parse the value. This means `enetc4_vsi_disable=0` and `enetc4_vsi_disable=1` both disable VSI messaging. If the intent is a boolean flag, the code is correct but the parameter string registration should clarify the usage (e.g., `"enetc4_vsi_disable=<0|1>"`). If any non-zero count disables messaging, the current code is fine.

---

## PATCH 08/10: net/enetc: add devargs to control VSI-PSI timeout and delay

### Errors

None.

### Warnings

**Missing release notes**  
New devargs should be documented.

**No input validation**  
```c
hw->vsi_timeout = (uint32_t)strtoul(val, NULL, 0);
```
If the user provides an invalid string or a value of zero, the driver will use zero as the timeout, causing `enetc4_msg_vsi_send()` to fail immediately (timeout loop never runs). Should validate that parsed values are non-zero or document that zero means "use default."

---

## PATCH 09/10: net/enetc: set user configurable priority to TX rings

### Errors

**Memory leak on device close**  
```c
hw->txq_prior = rte_zmalloc(NULL, hw->max_tx_queues * sizeof(uint32_t), 0);
```
This allocation in `parse_txq_prior()` (called from `enetc4_get_devargs()` -> `enetc4_dev_init()`) is never freed. The driver does not have a corresponding `rte_free(hw->txq_prior)` in `enetc4_dev_close()` or `enetc4_dev_uninit()`. Every device init/close cycle leaks `max_tx_queues * 4` bytes.

**Fix:**
```c
/* In enetc4_dev_close() or uninit: */
if (hw->txq_prior) {
    rte_free(hw->txq_prior);
    hw->txq_prior = NULL;
}
```

**Potential buffer overflow in `parse_txq_prior`**  
The loop `while (str != NULL && i < hw->max_tx_queues)` writes `hw->txq_prior[i++]` but if the user provides more `|`-separated values than `max_tx_queues`, the loop stops at `max_tx_queues` and the extra values are silently ignored. However, if `strtok` is called again after the loop (it isn't), there is no issue. The current code is safe but should document the truncation behavior.

### Warnings

**Missing release notes**  
New devarg should be documented.

**No input validation**  
`atoi(str)` can return zero for invalid input. If a user provides `enetc4_txq_prior=|2|3` (missing first value), `hw->txq_prior[0]` becomes zero, which may be a valid priority or may be an error. The driver should validate the input or document the format requirements.

---

## PATCH 10/10: net/enetc4: add cacheable BD ring support with SW cache maintenance

### Errors

**Race condition on TX BD flags field (correctness bug)**  
In `enetc_clean_tx_ring()`:
```c
txbd = ENETC_TXBD(*tx_ring, i);
txbd->flags = 0;
```
This write to `txbd->flags` happens during TX completion cleanup, clearing the descriptor for reuse. However, in `enetc_xmit_pkts_cacheable()`, the `flags` field is written multiple times:
1. Line 259: `txbd->flags = 0;` (initial clear)
2. Line 266: `if (seg->ol_flags & ENETC4_TX_CKSUM_OFFLOAD_MASK)` may modify flags
3. Line 284: `txbd->flags |= rte_cpu_to_le_16(ENETC4_TXBD_FLAGS_F);` (set frame-last)

Between the zeroing in `enetc_clean_tx_ring()` and the cache flush in `enetc_xmit_pkts_cacheable()`, there is no memory ordering guarantee that prevents HW from reading the zero-written `flags` before the TX path writes the final `FLAGS_F`. This is a classic TOCTOU race:
- TX path writes `flags = 0`, then `flags |= FLAGS_F`, then flushes cache.
- Meanwhile, `enetc_clean_tx_ring()` (running on the same or different core) writes `flags = 0` to a BD that is being prepared for transmission.
- If the clean-ring write happens after the TX path sets `FLAGS_F` but before the cache flush, the `dcbf` pushes the zero to memory, and HW sees a descriptor with `FLAGS_F` cleared, causing malformed transmission.

The fix is to never write to a BD that is owned by HW (between `next_to_use` and `hwci`). The `flags = 0` in `enetc_clean_tx_ring()` should only clear BDs that are definitively reclaimed (before `next_to_use`), or the clean-ring and TX-submit paths must use explicit memory barriers to prevent overlap.

**Missing cache maintenance on `enetc_clean_tx_ring` BD clear**  
The patch adds `txbd->flags = 0;` in `enetc_clean_tx_ring()` but does not flush this write to PoC. On a non-cache-coherent system, this zero-write may remain in CPU cache, and when the TX path later writes to the same BD and flushes, it may flush a cache line containing the stale flags value. The TX path should either:
- Skip the BD group flush if the BD was already flushed by clean-ring, or
- `enetc_clean_tx_ring()` must `dcbf` the BD after zeroing flags, so the zero is committed to memory before the TX path reuses the BD.

**Incorrect dcbf granularity in TX path**  
The TX flush loop:
```c
int n = first_bd_idx & ~ENETC_BD_PER_CL_MASK;
int written = (i - n + bd_count) % bd_count;
if (written == 0)
    written = bd_count;
written = (written + ENETC_BD_PER_CL_MASK) & ~ENETC_BD_PER_CL_MASK;
while (written > 0) {
    dcbf((void *)ENETC_TXBD(*tx_ring, n));
    n = (n + ENETC_BD_PER_CL) % bd_count;
    written -= ENETC_BD_PER_CL;
}
```
If `first_bd_idx` is not aligned to `ENETC_BD_PER_CL`, the calculation `n = first_bd_idx & ~ENETC_BD_PER_CL_MASK` rounds DOWN, but the loop increments by `ENETC_BD_PER_CL` starting from `n`. This means the first flush covers BDs before `first_bd_idx` (potentially flushing BDs that were not written in this burst). This is safe (flushing extra clean BDs does not break correctness) but inefficient.

However, if `i` (next_to_use) wraps around the ring during the burst, the `written` calculation may be incorrect. For example:
- `first_bd_idx = bd_count - 2`, `i = 1` (wrapped), `n = bd_count - 4`.
- `written = (1 - (bd_count - 4) + bd_count) % bd_count = 5`.
- Rounding up: `written = 8`.
- The loop flushes BDs from `bd_count - 4` through `bd_count - 1`, then `0` through `3`, but only BDs `bd_count - 2, bd_count - 1, 0` were actually written. BDs `bd_count - 4, bd_count - 3, 1, 2, 3` are flushed unnecessarily (safe but wasteful).

The logic is overly complex and prone to off-by-one errors. A simpler and safer approach is to track the range `[first_bd_idx, i)` and flush each 4-BD-aligned group that intersects this range, handling wrap-around explicitly.

**RX refill flush logic bug: partial group at wrap-around may not be flushed**  
In `enetc_refill_rx_ring()`:
```c
if (unlikely(i == rx_ring->bd_count)) {
    dcbf((void *)grp_start_rxbd);
    i = 0;
    rxbd = ENETC_RXBD(*rx_ring, i);
    rx_swbd = &rx_ring->q_swbd[i];
    grp_start_rxbd = rxbd;
}
```
When the ring wraps (`i == bd_count`), the code flushes the current `grp_start_rxbd`, resets `i` to 0, and sets `grp_start_rxbd = rxbd` (which is now BD 0). However, if the refill continues and writes more BDs starting at index 0, the final trailing flush at line 249:
```c
if (j && (i & ENETC_BD_PER_CL_MASK) != 0)
    dcbf((void *)grp_start_rxbd);
```
only triggers if `i` is misaligned (non-zero `& ENETC_BD_PER_CL_MASK`). But if the refill after wrap-around fills exactly 4, 8, 12, etc. BDs, `i` becomes aligned, and the trailing flush is skipped. Those post-wrap BDs are never flushed to HW.

**Fix:** Always flush `grp_start_rxbd` if `j > 0` and there were any writes in the current group, regardless of alignment:
```c
if (j && (i & ENETC_BD_PER_CL_MASK) != 0)
    dcbf((void *)grp_start_rxbd);
```
should be:
```c
/* Flush trailing partial group if we wrote anything in the final group. */
if (j > 0 && (i - (first_bd_idx & ~ENETC_BD_PER_CL_MASK)) % ENETC_BD_PER_CL != 0)
    dcbf((void *)grp_start_rxbd);
```
Or unconditionally flush after the loop if `j > 0`.

**RX path: reading BD status before invalidate**  
In `enetc_clean_rx_ring_cacheable()`:
```c
dccivac((void *)ENETC_RXBD(*rx_ring, (i & ~(int)ENETC_BD