|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