|WARNING| [RFC, v4, 2/2] net/bonding: restrict secondary control operations

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/2: net/bonding: skip unavailable member stats

### Errors

None.

### Warnings

None.

### Info

**Missing release notes:**
This is a bug fix (addressing incorrect accumulation of stale/uninitialized statistics on member error paths), but the patch does not update release notes.
Consider adding a brief entry in `doc/guides/rel_notes/release_26_11.rst` under "Fixed Issues" describing that bonding statistics now correctly skip failed member queries.

---

## Patch 2/2: net/bonding: restrict secondary control operations

### Errors

**1. Dev close callback must not return error in secondary process:**
```c
static const struct eth_dev_ops secondary_dev_ops = {
	.dev_close         = bond_ethdev_close,
```
`bond_ethdev_close()` performs cleanup operations that may fail in a secondary process (it calls `bond_ethdev_stop()` which can fail on `rte_eth_dev_stop()` for members, and it frees resources that may not be owned by the secondary).
The documentation states that in a secondary process, `rte_eth_dev_close()` is the detach operation, yet if `bond_ethdev_close()` returns non-zero on member errors, the ethdev layer may propagate that error instead of completing the detach.

Secondary processes should not fail to detach due to primary-owned state cleanup failures.
Consider implementing a secondary-specific close callback that always succeeds, or ensure `bond_ethdev_close()` returns 0 when called from a secondary process even if internal operations fail.

**2. Private dump skips LACP state without indicating unavailability:**
```c
if (internals->mode == BONDING_MODE_8023AD &&
		rte_eal_process_type() == RTE_PROC_PRIMARY)
	dump_lacp(dev->data->port_id, f);
```
A secondary process reading the private dump of an 802.3ad bonding device will receive output that omits LACP state without any indication that the omission occurred.
This silently provides incomplete information and could mislead an operator.

When the mode is 802.3ad and the caller is a secondary process, the dump should print a message indicating that LACP state is not available in secondary processes instead of silently skipping it.

**3. Linkstatus update path potential ABI inconsistency:**
```c
rte_eth_linkstatus_set(ethdev, &bond_link);
```
While `rte_eth_linkstatus_set()` is the correct modern API for atomic link updates, this change may alter behavior in secondary processes where the link status was previously written directly.
Verify that this does not introduce issues if the primary and secondary have different link status visibility (e.g., if the primary updates link status after the secondary reads it).

This is a correctness concern, not an error per se, but the patch does not document whether link status synchronization between primary and secondary has been considered.

### Warnings

**1. Release note placement suggestion:**
The release note is placed under "API Changes" which is appropriate, but the impact on secondary process users is significant.
Consider also documenting this in the bonding PMD release notes section or "Known Issues" if there are any limitations on secondary detach or query operations (e.g., LACP state unavailability).

**2. Documentation completeness:**
The programmer's guide update lists supported secondary operations and restrictions comprehensively, but it does not explicitly state what happens if a secondary process attempts an unsupported control operation.
The implementation returns `ENOTSUP`, but the documentation could clarify that ethdev will return an error for unsupported ops in a secondary process.

**3. Statistics get in secondary may silently skip failed members:**
Patch 1/2 introduces skipping of failed member stats in `bond_ethdev_stats_get()`.
While this is correct for handling transient member failures, in a secondary process this could mask issues where the primary has removed or changed members but the secondary has stale information.

Not an error, but operators should be aware that secondary process statistics may be incomplete if members fail to report stats (which could happen if members were removed by the primary).

### Info

**API design note:**
The `bond_check_primary()` helper cleanly gates control operations to the primary process.
The pattern is consistent and correct.

However, some of the control API functions (e.g., `rte_eth_bond_member_add()`) already return `-1` on error, while the helper suggests returning `-ENOTSUP`.
The patch returns the helper's suggested value (`-ENOTSUP` or `-1` depending on the function), which matches existing error return conventions.
No change needed, but ensure that callers expect `-1` where that is the existing return convention (some DPDK APIs use negative errno, others use `-1`).

**Testing suggestion:**
The patch does not mention whether tests were added to verify secondary process restrictions.
Consider adding a test case in `app/test/test_link_bonding.c` that spawns a secondary process and verifies that control operations correctly return `ENOTSUP`.

---

## Summary

**Patch 1/2** is correct but missing release notes.

**Patch 2/2** has three errors to address:
- Secondary dev close callback error handling
- Private dump silent omission of LACP state
- Link status update ABI consideration (verify secondary behavior)

And three warnings:
- Release note placement/completeness
- Documentation of error behavior
- Secondary stats may silently skip failed members (interaction with patch 1/2)