|WARNING| [2/2] net/mana: add hardware TSO support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/2: net/mana: fix transmit burst error handling

### Errors

**Double-free on transmitted packets after error (Error):**

The fix changes `continue` to `break` on error, which prevents subsequent packets from being transmitted while the caller treats them as unsent. However, the function does not skip the `pkt_sent++` increment for the failed packet. The result is:

1. Loop breaks on packet `i` (which failed)
2. `pkt_sent` includes packet `i` in the count
3. Function returns `pkt_sent` to caller
4. Caller assumes `tx_pkts[0..pkt_sent-1]` were transmitted, including the failed packet `i`
5. Caller does NOT free packet `i` (assumes it was sent)
6. Packet `i` was never actually queued to hardware

This creates a **resource leak** of the failed packet rather than a double-free. The failed packet is lost -- neither freed by the caller nor queued to hardware.

**Fix:** Increment `pkt_sent` only after successful `gdma_post_work_request()`:

```c
ret = gdma_post_work_request(&txq->gdma_sq, &work_req, &wqe_size_in_bu);
if (ret) {
    DP_LOG(ERR, "WQ post failed to queue %u", pkt_idx);
    break;
}

/* Only count as sent if successfully queued */
txq->desc_ring[txq->desc_ring_tail].pkt = m_pkt;
txq->desc_ring_tail = (txq->desc_ring_tail + 1) % txq->num_desc;
txq->desc_ring_len++;
pkt_sent++;  /* Move increment here */
```

Currently the patch breaks on the two error paths shown (max_send_sge, SGL population failure) but the increment happens unconditionally in the loop body after those checks, so the failed packet is counted as sent.

---

## PATCH 2/2: net/mana: add hardware TSO support

### Errors

**Resource leak on TSO SGL build failure (Error):**

Line 516-520 in the TSO branch:
```c
if (unlikely(m_seg != NULL || num_sge < 2)) {
    DP_LOG(ERR, "failed to build TSO SGL, num_sge %u", num_sge);
    break;
}
```

