|WARNING| [v3,2/2] net/cnxk: add FEC get set and capability ops

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/2: common/cnxk: add FEC configuration support

### Errors

**1. Resource leak on error path in `roc_nix_mac_fec_set()`**

The function calls `mbox_get()` to acquire the mailbox, then allocates a message with `mbox_alloc_msg_cgx_set_fec_param()`. If the allocation fails (`req == NULL`), the code jumps to `exit` and calls `mbox_put()`, which is correct. However, there is a potential issue with error handling flow:

```c
req = mbox_alloc_msg_cgx_set_fec_param(mbox);
if (req == NULL)
    goto exit;
```

If `mbox_alloc_msg_cgx_set_fec_param()` fails, `rc` is still set to `-ENOSPC` from initialization, which is correct. But trace the path: if `roc_nix_is_vf_or_sdp()` returns true, `rc` is set to `NIX_ERR_OP_NOTSUP` and we goto exit without ever calling `mbox_alloc_msg_cgx_set_fec_param()`. This is fine.

Actually, reviewing more carefully: the mailbox message allocation can fail, and the code handles it by checking `req == NULL` and jumping to exit where `mbox_put()` is called. This appears correct.

However, I need to verify if `mbox_process()` could fail in a way that leaves the allocated message un-freed. Looking at the pattern: after `mbox_alloc_msg_*()`, if `mbox_process()` fails, does the mbox infrastructure clean up? In DPDK mailbox patterns, `mbox_process()` sends the message and the mailbox infrastructure handles cleanup regardless of success/failure. The pattern here matches other code in the same file (e.g., `roc_nix_mac_link_state_set()`), so this appears to be the standard pattern for this driver.

**No resource leak detected.**

**2. Uninitialized `rsp` pointer usage in `roc_nix_mac_fec_supported_get()`**

```c
struct cgx_fw_data *rsp = NULL;
int rc;

if (roc_nix_is_vf_or_sdp(roc_nix)) {
    rc = NIX_ERR_OP_NOTSUP;
    goto exit;
}

mbox_alloc_msg_cgx_get_aux_link_info(mbox);
rc = mbox_process_msg(mbox, (void *)&rsp);
if (rc)
    goto exit;

*supported_fec = rsp->fwdata.supported_fec;
```

If `mbox_process_msg()` succeeds (`rc == 0`), then `rsp` is populated by the function. If it fails, we jump to exit without dereferencing `rsp`. The code appears safe assuming `mbox_process_msg()` populates `rsp` on success.

However, I should verify: what if `mbox_alloc_msg_cgx_get_aux_link_info()` returns NULL? The return value is not checked. Looking at the usage pattern in the same file in other functions like `roc_nix_mac_link_info_get()`:

```c
mbox_alloc_msg_cgx_get_link_info(mbox);
rc = mbox_process_msg(mbox, (void *)&rsp);
```

The return value of `mbox_alloc_msg_*()` is not checked there either. This appears to be the driver's pattern: if allocation fails, `mbox_process_msg()` will fail and return an error. So while the code doesn't explicitly check the alloc return, the subsequent `mbox_process_msg()` check handles it.

**No error detected** - follows driver pattern.

### Warnings

**1. Missing error check on `mbox_alloc_msg_cgx_get_aux_link_info()` return value**

While the driver pattern appears to handle this implicitly via `mbox_process_msg()`, explicitly checking the return value would be more defensive:

```c
if (mbox_alloc_msg_cgx_get_aux_link_info(mbox) == NULL) {
    rc = -ENOSPC;
    goto exit;
}
```

This would make the error handling more explicit and consistent with `roc_nix_mac_fec_set()` which does check `req == NULL`.

**2. Missing release notes update**

The commit message states "V3: Updated doc/guides/nics/cnxk.rst" but no changes to `doc/guides/rel_notes/` are included. These are new internal API functions (`__roc_api` / `RTE_EXPORT_INTERNAL_SYMBOL`) shared between DPDK components. According to the guidelines, internal API changes do not require release notes. However, since this is a new feature (FEC support) that will be exposed via the ethdev PMD in patch 2/2, the release notes should be updated when the feature is complete.

