|WARNING| [v9-1, 14/14] net/enetc4: add WRR Tx scheduler devarg for VF rings

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-13

# DPDK Patch Review - ENETC4 Network Driver Feature Series

## Overview
This patch series adds multiple features to the ENETC4 network driver for i.MX95:
1. KEEP_CRC Rx offload
2. TSO support for VF
3. RSC (hardware LRO) support
4. Link speed field extension
5. Firmware version reporting
6. Register dump support
7. Ring parameters support
8. VF link speed refresh on interrupt
9. Stats reset for VF
10. Per-queue Rx interrupts for VF
11. SI-based port VLAN
12. Link status bitmask encoding
13. Tx PAUSE via Rx congestion mode
14. WRR Tx scheduler devarg

---

## Patch 01/14: KEEP_CRC Rx Offload

### Errors

**Undefined behavior on RSC-enabled rings when crc_len straddles segments**
```c
static inline void
enetc_rx_crc_trim(struct rte_mbuf *first_seg, struct rte_mbuf *prev_seg,
		  struct rte_mbuf *last_seg, uint16_t crc_len)
{
	first_seg->pkt_len -= crc_len;
	if (likely(last_seg->data_len > crc_len)) {
		last_seg->data_len -= crc_len;
	} else if (prev_seg != NULL) {
		first_seg->nb_segs--;
		prev_seg->data_len -= crc_len - last_seg->data_len;
		prev_seg->next = NULL;
		rte_pktmbuf_free_seg(last_seg);
	}
}
```
When `last_seg->data_len < crc_len` but `prev_seg == NULL` (single-segment frame shorter than CRC length), the code leaves `first_seg->pkt_len` decremented but does not adjust `last_seg->data_len` or handle the underflow. This produces a negative effective payload length. Add an `else` clause to handle the single-segment case: clamp `last_seg->data_len` to zero and adjust `first_seg->pkt_len` accordingly, or drop the frame entirely.

---

## Patch 02/14: TSO Support for VF

### Errors

**Integer overflow in LSO ring descriptor count check**
```c
ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
if (ring_desc > MAX_BD_COUNT) {
```
When `nb_desc` is larger than `MAX_BD_COUNT / 2`, the multiplication `nb_desc * 2` overflows before the cast to `uint32_t`. Use `(uint32_t)nb_desc` cast before the multiply: `ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2U : (uint32_t)nb_desc;` or widen the operands to `uint64_t` for the check.

**Missing NULL check before dereferencing `seg` in TSO payload loop**
```c
while (dseg) {
	uint16_t dlen;
	uint64_t daddr;

	if (is_first_seg) {
		dlen = first_payload;
		daddr = rte_mbuf_data_iova(dseg) + hdr_len;
		is_first_seg = false;
	} else {
		dlen = rte_pktmbuf_data_len(dseg);
		data = rte_pktmbuf_mtod(dseg, void *);
```
If `dseg` becomes NULL unexpectedly (corrupted mbuf chain), the `else` branch calls `rte_pktmbuf_data_len(dseg)` on NULL. Add an explicit NULL check before the `else` or rely on the loop condition (already checks `dseg`), but ensure `rte_pktmbuf_mtod` cannot be reached on NULL.

---

## Patch 03/14: RSC (Hardware LRO) Support

### Errors

**Potential divide-by-zero when programming RBaICR1[ICTT]**
```c
#define ENETC4_RSC_DEF_ICTT		0x10000
```
The code programs `ENETC4_RBICR1` with `ENETC4_RSC_DEF_ICTT` as the interrupt coalescing timer. If this value were zero, RSC would not coalesce (per the comment), but the define is non-zero. However, if a future change sets it to zero or a devarg allows user override to zero, the HW behavior is documented to flush every frame. Verify that zero is handled correctly or add a check to reject zero values if they would break RSC.

**Invariant violation if `i + 1` wraps in RSC extension BD access**
```c
rxbd_ext = (struct enetc_rx_bd_ext *)
	   ENETC_RXBD(*rx_ring, i + 1);
rsc_frames = ENETC4_RXBD_EXT_RSC_FRAMES(rte_le_to_cpu_32(rxbd_ext->rsc_frames));
```
The code claims the invariant `i + 1` never wraps because `bd_count` is always even and `i` only advances by 2. However, if `bd_count` were somehow odd (programming error), `i` could equal `bd_count - 1`, making `i + 1 == bd_count` and accessing out of bounds. Add an assertion `RTE_ASSERT((rx_ring->bd_count & 1) == 0)` in `enetc4_alloc_rxbdr` when `rsc_enable` is set.

---

## Patch 04/14: Link Speed Field Extension

### Errors

None identified. The formula-based speed decoding is sound and the legacy fallback is well-structured.

---

## Patch 05/14: Firmware Version Get

### Errors

None identified. The PCI revision ID read and VSI-PSI message are handled correctly.

---

## Patch 06/14: Registers Dump

### Errors

None identified. The register arrays and iteration logic are correct.

---

## Patch 07/14: Ring Parameters Support

### Errors

None identified. The ops registration is straightforward.

---