When this `break` is taken, `m_pkt` (the current packet) has not been transmitted or freed. The function returns `pkt_sent`, and the caller assumes `tx_pkts[pkt_sent]` (this packet) was not sent and will free it. However, if the loop broke inside the `while (m_seg)` loop due to exceeding `MAX_SGL_ENTRIES_FOR_TRANSMIT` or MR allocation failure, the `m_seg != NULL` check triggers the outer break, but the packet is not accounted for. This is consistent with the pattern after the fix in patch 1/2 (break on error, don't count packet), so **this is actually correct** given the fix in patch 1/2. Rescinding this item.

**Incorrect TSO header fragmentation check (Error):**

Line 286-289:
```c
if (unlikely(m_pkt->data_len < gso_hs)) {
    DP_LOG(ERR, "TSO header cannot be fragmented");
    break;
}
```

This check rejects TSO when the TSO header (L2+L3+L4) spans multiple segments. However, the code at line 344-353 explicitly handles the case where the first mbuf contains only the header (`m_pkt->data_len == gso_hs`) by emitting only SGE0 with the header. The fragmentation case where `m_pkt->data_len < gso_hs` (header split across mbufs) is never handled -- there is no code to gather header bytes from subsequent segments into SGE0.

The check is therefore **correct** -- TSO header fragmentation is not supported by the implementation. However, the error path does not free the packet or increment a drop counter, so the packet is silently lost (same resource leak pattern as patch 1/2). The break is correct (don't count unsent packet), but a drop statistic should be incremented.

Not a correctness bug per se (the check is right), but the error handling is incomplete (no stats). Given the pattern in the existing code (other breaks also lack stats), this is Info not Error.

**Missing validation of tso_segsz (Error):**

Line 291 checks:
```c
if (likely(gso_hs != 0 && m_pkt->pkt_len > gso_hs && m_pkt->tso_segsz != 0))
    is_tso = true;
```

This allows `m_pkt->tso_segsz` to be zero when `RTE_MBUF_F_TX_TCP_SEG` is set but the packet is too short or malformed. Later, at line 582, `work_req.client_data_unit = m_pkt->tso_segsz` assigns zero to the hardware TSO segment size, which is invalid.

The check should either:
1. Reject TSO when `tso_segsz == 0` (break with error), or
2. Ensure `work_req.client_data_unit` is not set to zero

The current code silently falls through to non-TSO path when `is_tso` remains false, which is acceptable. However, if a future maintainer changes the condition or if the hardware interprets `client_data_unit = 0` incorrectly, this could cause undefined behavior.

This is a **potential correctness bug**: if `tso_segsz == 0` makes it to line 582 (e.g. if the `likely()` condition is true but `tso_segsz` is still zero due to a logic error), the hardware receives an invalid configuration. The current logic prevents this (the condition is conservative), but it's fragile.

**Recommended fix:** Add explicit validation:
```c
if (m_pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
    gso_hs = m_pkt->l2_len + m_pkt->l3_len + m_pkt->l4_len;
    if (unlikely(m_pkt->data_len < gso_hs)) {
        DP_LOG(ERR, "TSO header cannot be fragmented");
        break;
    }
    if (unlikely(gso_hs == 0 || m_pkt->pkt_len <= gso_hs ||
                 m_pkt->tso_segsz == 0)) {
        DP_LOG(ERR, "Invalid TSO parameters");
        break;
    }
    is_tso = true;
}
```

**Unbounded loop in TSO segment traversal (Error):**

Line 361-377 traverses `m_seg->next` in a `while (m_seg)` loop with no iteration bound. If the mbuf chain is circular (corrupted) or if `m_pkt->nb_segs` is wrong, this loop may never terminate.

**Fix:** Cap iterations at `m_pkt->nb_segs`:
```c
uint16_t seg_count = 1; /* Already processed first mbuf */
m_seg = m_pkt->next;
while (m_seg && seg_count < m_pkt->nb_segs) {
    /* existing code */
    num_sge++;
    m_seg = m_seg->next;
    seg_count++;
}
```

This is a hardening suggestion in most cases (mbuf chains are trusted), but DPDK guidelines require bounded traversal when the chain could theoretically be corrupted. Given that TSO is new code, add the bound.

### Warnings

**Incorrect expected_sge calculation (Warning):**

Line 296-297:
```c
expected_sge = m_pkt->nb_segs + (is_tso && m_pkt->data_len > gso_hs ? 1 : 0);
```

This adds 1 when TSO splits the first mbuf into header and payload SGEs. However, the actual SGE count after the TSO SGL build (line 330-379) can be:
- `num_sge = 1` if first mbuf is exactly the header (`data_len == gso_hs`)
- `num_sge = 2` if first mbuf contains header + payload
- `num_sge = 2 + (nb_segs - 1)` if there are additional segments

The formula should be:
```c
expected_sge = is_tso ? (m_pkt->nb_segs + (m_pkt->data_len > gso_hs ? 1 : 0)) :
                        m_pkt->nb_segs;
```

Wait, let me re-examine. If `is_tso` is true:
- First mbuf: contributes 1 SGE (header) + (1 if data_len > gso_hs else 0) = 1 or 2 SGEs
- Remaining mbufs: `m_pkt->nb_segs - 1` SGEs
- Total: `1 + (data_len > gso_hs ? 1 : 0) + (nb_segs - 1)` = `nb_segs + (data_len > gso_hs ? 1 : 0)`

The calculation is **correct**. Rescinding this item.

**No release notes update (Warning):**

The patch adds a new feature (TSO) and updates `mana.ini`, but does not add a release notes entry. Per guidelines, new features require release notes.

**Missing testpmd command or test (Warning):**

New Tx offload (TSO) should have testpmd integration or a test case in `app/test`. The patch does not add either.

### Info

**TSO checksum computation forced even when not requested (Info):**

Line 356-358:
```c
tx_oob.short_oob.tx_compute_IP_header_checksum =
    (m_pkt->ol_flags & RTE_MBUF_F_TX_IPV4) ? 1 : 0;
tx_oob.short_oob.tx_compute_TCP_checksum = 1;
```

When TSO is enabled, the code unconditionally sets `tx_compute_TCP_checksum = 1`, even if the mbuf flags do not include `RTE_MBUF_F_TX_TCP_CKSUM`. This is likely correct for TSO (hardware must compute checksums for segmented packets), but it's a deviation from the application's request. Worth noting.

**Pre-existing: missing stats increment on error paths (Info, pre-existing):**

All the `break` statements on error (existing and new) do not increment any error counters (`oerrors`, `odropped`). The device statistics will not reflect dropped packets. This pattern exists in the original code and is not introduced by this patch.

---

## Summary

**Patch 1/2:**
- **Error:** Failed packet counted as sent (resource leak). Needs to move `pkt_sent++` after successful `gdma_post_work_request()`.

**Patch 2/2:**
- **Error:** No validation that `tso_segsz != 0` before assigning to `client_data_unit`. Could send zero segment size to hardware if TSO flag is set but packet is malformed.
- **Error:** Unbounded loop in TSO segment traversal. Add iteration cap at `m_pkt->nb_segs`.
- **Warning:** Missing release notes for new TSO feature.
- **Warning:** No testpmd integration or functional test for TSO.
- **Info:** TSO forces TCP checksum computation even if not requested (may be correct for TSO).
- **Info:** Error paths do not increment stats (pre-existing pattern, not introduced by patch).

Review-Result: ERROR