Since patch 2/2 will likely add the release notes entry, and internal ROC API doesn't require release notes on its own, this is acceptable as part of a series.

**No warning** - defer to patch 2/2 for release notes.

---

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

### Errors

None detected.

### Warnings

**1. FEC capability fallback missing**

In `cnxk_nix_fec_get_capability()`:

```c
rc = roc_nix_mac_fec_supported_get(nix, &supported_fec);
if (rc == 0 && supported_fec != 0) {
    /* ... populate capability from firmware ... */
    return 1;
}

return rc;
```

The commit message states: "If firmware provides supported FEC info, return actual capabilities for current link speed. Otherwise, fall back to a default capability table for common speeds."

However, the code does NOT implement this fallback. When `rc != 0` or `supported_fec == 0`, the function returns the error code or zero, not a fallback capability table. This contradicts the commit message and the documentation which says "If firmware provides supported FEC info... Otherwise, fall back to a default capability table".

**Either the code should implement the fallback, or the documentation/commit message should be corrected to say it returns an error when firmware info is unavailable.**

**2. Missing release notes update**

This patch adds a new feature (FEC support) with three new ethdev operations. According to the guidelines, "Changes to existing API require release notes" and "New drivers or subsystems must have release notes". This is a significant new feature for the cnxk PMD.

The patch should update `doc/guides/rel_notes/release_26_03.rst` (or whatever the current release is) to document this new feature.

**3. Missing testpmd hooks**

The guidelines state: "New API functions must have hooks in app/testpmd". The FEC ethdev operations are not new API (they're existing ethdev ops), but for a PMD to claim support for these operations, testpmd should be able to exercise them. Check if testpmd already has FEC commands - if it does, this is satisfied. If not, the PMD claiming FEC support without testpmd hooks may make it difficult to test.

However, testpmd already has FEC support commands (`show port <port_id> fec_mode`, `set port <port_id> fec_mode <mode>`), so this is satisfied.

**Not a warning** - testpmd support already exists.

**4. `RTE_BIT64()` vs `1ULL <<` in `cnxk_fec_capa_from_supported()`**

```c
if (supported_fec & (1ULL << ROC_FEC_BASER))
    capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
if (supported_fec & (1ULL << ROC_FEC_RS))
    capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
```

The code uses `1ULL <<` which is correct for 64-bit masks. However, DPDK coding style prefers `RTE_BIT64()` macro for consistency:

```c
if (supported_fec & RTE_BIT64(ROC_FEC_BASER))
    capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
if (supported_fec & RTE_BIT64(ROC_FEC_RS))
    capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
```

This is a minor style preference. `1ULL <<` is not wrong, but `RTE_BIT64()` is the DPDK preferred style.

### Info

**1. Documentation placement in cnxk.rst**

The FEC documentation is added to `doc/guides/nics/cnxk.rst` under a new section "Forward Error Correction (FEC)". This is well-structured and provides good usage examples. The placement after "Runtime Config Options" and before "Limitations" is appropriate.

**2. Features matrix update**

The patch correctly adds `FEC = Y` to `doc/guides/nics/features/cnxk.ini`, which is required per the guidelines: "PMD features must match the features matrix in doc/guides/nics/features/".

---

## Summary

### Patch 1/2: common/cnxk: add FEC configuration support

**Warnings:**
- Consider adding explicit NULL check on `mbox_alloc_msg_cgx_get_aux_link_info()` return value for consistency with `roc_nix_mac_fec_set()` pattern

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

**Warnings:**
- Missing implementation of fallback capability table described in commit message and documentation. Either implement the fallback or update documentation to state the function returns error when firmware info is unavailable.
- Missing release notes update for new FEC feature support
- Consider using `RTE_BIT64()` instead of `1ULL <<` for consistency with DPDK style preferences