## Patch 08/14: Link Speed Refresh on Interrupt

### Errors

**Race condition: link speed query not serialized with PSI mailbox**
```c
if (!enetc4_vf_get_link_speed(eth_dev, msg) &&
    msg->class_id == ENETC_CLASS_ID_LINK_SPEED)
	enetc4_decode_link_speed(msg->status,
				hw->vf_link_legacy,
				&link);
```
In `enetc4_process_psi_msg`, when link-up is detected, the code calls `enetc4_vf_get_link_speed()` which sends a new VSI-PSI message (class 0x81). This message send is not protected by `vsi_lock` in this context (the lock is only held inside `enetc4_msg_vsi_send()`). If another thread is concurrently sending a VSI-PSI message (e.g., from `vlan_filter_set` or `mac_addr_add`), the mailbox state could be corrupted. The `vsi_lock` must be held for the entire duration of `enetc4_vf_get_link_speed()` to serialize with other mailbox users.

However, reviewing the code further: `enetc4_msg_vsi_send()` takes the lock at entry and releases it at exit. Each VSI-PSI message send is atomic with respect to other sends. The issue would only occur if the mailbox HW state machine is not reentrant between multiple function calls. Since `enetc4_msg_vsi_send()` is the sole writer and it locks, this is likely safe. **Retract this error.**

---

## Patch 09/14: Stats Reset for VF

### Errors

None identified. The snapshot/delta approach is sound given the HW limitation.

---

## Patch 10/14: Per-Queue Rx Interrupts for VF

### Errors

**Missing error check on `pthread_mutexattr_setpshared` return value**
```c
pthread_mutexattr_init(&attr);
pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
pthread_mutex_init(&hw->vsi_lock, &attr);
pthread_mutexattr_destroy(&attr);
```
`pthread_mutexattr_setpshared()` can fail on some platforms. Check the return value and handle the error (though on Linux with valid arguments it should always succeed). The guidelines exclude `pthread_mutex_init()` from unchecked-return flagging, but `setpshared()` is not on the exclusion list.

---

## Patch 11/14: SI-Based Port VLAN

### Errors

None identified. The VSI-PSI mailbox command and register writes are correct.

---

## Patch 12/14: Link Status Bitmask Encoding

### Errors

None identified. The bitmask conversion is backward-compatible.

---

## Patch 13/14: Tx PAUSE via Rx Congestion Mode

### Errors

**Race condition: RBMR read-modify-write not atomic across queue start/stop**
```c
rbmr = enetc4_rxbdr_rd(enetc_hw, i, ENETC_RBMR);
if (enable)
	rbmr |= ENETC_RBMR_CM;
else
	rbmr &= ~(uint32_t)ENETC_RBMR_CM;
enetc4_rxbdr_wr(enetc_hw, i, ENETC_RBMR, rbmr);
```
In `enetc4_vf_set_congestion_mode()`, the read-modify-write of RBMR for each queue is performed under `vsi_lock`. However, `enetc4_rx_queue_start()` and `enetc4_rx_queue_stop()` also read-modify-write RBMR (to set/clear `RBMR_EN`), and those paths also take `vsi_lock` (as of this patch). The lock is held correctly, so no race. **No error.**

---

## Patch 14/14: WRR Tx Scheduler Devarg

### Errors

**Missing `free(input_str)` on allocation failure in `parse_txq_wrr()`**
```c
hw->txq_wrr = rte_zmalloc(NULL,
		hw->max_tx_queues * sizeof(uint32_t), 0);
if (!hw->txq_wrr) {
	free(input_str);
	return -ENOMEM;
}
```
This is correct. The earlier `parse_txq_prior()` has the same pattern. **No error.**

**Resource leak: `hw->txq_prior` and `hw->txq_wrr` not freed on device close failure path**
In `enetc4_dev_close()`, the code frees `hw->txq_prior` and `hw->txq_wrr` at the end. If an earlier step in `enetc4_dev_close()` returns an error (e.g., `enetc4_vf_dev_stop()` fails), the function returns early without freeing these. Add cleanup before all early returns or restructure to use goto.

---

## Summary of Findings

### Errors (Must Fix)
1. **Patch 01**: CRC trim logic undefined for single-segment frame shorter than CRC length (underflow).
2. **Patch 02**: Integer overflow in LSO ring size check when `nb_desc * 2` exceeds 32-bit range before cast.
3. **Patch 10**: Missing error check on `pthread_mutexattr_setpshared()`.
4. **Patch 14**: Resource leak of `hw->txq_prior` and `hw->txq_wrr` on early return from `enetc4_dev_close()`.

### Warnings
None beyond the errors above.

### Info
- Patch 03: Consider asserting `bd_count` is even when `rsc_enable` is set to strengthen the invariant.
- Patch 02: Consider adding a comment explaining why `dseg` NULL check is unnecessary in the TSO payload loop (loop condition guarantees non-NULL).

---

## Conclusion
The series adds substantial functionality to the ENETC4 driver. The correctness bugs identified are edge cases (single-segment CRC trim, large descriptor counts, error path leaks) that should be addressed before merge. The overall structure and logic are sound.