|WARNING| [v2, 22/22] common/cnxk: fix TM link config selection in debug dump

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

This review covers a 22-patch series from Marvell for the DPDK project, targeting the cnxk (Marvell OCTEON) driver family.

---

## Summary

- **Patches with Issues**: 5
- **Clean Patches**: 17

---

## Patch 01/22: net/cnxk: update mbuf next field for multi segment

**Status**: Clean

**Summary**: Resets `mbuf->next` and `mbuf->nb_segs` fields for multi-segment mbufs on cn9k to comply with `rte_mbuf_raw_reset_bulk()` requirements.

---

## Patch 02/22: common/cnxk: add API of SA valid for cn20k platform

**Status**: Clean

**Summary**: Adds `cnxk_ow_ipsec_inb_sa_valid()` and `cnxk_ow_ipsec_outb_sa_valid()` to query SA validity on CN20K.

---

## Patch 03/22: common/cnxk: additional NIX SQ ctx fields prints

**Status**: **1 Error**

### Error

**Access to potentially NULL pointer without check**

**Location**: `drivers/common/cnxk/roc_nix_debug.c:596`

```c
sq_cnt_ptr = (int64_t *)(uintptr_t)(ctx->sq_count_iova << 3);
if (sq_cnt_ptr && ctx->update_sq_count)
    nix_dump(file, "sq_count value \t\t0x%" PRIx64 "",
             plt_atomic_load_explicit((uint64_t __plt_atomic *)sq_cnt_ptr,
                                      plt_memory_order_relaxed));
```

**Issue**: `ctx->sq_count_iova` is a hardware-provided 57-bit IOVA. When shifted left by 3 and cast to a pointer, it produces a kernel virtual address that may not be mapped in userspace. Dereferencing this pointer without validation can cause a segmentation fault.

**Fix**: Validate that the resulting address is within a known mapped region (e.g., NIX LF BAR) before dereferencing, or remove the dereference entirely if the IOVA is not guaranteed to be userspace-accessible.

---

## Patch 04/22: common/cnxk: update NIX irq handler

**Status**: Clean

**Summary**: Moves queue context dump and register print before interrupt clear in the NIX IRQ handler to preserve error state for debugging.

---

## Patch 05/22: common/cnxk: configure LSO mask for single segments

**Status**: Clean

**Summary**: Configures LSO flag mask for single packets by setting `alt_ssf_set` and `alt_ssf_mask`.

---

## Patch 06/22: net/cnxk: reserve memory for lookup mem at probe

**Status**: **1 Warning**

### Warning

**Missing error message on lookup memory reservation failure**

**Location**: `drivers/net/cnxk/cnxk_ethdev.c:2225`

```c
if (!cnxk_nix_fastpath_lookup_mem_get()) {
    plt_err("Failed to reserve lookup memory rc=%d", rc);
    goto dev_fini;
}
```

**Issue**: `rc` is used in the error message, but `cnxk_nix_fastpath_lookup_mem_get()` returns a pointer, not an error code. The variable `rc` is uninitialized at this point, producing a misleading error message.

**Fix**:
```c
void *lookup_mem = cnxk_nix_fastpath_lookup_mem_get();
if (lookup_mem == NULL) {
    plt_err("Failed to reserve lookup memory");
    rc = -ENOMEM;
    goto dev_fini;
}
```

---

## Patch 07/22: drivers: add support for devargs skip size

**Status**: Clean

**Summary**: Adds `skip_size` devargs to configure the number of bytes to skip before the Ethernet header when parsing incoming packets.

---

## Patch 08/22: net/cnxk: update inbound SA pkind for skip size

**Status**: Clean

**Summary**: Updates inbound SA pkind using `roc_npc_skip_size_pkind_get()` during session create/update to support skip size configuration.

---

## Patch 09/22: common/cnxk: fix cnxk xstats names

**Status**: Clean

**Summary**: Prevents out-of-bounds writes in `roc_nix_xstats_names_get()` by returning the required count when the buffer is too small.

---

## Patch 10/22: common/cnxk: fix event type for soft expiry

**Status**: **1 Error**

### Error

**Incorrect initialization value for event type tag**

