|WARNING| [4/4] test: add ethdev module EEPROM decoding tests

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-28

# Code Review: ethdev module EEPROM decoder decoupling and API exposure

## PATCH 1/4: Decouple SFF decoders from telemetry

### Errors

None.

### Warnings

None.

### Info

**Patch summary:** Introduces an output descriptor (`struct sff_output`) with a callback-based interface, allowing SFF decoders to be used outside telemetry. Telemetry becomes one consumer of the callback. This is a clean refactoring with no functional change to telemetry output.

The design follows the "compiler not framework" model: decoders take input and call a user-provided callback with structured output. No correctness or style issues found.

---

## PATCH 2/4: Check module EEPROM length before decoding

### Errors

None.

### Warnings

**1. Error message uses raw `length` instead of the required minimum**

The error log "module EEPROM is too short: %u bytes" only reports the actual length, not what length was required. This makes debugging harder.

**Suggested fix:**
```c
/* Current */
RTE_ETHDEV_LOG_LINE(ERR, "Port %u module EEPROM is too short: %u bytes",
    port_id, eeprom.length);

/* Better - report both actual and required */
if (ret == -EINVAL) {
    const char *req = (minfo.type == RTE_ETH_MODULE_SFF_8472) ?
        "at least 256 for base info, 512 for diagnostics" :
        "at least 256";
    RTE_ETHDEV_LOG_LINE(ERR,
        "Port %u module EEPROM too short: got %u bytes, need %s",
        port_id, eeprom.length, req);
}
```

Or add a `required_len` out-parameter to `sff_decode_module_eeprom()` so the caller can report the specific minimum.

### Info

The length checks are correct:
- SFF-8079: requires 256 bytes (one page)
- SFF-8472: requires 256 bytes for base, 512 for diagnostics (graceful degradation if only 256 present)
- SFF-8436/8636: requires 256 bytes

The switch on `type` returning `-ENOTSUP` for unknown types is correct.

---

## PATCH 3/4: Add API to decode module EEPROM

### Errors

None.

### Warnings

**1. Release notes claim "can decode data from any source" but don't warn about module type compatibility**

The release note and API doc correctly state that the function does not access the device and can decode EEPROM from other sources like Linux ethtool. However, they don't mention that the `type` must match the module layout. If a user passes the wrong type (e.g., `RTE_ETH_MODULE_SFF_8079` for a QSFP module), the decoding will produce nonsense. This is a documentation clarity issue.

**Suggested addition to `rte_eth_module_eeprom_parse()` docstring:**
```c
 * @param type
 *   Module type, one of RTE_ETH_MODULE_SFF_*,
 *   as reported by rte_eth_dev_get_module_info().
+ *   The type must match the actual module layout;
+ *   passing the wrong type will produce incorrect output.
```

**2. Missing validation of `type` against known values in public API**

The internal `sff_decode_module_eeprom()` returns `-ENOTSUP` for unknown types, which is good. The public wrapper `rte_eth_module_eeprom_parse()` checks for NULL `data` and `cb`, but does not pre-validate `type` before calling the internal function. This means the `-ENOTSUP` path is only documented internally, not in the public API doc.

**Suggested fix:** Update the public API docstring to explicitly list the error code:
```c
 * @return
 *   - (0) if successful.
 *   - (-EINVAL) if bad parameter or data is too short for the module type.
 *   - (-ENOTSUP) if the module type is not supported.
```

(This is already present in the patch, so no change needed -- verification confirms it's documented.)

**3. Programmer's Guide example does not check `malloc()` return before use**

The example in `doc/guides/prog_guide/ethdev/ethdev.rst`:
```c
eeprom.data = malloc(info.eeprom_len);
eeprom.length = info.eeprom_len;
if (eeprom.data != NULL &&
        rte_eth_dev_get_module_eeprom(port_id, &eeprom) == 0)
    rte_eth_module_eeprom_parse(info.type, eeprom.data, eeprom.length,
            print_field, NULL);
free(eeprom.data);
```

The code checks `eeprom.data != NULL` before calling `rte_eth_dev_get_module_eeprom()`, which is correct. However, if `malloc()` fails, the code falls through to `free(NULL)`, which is safe. This is actually fine -- **no issue here**. (Initial concern retracted.)

### Info

**Good API design:**
- The function does not access the device (as documented), making it usable without EAL.
- The callback approach (`rte_eth_module_eeprom_field_cb`) is appropriate -- each field is reported once as it is decoded, avoiding the need for large intermediate storage.
- Marking the function `__rte_experimental` is correct for a new API.
- The `RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_module_eeprom_parse, 26.11)` placement and syntax are correct.

**Documentation:**
- The release notes and Programmer's Guide additions are clear and helpful.
- The example code demonstrates the typical use case.

---

## PATCH 4/4: Add ethdev module EEPROM decoding tests

### Errors

None.

### Warnings

None.

### Info

**Test coverage is good:**
- Tests decode SFF-8079, SFF-8472 (with full and short data), and SFF-8636 EEPROM images.
- Tests verify that SFF-8472 diagnostics are skipped when only page A0h is provided (graceful degradation).
- Tests verify that invalid parameters, too-short data, and unknown module types are rejected without invoking the callback.
- The `CHECK_FIELD()` and `CHECK_NO_FIELD()` macros are clear and appropriate.

**Test structure:**
- Uses `unit_test_suite_runner` and `TEST_ASSERT` macros correctly.
- Registered with `REGISTER_FAST_TEST(ethdev_module_eeprom, NOHUGE_OK, ASAN_OK, test_module_eeprom);` -- correct.

**Test data construction:**
- `fill_sfp()` and `fill_qsfp()` build realistic EEPROM images with vendor names, OUI, diagnostics data, etc.
- The tests check both identification fields (vendor name, connector type) and diagnostic fields (temperature, optical power), ensuring the decoder handles the full layout.

No correctness or style issues.

---

## Summary

**Errors:** None

**Warnings:**
1. (Patch 2) Error message for short EEPROM could report the required length, not just the actual length.
2. (Patch 3) API documentation could explicitly warn that `type` must match the module layout.

**Overall assessment:** This is a well-designed patch series. The refactoring in patch 1 is clean, the length checks in patch 2 are correct, the new API in patch 3 follows DPDK conventions, and the tests in patch 4 are thorough. The warnings are minor documentation/UX improvements, not correctness bugs.