|WARNING| [4/4] test/crypto: add unit test for Rx inject multi seg

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/4: test/crypto: add asymmetric sessionless test case

### Errors

**Resource leak on error path**

In `test_mod_exp_sessionless()`, the crypto operation allocated at line 1226 is not freed on several error paths:

```c
op = rte_crypto_op_alloc(op_mpool, RTE_CRYPTO_OP_TYPE_ASYMMETRIC);
if (!op) {
    RTE_LOG(ERR, USER1, "line %u FAILED: %s", __LINE__,
        "Failed to allocate asymmetric crypto operation struct");
    return TEST_FAILED;  // BUG: 'op' not freed
}
```

If the function returns `TEST_SKIPPED` at lines 1203, 1206, 1212, or 1222, the allocated `op` leaks.

**Fix**: Move `rte_crypto_op_alloc()` after all capability checks that can return `TEST_SKIPPED`, or add `rte_crypto_op_free(op)` before each early return.

---

**Session pool initialization moved without justification**

In `testsuite_setup()`, the session pool creation was moved from after queue setup (original location) to before it. The new order is:

1. Device configure
2. Session pool create - **moved here**
3. Queue setup (which references `ts_params->session_mpool`)

While functionally correct (the pool is now available when queue setup needs it), this reordering should be mentioned in the commit message if intentional. If the original code worked, this change may indicate the queue setup was not actually using the pool, making the reorder unnecessary.

---

### Warnings

**Missing release notes**

The patch adds a new test case (`test_mod_exp_sessionless`) but does not update release notes. While test-only changes do not strictly require release notes, this test exercises a new code path (sessionless operations) that may be worth documenting if it reveals a feature not previously tested.

---

**Inconsistent error message formatting**

At line 1207, the error message uses a different format than the rest of the function:

```c
RTE_LOG(ERR, USER1, "Unsupported MODULUS length specified\n");
```

vs. the pattern used elsewhere in the same function:

```c
RTE_LOG(ERR, USER1, "line %u FAILED: %s", __LINE__, "Error sending packet...");
```

Prefer the `line %u FAILED:` format for consistency within the function.

---

## Patch 2/4: test/crypto: add asym autotest support for cn20k

### Info

No issues found. The patch correctly registers a new test function following the existing pattern for other device types.

---

## Patch 3/4: test/crypto: add autotest support for cn20k

### Warnings

**Missing definition of CRYPTODEV_NAME_CN20K_PMD**

The patch adds `CRYPTODEV_NAME_CN20K_PMD` to `test_cryptodev.h` at line 79, but the actual PMD driver name definition is not shown. Verify that `crypto_cn20k` is the correct driver name string used by the cn20k PMD. If the driver has not yet been merged, this creates a forward dependency.

---

## Patch 4/4: test/crypto: add unit test for Rx inject multi seg

### Errors

**Missing RTE_ETH_TX_OFFLOAD_SECURITY offload**

In `ut_setup_security_rx_inject()`, the TX offloads are configured as:

```c
.txmode = {
    .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
},
```

However, security operations typically require `RTE_ETH_TX_OFFLOAD_SECURITY` on the TX side as well. The RX side correctly has `RTE_ETH_RX_OFFLOAD_SECURITY`. If this setup function is shared by bidirectional tests or if the test performs any TX operations, the missing TX security offload may cause failures.

**Fix**: Add `RTE_ETH_TX_OFFLOAD_SECURITY` to `.txmode.offloads`:

```c
.txmode = {
    .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE |
                RTE_ETH_TX_OFFLOAD_SECURITY,
},
```

---

### Warnings

**Hardcoded nb_segs_in_mbuf value without explanation**

The new test sets `flags.nb_segs_in_mbuf = 4` at line 10793. This appears arbitrary. Consider:

- Adding a comment explaining why 4 segments is the chosen test value
- Using a `#define` constant if this value is reused or has significance
- Verifying that the test framework and mbuf pool can handle 4 segments

---

**Test only differs from existing test by one flag**

The new test `test_ipsec_proto_known_vec_inb_rx_inject_multi_seg()` is nearly identical to `test_ipsec_proto_known_vec_inb_rx_inject()` (only difference: `flags.nb_segs_in_mbuf = 4`). Consider refactoring to reduce duplication:

```c
static int
test_ipsec_proto_known_vec_inb_rx_inject_impl(int nb_segs)
{
    struct ipsec_test_flags flags;
    /* ... */
    flags.rx_inject = true;
    flags.nb_segs_in_mbuf = nb_segs;
    /* ... */
}

static int
test_ipsec_proto_known_vec_inb_rx_inject(const void *test_data)
{
    return test_ipsec_proto_known_vec_inb_rx_inject_impl(0);
}

static int
test_ipsec_proto_known_vec_inb_rx_inject_multi_seg(const void *test_data)
{
    return test_ipsec_proto_known_vec_inb_rx_inject_impl(4);
}
```

This reduces copy-paste errors and makes the difference between tests explicit.

---

## Summary

### Critical Issues (must fix before merge)
- **Patch 1**: Resource leak in `test_mod_exp_sessionless()` when early returns occur after `rte_crypto_op_alloc()`
- **Patch 4**: Missing `RTE_ETH_TX_OFFLOAD_SECURITY` in TX offloads may cause test failures

### Should Fix
- **Patch 1**: Session pool initialization reorder should be explained or reverted
- **Patch 1**: Use consistent error message formatting
- **Patch 3**: Verify `CRYPTODEV_NAME_CN20K_PMD` matches actual driver name
- **Patch 4**: Document or define `nb_segs_in_mbuf = 4` choice
- **Patch 4**: Refactor duplicated test code