|WARNING| [v2, 4/4] net/af_packet: fix QinQ outer TPID on VLAN reinsertion

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/4: net: fix VLAN insert doc comment for shared-mbuf error code

### Documentation
**Warning**: The updated doc comment mentions "mbuf's first segment is too short" as a reason for `-EINVAL`, but this condition is not checked in the implementation. The implementation only returns `-EINVAL` for shared/indirect mbufs, and `-ENOSPC` for headroom issues.

```c
/* Current implementation (lib/net/rte_ether.h, rte_vlan_insert): */
if (rte_mbuf_refcnt_read(*m) > 1 || RTE_MBUF_DIRECT(*m) == 0)
    return -EINVAL;  /* Only this check returns -EINVAL */

if (rte_pktmbuf_headroom(*m) < sizeof(*vh))
    return -ENOSPC;  /* Headroom check returns -ENOSPC, not -EINVAL */
```

The doc comment should not mention "mbuf's first segment is too short" as an `-EINVAL` condition. Either remove that phrase or clarify that it's also an `-ENOSPC` case.

## PATCH 2/4: net/af_packet: make Rx VLAN stripped flag handling explicit

No issues found.

## PATCH 3/4: net: add VLAN insert function with a TPID argument

### API Design
**Error**: New experimental function `rte_vlan_insert_tpid()` is not marked with the `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macro in a corresponding `.c` file. Since this is a static inline function in a header, it has no implementation file and does not require an export macro. However, the function must still be documented in the release notes (which it is) and included in the experimental API section of the library's version map.

Actually, upon closer inspection: static inline functions do not need `RTE_EXPORT_*` macros since they are not symbols in the library binary. The `__rte_experimental` attribute is sufficient. This is correct as-is.

### Testing
**Info**: The new tests `test_vlan_insert_8021q` and `test_vlan_insert_tpid` correctly verify the new TPID functionality. Good test coverage.

### Code Review

In `__rte_vlan_insert()`:
```c
static inline int __rte_vlan_insert(struct rte_mbuf **m, uint16_t tpid)
{
    /* ... */
    nh->ether_type = rte_cpu_to_be_16(tpid);
    /* ... */
}
```

The `tpid` parameter is documented as "host order" in the Doxygen comment for `rte_vlan_insert_tpid()`, and is correctly converted to network order with `rte_cpu_to_be_16()`. This is consistent and correct.

## PATCH 4/4: net/af_packet: fix QinQ outer TPID on VLAN reinsertion

### Correctness
**Error**: The code reads `ppd->tp_vlan_tpid` when `TP_STATUS_VLAN_TPID_VALID` is set, but does not convert it from network byte order to host byte order. The `rte_vlan_insert_tpid()` function expects a TPID in host order (per its Doxygen comment: "Tag Protocol Identifier to insert (host order)"), and internally converts it to network order with `rte_cpu_to_be_16(tpid)`.

However, `ppd->tp_vlan_tpid` is a kernel-provided value in the `tpacket2_hdr` structure. Linux kernel documentation and the af_packet(7) man page indicate that `tp_vlan_tpid` is in **network byte order** (big-endian), matching the wire format.

The code passes this value directly to `rte_vlan_insert_tpid()` without conversion:

```c
tpid = (ppd->tp_status & TP_STATUS_VLAN_TPID_VALID) ?
    ppd->tp_vlan_tpid : RTE_ETHER_TYPE_VLAN;
if (rte_vlan_insert_tpid(&mbuf, tpid) != 0) {
```

Since `rte_vlan_insert_tpid()` expects host order and calls `rte_cpu_to_be_16(tpid)`, but `ppd->tp_vlan_tpid` is already in network order, this will double-swap on little-endian systems, producing the wrong byte order.

**Fix**: Convert `ppd->tp_vlan_tpid` to host order before passing to `rte_vlan_insert_tpid()`:

```c
tpid = (ppd->tp_status & TP_STATUS_VLAN_TPID_VALID) ?
    rte_be_to_cpu_16(ppd->tp_vlan_tpid) : RTE_ETHER_TYPE_VLAN;
if (rte_vlan_insert_tpid(&mbuf, tpid) != 0) {
```

### Style
The patch correctly uses explicit comparison for the error check (`!= 0`), consistent with DPDK guidelines.

---

## Summary by Severity

### Errors
1. **PATCH 4/4**: Missing byte order conversion on `ppd->tp_vlan_tpid` before passing to `rte_vlan_insert_tpid()`. The kernel provides the TPID in network order, but `rte_vlan_insert_tpid()` expects host order. This will produce incorrect TPID values on little-endian systems.

### Warnings
1. **PATCH 1/4**: Doc comment lists "mbuf's first segment is too short" as a reason for returning `-EINVAL`, but the implementation only returns `-ENOSPC` for length issues. The doc comment is misleading.

### Info
- **PATCH 3/4**: Good test coverage for the new TPID functionality.