**Location**: `drivers/common/cnxk/roc_nix_inl_dev.c:1237`

```c
uint64_t tmp[2];
tmp[0] = ~0ULL;
inl_dev->work_cb(tmp, sa, NIX_INL_SOFT_EXPIRY_THRD, NULL, port_id);
```

**Issue**: The commit message states "Fix event type to default for inline soft expiry processing," but the code sets `tmp[0]` to `~0ULL` (all bits set). The W0 format for SSO events uses bits 28-31 for the event type tag. Setting all bits to 1 produces an event type of 0xF, which is not "default"--it's the maximum tag value. The intended default is likely 0 (RTE_EVENT_TYPE_ETHDEV).

**Fix**:
```c
uint64_t tmp[2];
tmp[0] = 0;  /* RTE_EVENT_TYPE_ETHDEV */
tmp[1] = 0;
inl_dev->work_cb(tmp, sa, NIX_INL_SOFT_EXPIRY_THRD, NULL, port_id);
```

---

## Patch 11/22: net/cnxk: enable CPT CQ by default for inline IPsec

**Status**: Clean

**Summary**: Changes the default value of `cpt_cq_enable` devarg from 0 to 1 for CN20K, enabling hardware-based completion notification.

---

## Patch 12/22: net/cnxk: fix unsigned integer underflow in LSO calculation

**Status**: Clean

**Summary**: Replaces branchless mask-based selection with ternary operator to resolve Coverity unsigned integer underflow warning in LSO offset calculation.

---

## Patch 13/22: net/cnxk: derive ethdev from SA for inbound CPT CQ events

**Status**: Clean

**Summary**: Adds `eth_dev` back pointer to `cnxk_eth_sec_sess` to resolve ethdev for inbound CPT CQ events when `port_id` is unavailable.

---

## Patch 14/22: net/cnxk: fix bitwise operand size mismatch in link mode

**Status**: Clean

**Summary**: Casts enum `roc_nix_link_mode` values to `uint64_t` before bitwise OR with `uint64_t advertise` variable.

---

## Patch 15/22: common/cnxk: add cipher key length check in key set

**Status**: Clean

**Summary**: Adds upper-bound checks before `memcpy` into `encr_key[32]` in `roc_se_ciph_key_set()` to prevent buffer overflow.

---

## Patch 16/22: common/cnxk: fix Klocwork static analysis issues

**Status**: **1 Warning**

### Warning

**Inconsistent error path cleanup order**

**Location**: `drivers/common/cnxk/roc_dev.c:1807-1825`

```c
vf_flr_unregister:
    if (!is_vf)
        dev_vf_flr_unregister_irqs(pci_dev, dev);
stop_msg_thrd:
    if (dev->sync.start_thread) {
        dev->sync.start_thread = false;
        plt_thread_join(dev->sync.pfvf_msg_thread, NULL);
    }
thread_fail:
    if (pci_dev->max_vfs > 0) {
        pthread_mutex_destroy(&dev->sync.mutex);
        pthread_cond_destroy(&dev->sync.pfvf_msg_cond);
    }
iounmap:
    dev_vf_mbase_put(pci_dev, vf_mbase);
    mbox_fini(&dev->mbox_vfpf);
    mbox_fini(&dev->mbox_vfpf_up);
```

**Issue**: The new `vf_flr_unregister` label skips thread cleanup but falls through to `thread_fail`, which may destroy uninitialized mutexes if `max_vfs == 0`. The ordering suggests that threads should be stopped before VF FLR unregistration, but the code does the opposite.

**Suggested Fix**: Verify the intended cleanup order. If threads depend on VF FLR state, swap the labels:
```c
stop_msg_thrd:
    if (dev->sync.start_thread) { ... }
vf_flr_unregister:
    if (!is_vf)
        dev_vf_flr_unregister_irqs(pci_dev, dev);
thread_fail:
    if (pci_dev->max_vfs > 0) { ... }
```

---

## Patch 17/22: common/cnxk: add auth key len check in inbound SA

**Status**: Clean

**Summary**: Adds auth key length validation before `memcpy` in `cnxk_on_ipsec_inb_sa_create()` to prevent buffer overflow.

