|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

## Summary

This patch restricts bonding device control operations to the primary process in multi-process scenarios. The approach is sound: reject control operations in secondary processes via a reduced `dev_ops` table and API-level checks. However, there are several correctness and style issues that must be addressed.

---

## Errors

### 1. Missing error propagation in `bond_check_primary()`

**File:** `drivers/net/bonding/eth_bond_private.h`  
**Location:** Lines 217-224

The function `bond_check_primary()` always returns `err` when called from a non-primary process, but callers pass different error codes (`-1`, `-ENOTSUP`). The function itself never returns 0 on success -- it only checks `rte_eal_process_type()` and returns `err` or falls through to return 0. However, the variable name `err` is misleading because it's the **return value on error**, not an error code to check.

The real issue: **all callers check `if (ret != 0) return ret;`**, which is correct. But the inconsistent error codes (`-1` vs `-ENOTSUP`) across the API are problematic. The bonding API should use consistent error codes.

**Suggested fix:** Standardize on `-ENOTSUP` for all secondary-process rejections. The current mix of `-1` and `-ENOTSUP` is inconsistent.

```c
/* In rte_eth_bond_api.c - example from rte_eth_bond_create */
ret = bond_check_primary(__func__, -ENOTSUP);  /* not -1 */
if (ret != 0)
    return ret;
```

Apply this to all call sites using `-1`.

---

### 2. Query operations rejected in secondary despite documented as available

**File:** `drivers/net/bonding/rte_eth_bond_8023ad.c`  
**Location:** Lines 1666-1671 (`rte_eth_bond_8023ad_ext_collect_get`), Lines 1680-1685 (`rte_eth_bond_8023ad_ext_distrib_get`)

The documentation states "supported query and detach operations" remain available to secondary processes, but the patch rejects LACP query functions (`_get` suffixed functions) in secondary:

```c
int err;

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

These are **read-only query operations** that should be allowed in secondary processes per the stated design. Rejecting them contradicts the documentation.

**Suggested fix:** Remove the `bond_check_primary()` check from:
- `rte_eth_bond_8023ad_ext_collect_get()`
- `rte_eth_bond_8023ad_ext_distrib_get()`

These functions only read the `port->rx_machine_state` and `port->tx_machine_state`, they do not modify LACP state.

---

### 3. LACP state may be shared memory requiring atomics

**File:** `drivers/net/bonding/rte_eth_bond_8023ad.c`  
**Context:** Query functions read `port->rx_machine_state` and `port->tx_machine_state`

If the `port` structure is in shared memory (likely, since bonding state is primary-owned but secondary-readable), these reads **must** use `rte_atomic_load_explicit()` with at least `rte_memory_order_relaxed`. Plain reads of shared variables from secondary processes are data races.

The patch does not show the `port` structure definition, but if it's in shared memory (as implied by the multi-process design), this is a **race condition**.

**Verification needed:** Check if `port` lives in shared memory (`rte_malloc`/`rte_memzone`). If yes, the `rx_machine_state` and `tx_machine_state` fields must be accessed atomically in query paths.

**Suggested fix (if port is in shared memory):**

```c
/* In rte_eth_bond_8023ad_ext_collect_get */
return rte_atomic_load_explicit(&port->rx_machine_state,
    rte_memory_order_relaxed) == RX_CURRENT;

/* In rte_eth_bond_8023ad_ext_distrib_get */
return rte_atomic_load_explicit(&port->tx_machine_state,
    rte_memory_order_relaxed) == TX_TRANSMIT;
```

Only flag this if you can confirm `port` is in shared memory. The patch does not show enough context to be certain, but the design strongly implies it.

---

## Warnings

### 1. Inconsistent return value convention

**File:** `drivers/net/bonding/rte_eth_bond_api.c` (entire file)

The bonding API uses a mix of `-1` and specific error codes (`-EINVAL`, `-ENOTSUP`). The new secondary-process checks introduce another inconsistency: some call sites pass `-1`, others pass `-ENOTSUP`.

**Suggested fix:** Convert all `-1` returns in the patch to `-ENOTSUP` for secondary-process rejections. This is clearer and follows DPDK conventions.

Example:
```c
/* Change from */
ret = bond_check_primary(__func__, -1);

/* To */
ret = bond_check_primary(__func__, -ENOTSUP);
```

This affects: `rte_eth_bond_create()`, `rte_eth_bond_member_add()`, `rte_eth_bond_member_remove()`, `rte_eth_bond_mode_set()`, etc.

---

### 2. Release notes claim rejection of "configuration changes" but reject more

**File:** `doc/guides/rel_notes/release_26_11.rst`  
**Location:** Lines 58-61

The release notes say "bonding device configuration changes are rejected", but the patch also rejects LACP runtime state queries (`ext_collect_get`, `ext_distrib_get`), which are not configuration operations.

If the query rejections in Error #2 are removed (as they should be), this note is accurate. Otherwise, it's misleading.

**Suggested fix:** Either remove query rejections (preferred), or clarify the note:
```
Bonding device configuration and LACP control operations are rejected in
secondary processes. Supported query and detach paths remain available.
```

---

### 3. Secondary dev_ops missing `eth_dev_priv_dump`

**File:** `drivers/net/bonding/rte_eth_bond_pmd.c`  
**Location:** Lines 3647-3654

The `secondary_dev_ops` table omits `.eth_dev_priv_dump`, even though this is a read-only debug operation that should work in secondary processes. The comment in the commit message says "Remove private dump from secondary dev_ops" but does not justify why a read-only query is restricted.

**Suggested fix:** Add `.eth_dev_priv_dump = bond_ethdev_priv_dump` to `secondary_dev_ops`. It's harmless and useful for debugging secondary-process issues.

---

## Info

### 1. Documentation wording suggestion

**File:** `doc/guides/prog_guide/link_bonding_poll_mode_drv_lib.rst`  
**Location:** Lines 263-267

The phrase "Rx and Tx are not supported on a bonding device in a secondary process; receive returns no packets and transmit drops packets" could be clearer.

**Suggested rewording:**
```
Data-plane operations (Rx/Tx) on a bonding device are not supported in
secondary processes. Receive operations return zero packets and transmit
operations drop all packets.
```

This makes it explicit these are **data-plane** operations, distinguishing them from control-plane.

---

### 2. Consider marking LACP query functions with `__rte_experimental` removal notice

If the LACP query functions (`ext_collect_get`, `ext_distrib_get`) are rarely used, consider deprecating them in favor of a unified LACP state query API in a future release. This is a long-term API cleanup suggestion, not required for this patch.

---

## Final Checklist

- [x] Code compiles independently
- [x] Documentation updated with code
- [ ] **Error #1:** Inconsistent error codes (`-1` vs `-ENOTSUP`) -- standardize on `-ENOTSUP`
- [ ] **Error #2:** Query operations incorrectly rejected -- remove checks from `_get` functions
- [ ] **Error #3:** Verify `port` atomics if shared memory -- flag as race condition if not atomic
- [ ] **Warning #1:** Convert all `-1` to `-ENOTSUP` for clarity
- [ ] **Warning #2:** Update release notes if query rejections are removed
- [ ] **Warning #3:** Consider adding `eth_dev_priv_dump` to secondary ops

---

## Conclusion

The patch's core design is correct: restrict secondary processes to query/detach via reduced `dev_ops` and API checks. However, it over-restricts by rejecting read-only LACP queries (Error #2), uses inconsistent error codes (Error #1, Warning #1), and may have atomicity issues if `port` is in shared memory (Error #3). Fixing the query rejections and standardizing error codes will align the implementation with the documented design.