|WARNING| [1/1] common/sfc_efx/base: clear VADAPTER stats upon allocation

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Mon Aug 24 13:48:51 CEST 2026


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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch adds a call to clear MAC statistics after VADAPTER allocation in the EF10 NIC initialization path. The patch is well-structured and includes proper error handling, but has one **correctness error** related to the error path cleanup order.

---

## Errors

### Resource Leak on Error Path (Error)

**File:** `drivers/common/sfc_efx/base/ef10_nic.c`
**Lines:** New `fail6` label

The new error path `fail6` frees the VADAPTER but does not clean up resources allocated before the new stats clear call. Specifically, if `alloc_vadaptor` is `B_FALSE` (meaning the VADAPTER was allocated by the caller and exists before entering this function), the error path at `fail6` will attempt to free it when it should not. More importantly, if the stats clear fails, the code jumps to `fail6` which only handles VADAPTER cleanup, but does not execute the cleanup at `fail5`/`fail4`/`fail3` for resources allocated earlier in the function.

The error path is backwards: `fail6` should fall through to `fail5` to ensure all previously allocated resources are cleaned up, not duplicate the VADAPTER-only logic.

**Fix:**
```c
#if EFSYS_OPT_MAC_STATS
fail6:
	EFSYS_PROBE(fail6);
	if (alloc_vadaptor != B_FALSE) {
		(void) efx_mcdi_vadaptor_free(enp, enp->en_vport_id);
		enp->en_vport_id = EVB_PORT_ID_NULL;
	}
	/* FALLTHROUGH */
#endif
fail5:
	EFSYS_PROBE(fail5);
	/* ... existing cleanup ... */
```

Add an explicit `/* FALLTHROUGH */` comment after the `fail6` cleanup block so it chains into `fail5`. This ensures that if stats clear fails, both the VADAPTER cleanup (fail6) and any prior resource cleanup (fail5/fail4/fail3) are executed.

---

## Warnings

None.

---

## Info

None.

---

## Positive Observations

- Proper use of `#if EFSYS_OPT_MAC_STATS` to guard both the stats clear call and the corresponding error path
- Error code correctly propagated from `efx_mcdi_mac_stats_clear()`
- VADAPTER freed on error path with `enp->en_vport_id` reset to `EVB_PORT_ID_NULL`
- Commit message clearly explains the rationale and scope of the change
- Conditional check `alloc_vadaptor != B_FALSE` correctly guards VADAPTER cleanup to avoid double-free


More information about the test-report mailing list