|WARNING| [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/167530

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None.

### Warnings

None.

### Info

- This patch updates vendor-supplied MCDI headers from the upstream libefx repository
- The changes are primarily new enum values, struct definitions, and documentation updates for statistics and configuration
- The code is in a driver base directory (`drivers/*/base/`) which is exempt from standard DPDK naming conventions per the guidelines
- No functional code changes, only header definitions

---

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

### Errors

None.

### Warnings

None.

### Info

- Adds new VADAPTER statistics mappings to the `efx_np_mac_stat_map` array
- Follows the existing pattern for MAC and PHY statistics
- All entries use the same macro pattern (`EFX_NP_STAT_VADAPTER`) consistent with the file's style
- The `#undef EFX_NP_STAT_VADAPTER` cleanup at the end follows the existing pattern

---

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

### Errors

None.

### Warnings

**Argument order change (`efx_np_mac_stats`):**

The patch changes the parameter order of `efx_np_mac_stats()`, moving `vport_id` before `esmp` and `action` after `esmp`. This reorders parameters in an internal function.

While the commit message justifies this as "for clarity ... to match the legacy efx_mcdi_mac_stats", this type of signature change in existing code can be error-prone. All call sites have been updated in this patch, but verify:

1. No out-of-tree code or vendor code calls this function
2. The new order is consistent with related functions

This is flagged as a Warning because it's an internal API (not exported to applications) and all in-tree call sites are updated atomically.

---

**Comment accuracy (EVB_PORT_ID_ASSIGNED vs EVB_PORT_ID_NULL):**

In `efx_np.c` at line 1669:

```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.
 */
MCDI_IN_SET_DWORD(req, GET_NETPORT_STATISTICS_V2_IN_PORT_ID,
	(disable ? EVB_PORT_ID_NULL : vport_id));
```

The comment says "do not use `EVB_PORT_ID_ASSIGNED`" but the code uses `EVB_PORT_ID_NULL` when disabling. The comment appears to be explaining why the code does what it does, which is correct. However, the phrasing could be clearer--it reads as a prohibition but is actually documenting the implemented behavior.

Consider rewording to: "Use `EVB_PORT_ID_NULL` when disabling periodic stats, as `EVB_PORT_ID_ASSIGNED` may fail (and leave periodic DMA enabled) if the vadapter has already been deleted."

This is a minor documentation clarity issue, not a code error.

### Info

- Upgrades `MC_CMD_GET_NETPORT_STATISTICS` to `_V2` variant to support VADAPTER stats
- The new `_V2` request includes a `PORT_ID` field for EVB port mapping
- All call sites updated: `efx_mcdi_c` (one call) and `medford4_mac.c` (four calls)
- The parameter reordering (`vport_id` before `esmp`, `action` after `esmp`) matches the legacy `efx_mcdi_mac_stats` signature for consistency

---

## Summary

This patch series adds support for VADAPTER statistics in the sfc_efx driver by updating MCDI headers, adding statistic ID mappings, and switching to the V2 statistics command that supports EVB port IDs.

**Overall Assessment:**
- No correctness bugs detected
- Code follows existing driver patterns and is in the `drivers/*/base/` exemption for naming
- One minor documentation clarity suggestion (Warning level)
- Parameter reorder is internal-only and all call sites updated atomically

The patches are in good shape. The only actionable item is the optional comment clarification noted above.