|WARNING| [v3, 3/3] common/sfc_efx/base: switch netport stats to use EVB port ID

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-14

# DPDK Patch Review

## Patch 1/3: common/sfc_efx/base: update MCDI headers

**No issues found.**

This patch imports MCDI header updates from the vendor repository. It adds new statistics IDs and updates privilege categories. As a selective vendor header import in `drivers/common/sfc_efx/base/`, the different naming conventions and style are acceptable per the exception in AGENTS.md for driver base directories.

---

## Patch 2/3: common/sfc_efx/base: add support for VADAPTER statistics IDs

**No issues found.**

This patch adds VADAPTER statistics mappings. The code follows the existing pattern for MAC and PHY statistics and is consistent with the codebase style.

---

## Patch 3/3: common/sfc_efx/base: switch netport stats to use EVB port ID

### Errors

**Function parameter order change without compatibility consideration**

The patch changes the parameter order of `efx_np_mac_stats()`:

```c
/* Before */
efx_np_mac_stats(enp, nph, action, esmp, period_ms)

/* After */
efx_np_mac_stats(enp, nph, vport_id, esmp, action, period_ms)
```

This is an internal API within the driver (in `efx_impl.h`, not an exported header), but all call sites must be updated atomically. The patch updates all four call sites in `efx_mcdi.c` and `medford4_mac.c`, so compilation will succeed. However, verify that no other out-of-tree code relies on this internal function.

### Warnings

**Magic constant `EVB_PORT_ID_NULL` not defined in patch context**

The code uses `EVB_PORT_ID_NULL` in the conditional:

```c
MCDI_IN_SET_DWORD(req, GET_NETPORT_STATISTICS_V2_IN_PORT_ID,
    (disable ? EVB_PORT_ID_NULL : vport_id));
```

This constant is not defined in the patch. Verify that it exists in the codebase (likely in the MCDI headers) and has the correct value for the firmware API.

**Comment style inconsistency**

The new comment uses C++ style:

```c
// as this may fail (and leave periodic DMA enabled) if the
```

DPDK coding standard requires C-style multi-line comments. Should be:

```c
/*
 * NOTE: Do not use EVB_PORT_ID_ASSIGNED when disabling periodic stats,
 *       as this may fail (and leave periodic DMA enabled) if the
 *       vadapter has already been deleted.
 */
```

**Inconsistent spacing in function call**

In `medford4_mac.c`, the updated calls have inconsistent indentation:

```c
/* Inconsistent - some have space before esmp, some don't */
rc = efx_np_mac_stats(enp, epp->ep_np_handle, enp->en_vport_id,
    esmp, EFX_STATS_UPLOAD, 0);

rc = efx_np_mac_stats(enp, epp->ep_np_handle,
    enp->en_vport_id, NULL, EFX_STATS_DISABLE, 0);
```

Prefer consistent alignment - either all on one line if short enough, or consistently break at the same depth.

---

## Series-Level Comments

**Release notes required**

This series adds new VADAPTER statistics support and changes the netport statistics API to use EVB port IDs. These are significant functional changes that affect SR-IOV VF statistics. The current release notes file should be updated to document:

1. Addition of VADAPTER statistics for VFs
2. Requirement for GET_NETPORT_STATISTICS_V2 firmware support

**Testing coverage**

The patch series does not include tests. For statistics changes of this nature, verify:

1. That statistics can be successfully retrieved for both PF and VF
2. That periodic statistics collection works correctly
3. That disabling statistics leaves the system in a clean state

---

## Summary

- **Errors**: 0 (parameter reordering is internal API, acceptable if all callers updated)
- **Warnings**: 3 (undefined constant reference, comment style, formatting inconsistency)
- **Info**: Release notes and testing coverage should be addressed

The series is functionally correct but has minor style issues and should include release notes for the new VADAPTER statistics feature.