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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/bonding: restrict secondary control operations

## Summary

This patch restricts bonding device control operations to the primary process, installing a reduced `dev_ops` table in secondary processes and rejecting bonding control API calls from non-primary contexts. Below are the findings organized by severity.

---

## Errors

### 1. Error return type mismatch in `bond_check_primary()`

**File:** `drivers/net/bonding/eth_bond_private.h`

The `bond_check_primary()` helper returns `int` but is called with inconsistent error values (`-1`, `-ENOTSUP`). Some callers expect `-1`, others expect `-ENOTSUP`. This creates an inconsistent API contract.

**Suggested fix:**
Standardize on one error code. Since this is a "not supported" condition, `-ENOTSUP` is more semantically correct:

```c
static inline int
bond_check_primary(const char *op)
{
	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
		return 0;

	RTE_BOND_LOG(ERR, "%s not supported in non-primary process", op);
	return -ENOTSUP;
}
```

Then update all call sites to expect `-ENOTSUP`, and change API functions currently returning `-1` on error to return `-ENOTSUP` for consistency.

---

### 2. Missing experimental tag on new restriction behavior

**File:** `drivers/net/bonding/rte_eth_bond_api.c`, `rte_eth_bond_8023ad.c`

These API functions are not new, but their behavior is changing: they now fail in secondary processes where they previously may have appeared to work (even if incorrectly). This is an API behavior change that affects existing applications and should be documented with a deprecation notice or experimental tag if this is a trial restriction.

**Concern:**
Applications currently calling these functions from secondary processes will start receiving `-ENOTSUP` errors. While this is arguably a fix (the functions were never safe in secondary processes), it is a user-visible behavior change.

**Suggested action:**
Add a release note entry under "API Changes" documenting that bonding control operations now explicitly fail in secondary processes, and consider whether a deprecation period is appropriate.

---

## Warnings

### 1. Inconsistent variable naming: `ret` vs `retval`

**File:** `drivers/net/bonding/rte_eth_bond_api.c`

Some functions use `retval` for the primary check, others use `ret`. Within the same file, this inconsistency reduces readability.

**Suggested fix:**
Standardize on `ret` throughout (it's shorter and matches DPDK conventions):

```c
int ret;

ret = bond_check_primary(__func__, -ENOTSUP);
if (ret != 0)
	return ret;
```

---

### 2. Secondary `dev_ops` missing `eth_dev_priv_dump`

**File:** `drivers/net/bonding/rte_eth_bond_pmd.c`

The `secondary_dev_ops` table omits `eth_dev_priv_dump` (which is a read-only query operation). The release notes claim "supported query and detach paths remain available," but this query path is removed.

**Suggested fix:**
If `eth_dev_priv_dump` is safe to call from secondary processes (it appears to be read-only), add it to the secondary ops table:

```c
static const struct eth_dev_ops secondary_dev_ops = {
	.dev_close         = bond_ethdev_close,
	.dev_infos_get     = bond_ethdev_info,
	.link_update       = bond_ethdev_link_update,
	.stats_get         = bond_ethdev_stats_get,
	.reta_query        = bond_ethdev_rss_reta_query,
	.rss_hash_conf_get = bond_ethdev_rss_hash_conf_get,
	.eth_dev_priv_dump = bond_ethdev_priv_dump,
};
```

If it is unsafe (e.g., because it accesses mutable state without locking), document why it is excluded.

---

### 3. Documentation claims "Rx and Tx are not supported" but no enforcement

**File:** `doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst`

The documentation states:
> Rx and Tx are not supported on a bonding device in a secondary process; receive returns no packets and transmit drops packets.

However, the secondary `dev_ops` table does not install NULL Rx/Tx burst functions or explicitly enforce this. The default behavior will be whatever was inherited from the primary process, which may allow packets through or may crash.

**Suggested fix:**
Either:
1. Install explicit no-op Rx/Tx burst functions in secondary processes:
   ```c
   eth_dev->rx_pkt_burst = bond_secondary_rx_burst;  /* returns 0 */
   eth_dev->tx_pkt_burst = bond_secondary_tx_burst;  /* returns nb_pkts (drops) */
   ```
   and implement these as safe no-ops, OR

2. Clarify the documentation to match the actual behavior (e.g., "results are undefined; do not attempt Rx/Tx in secondary processes").

---

### 4. Release notes wording unclear

**File:** `doc/guides/rel_notes/release_26_11.rst`

The entry reads:
> Supported query and detach paths remain available to secondary processes, while bonding device configuration changes are rejected.

This is vague. What are "query and detach paths"? Which specific operations remain available?

**Suggested fix:**
Be explicit:

```rst
* **Restricted bonding device control to the primary process.**

  Bonding device configuration (mode, members, MAC, MTU, queues, start/stop)
  and LACP state modification are now restricted to the primary process.
  Secondary processes may query device info, stats, link state, RSS hash config,
  and close/detach the device, but cannot modify configuration.
```

---

### 5. Missing release notes for LACP secondary restrictions

The release notes mention bonding device control restrictions but do not specifically call out that LACP runtime state queries are also now rejected in secondary processes. Since LACP is a distinct feature, this should be mentioned.

**Suggested addition:**
```rst
  LACP runtime state queries and modifications are also restricted to the
  primary process.
```

---

## Info

### 1. Repeated pattern: consider a macro for check-and-return

The pattern:
```c
int ret;

ret = bond_check_primary(__func__, -ENOTSUP);
if (ret != 0)
	return ret;
```

appears 20+ times. Consider a macro:

```c
#define BOND_CHECK_PRIMARY() do { \
	int _ret = bond_check_primary(__func__); \
	if (_ret != 0) \
		return _ret; \
} while (0)
```

Then:
```c
BOND_CHECK_PRIMARY();
```

This reduces boilerplate and makes the intent clearer. (But this is optional; the current approach is also acceptable.)

---

### 2. Documentation: "member" terminology

The patch uses "member" (modern inclusive term) consistently. Good. No issue.

---

## Positive Observations

- The helper `bond_check_primary()` is a clean abstraction.
- Documentation is updated alongside code (atomicity requirement satisfied).
- Release notes are present.
- Secondary ops table is correctly const.

---

## Final Recommendation

**Overall assessment:** The patch is on the right track but has one critical issue (error code inconsistency) and several warnings that should be addressed before merging.

**Next steps:**
1. Fix error return code inconsistency (Error #1).
2. Add API behavior change note to release notes (Error #2).
3. Standardize variable naming (Warning #1).
4. Decide on `eth_dev_priv_dump` inclusion (Warning #2).
5. Enforce or clarify Rx/Tx behavior in secondary (Warning #3).
6. Improve release notes clarity (Warning #4, #5).