---

## Patch 18/22: common/cnxk: add FEC configuration support

**Status**: Clean

**Summary**: Adds ROC APIs `roc_nix_mac_fec_set` and `roc_nix_mac_fec_supported_get` for Forward Error Correction configuration.

---

## Patch 19/22: net/cnxk: add FEC get set and capability ops

**Status**: **1 Error**

### Error

**Uninitialized variable used in error return path**

**Location**: `drivers/net/cnxk/cnxk_ethdev_ops.c:1460-1463`

```c
int
cnxk_nix_fec_get_capability(struct rte_eth_dev *eth_dev, ...)
{
    ...
    uint64_t supported_fec = 0;
    int rc;

    rc = roc_nix_mac_fec_supported_get(nix, &supported_fec);
    if (rc == 0 && supported_fec != 0) {
        ...
        return 1;
    }

    return rc;  /* BUG: rc may be uninitialized if fec_supported_get fails */
}
```

**Issue**: If `roc_nix_mac_fec_supported_get()` returns a non-zero `rc` (failure), the function returns `rc`. However, the code path suggests that when firmware does not provide FEC info (`supported_fec == 0`), the function should not return an error but rather indicate no capabilities. The current code conflates "firmware does not support FEC query" with "error".

**Fix**:
```c
rc = roc_nix_mac_fec_supported_get(nix, &supported_fec);
if (rc < 0)
    return rc;  /* Actual error */

if (supported_fec != 0) {
    /* Return capabilities from firmware */
    ...
    return 1;
}

/* Firmware has no FEC info; return 0 capabilities */
return 0;
```

---

## Patch 20/22: event/cnxk: fix Klocwork static analysis issues

**Status**: Clean

**Summary**: Casts `uint16_t` operands to `uint64_t` before bitwise OR with `uint64_t rx_offloads`; adds NULL check for bracket parser end pointer.

---

## Patch 21/22: crypto/cnxk: enforce DES/3DES cipher key length

**Status**: Clean

**Summary**: Enforces exact key length match for DES/3DES algorithms in `fill_sess_cipher()` (8 or 24 bytes), preventing oversized keys.

---

## Patch 22/22: common/cnxk: fix TM link config selection in debug dump

**Status**: **1 Warning**

### Warning

**Potential array out-of-bounds if nix->tx_link is large**

**Location**: `drivers/common/cnxk/roc_nix_debug.c:1234-1237`

```c
if (!nix->sdp_link &&
    nix->tm_link_cfg_lvl == NIX_TXSCH_LVL_TL3) {
    reg[k] = NIX_AF_TL3_TL2X_LINKX_CFG(schq, nix->tx_link);
    snprintf(regstr[k++], NIX_REG_NAME_SZ,
             "NIX_AF_TL3_TL2[%u]_LINK[%u]_CFG", schq,
             nix->tx_link);
}
```

**Issue**: `nix->tx_link` is a 16-bit value (`uint16_t`). The `NIX_AF_TL3_TL2X_LINKX_CFG` macro likely computes a register offset using `schq` and `link` as array indices. If `nix->tx_link` exceeds the hardware's maximum link index (typically 0-15 or 0-31), the macro may compute an out-of-bounds offset, reading garbage or causing a fault.

**Suggested Fix**: Add a range check:
```c
if (nix->tx_link >= NIX_MAX_LINK) {
    plt_warn("tx_link %u exceeds max %u, skipping link cfg dump",
             nix->tx_link, NIX_MAX_LINK - 1);
} else {
    reg[k] = NIX_AF_TL3_TL2X_LINKX_CFG(schq, nix->tx_link);
    ...
}
```

---

## Final Recommendation

- **Patch 03**: Fix NULL pointer dereference of `sq_cnt_ptr`.
- **Patch 06**: Initialize `rc` or remove it from error message.
- **Patch 10**: Correct `tmp[0]` initialization to 0 for default event type.
- **Patch 16**: Verify error path cleanup order.
- **Patch 19**: Handle `roc_nix_mac_fec_supported_get` return value correctly.
- **Patch 22**: Add range check for `nix->tx_link`.

All other patches are clean and follow DPDK coding standards.