|WARNING| net/intel: add MMG device support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-01

# DPDK Patch Review

## Summary
This patch replaces direct device_id comparisons with `idpf_is_vf_device()` helper calls and adds VF device handling for operations unsupported on VF. Overall the changes are reasonable, but there are several correctness and style issues that need to be addressed.

---

## Errors

### 1. Missing return value check after `idpf_vc_stats_query()`
**File:** `drivers/net/intel/cpfl/cpfl_ethdev.c:382`

The function calls `idpf_vc_stats_query()` which can fail, checks the return value, but then continues to use `pstats` which may be NULL or invalid if the call failed. The early return for VF is correct, but after the VF check, if `idpf_vc_stats_query()` returns non-zero, the code should return immediately before attempting to use `pstats`.

**Current code:**
```c
ret = idpf_vc_stats_query(vport, &pstats);
if (ret != 0)
    return ret;

cpfl_vport_stats_update(&cpfl_vport->eth_stats_offset, pstats);
```

**Issue:** If `idpf_vc_stats_query()` fails, `pstats` is likely NULL or points to invalid data. The code correctly returns the error, but the subsequent line `cpfl_vport_stats_update()` would be unreachable. This appears correct as-is, but verify that no other code path reaches `cpfl_vport_stats_update()` with a bad `pstats`.

Actually, re-reading: the code is correct. The `if (ret != 0) return ret;` ensures that `cpfl_vport_stats_update()` is only called when `ret == 0`. No issue here. **Disregard this item.**

---

## Warnings

### 1. `RTE_EXPORT_INTERNAL_SYMBOL` should be alone on a line preceding the function
**File:** `drivers/net/intel/idpf/idpf_common_device.c:454`

The `RTE_EXPORT_INTERNAL_SYMBOL` macro is placed on a line by itself, but it should immediately precede the return type, not be followed by a blank line.

**Current:**
```c
RTE_EXPORT_INTERNAL_SYMBOL(idpf_is_vf_device)
bool idpf_is_vf_device(struct idpf_hw *hw)
```

This is correct per the AGENTS.md guideline that export macros "must appear alone on the line" (referring to `__rte_experimental` and `__rte_internal`). However, `RTE_EXPORT_INTERNAL_SYMBOL` is a different macro (used in `.c` files for linker symbol export), not the function attribute macro. The guideline for `RTE_EXPORT_*` macros states they should be in `.c` files and precede the function definition, which this patch does correctly.

**Correction:** No issue. The macro is placed correctly for a `.c` file symbol export. The `__rte_internal` attribute macro in the header is what must be alone on the line preceding the return type, which is correctly done in the header change.

### 2. Missing release notes update
**File:** (release notes not included in patch)

This patch adds support for a new device type (MMG) and changes behavior for VF devices (skipping unsupported operations). These are significant functional changes that should be documented in the current release notes under `doc/guides/rel_notes/release_26_07.rst` (assuming this is the 26.07 release based on the patch date).

**Suggested addition:**
```rst
* **net/cpfl: Added MMG device support.**

  Updated CPFL PMD to support MMG devices and properly handle VF-specific
  limitations for statistics queries and RSS hash configuration.
```

### 3. Inconsistent VF device check pattern
**File:** `drivers/net/intel/cpfl/cpfl_ethdev.c` (multiple locations)

The patch converts most `device_id == CPFL_DEV_ID_MEV` checks to `!idpf_is_vf_device()`, and `device_id == IXD_DEV_ID_VCPF` checks to `idpf_is_vf_device()`. This is correct, but the negation pattern (`!idpf_is_vf_device()`) for PF checks is less readable than a hypothetical `idpf_is_pf_device()` helper.

**Suggestion:** Consider adding a companion `idpf_is_pf_device()` helper for symmetry:
```c
static inline bool idpf_is_pf_device(struct idpf_hw *hw)
{
    return !idpf_is_vf_device(hw);
}
```

This would make the code more self-documenting: `if (idpf_is_pf_device(hw))` reads better than `if (!idpf_is_vf_device(hw))`.

However, this is not a requirement -- the current pattern is acceptable.

### 4. VF RSS hash check placement
**File:** `drivers/net/intel/idpf/idpf_common_device.c:627`

The comment states "VF devices do not support setting RSS hash", but the check uses `!idpf_is_vf_device(hw)`, which means RSS hash is **skipped for PF and set for VF**. This is inverted logic.

**Current code:**
```c
/* VF devices do not support setting RSS hash */
if (!idpf_is_vf_device(hw)) {
    ret = idpf_vc_rss_hash_set(vport);
    ...
}
```

**Issue:** The condition should be `if (!idpf_is_vf_device(hw))` to **skip** for VF (run for PF), which is what the code does. But the comment says "VF devices do not support", implying we should skip for VF, which the code correctly does. 

Wait, re-reading: the code is correct. `!idpf_is_vf_device(hw)` means "if NOT a VF device" (i.e., if PF), then set RSS hash. So RSS hash is skipped for VF. The comment and code are consistent.

**Correction:** No issue.

---

## Info

### 1. Consider adding MMG device ID definition
The commit message mentions "MMG device" but the patch does not add or reference a `CPFL_DEV_ID_MMG` or similar constant. The helper `idpf_is_vf_device()` only checks for `IDPF_DEV_ID_SRIOV` and `IXD_DEV_ID_VCPF`. 

If MMG has its own device ID, it should be added to the `idpf_is_vf_device()` check. If MMG reuses one of these existing IDs, that should be clarified in the commit message or a comment.

**Suggestion:** Verify whether MMG requires an additional device ID check in `idpf_is_vf_device()`, or document that MMG uses one of the existing IDs.

---

## Summary of Required Changes

**Must fix (Errors):**
- None identified after verification.

**Should fix (Warnings):**
- Add release notes entry documenting MMG support and VF behavior changes.

**Consider (Info):**
- Clarify MMG device ID handling in commit message or code comments.
- Consider adding `idpf_is_pf_device()` helper for readability (optional).