|WARNING| [RFC, v5, 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/168632

_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

None.

---

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

### Errors

**1. Resource leak on error path in `bond_ethdev_link_update()`**

When `link_update()` fails for the primary member in `BONDING_MODE_ACTIVE_BACKUP`,
the code does `goto out` which calls `rte_eth_linkstatus_set()` but never returns the error.
The function signature returns `int`, and the caller expects error propagation.
After the `goto out` label, the function unconditionally returns `0` even when an error occurred.

```c
case BONDING_MODE_ACTIVE_BACKUP:
	ret = link_update(bond_ctx->current_primary_port, &member_link);
	if (ret < 0) {
		RTE_BOND_LOG(ERR, "Member (port %u) link get failed: %s",
			bond_ctx->current_primary_port,
			rte_strerror(-ret));
		goto out;    /* ERROR: loss of error code */
	}

/* ... */

out:
	rte_eth_linkstatus_set(ethdev, &bond_link);
	return 0;    /* ERROR: always returns success */
```

**Suggested fix:**

```c
out:
	rte_eth_linkstatus_set(ethdev, &bond_link);
	return ret;    /* Propagate error from link_update */
```

Or initialize `ret = 0` at the top of the function and set it before each error `goto`.

**2. Missing release notes update for API restriction**

The release notes document new secondary-process query support,
but the patch also **restricts** existing API functions to primary-only
(all the `rte_eth_bond_*` control APIs now return `-1`/`-ENOTSUP` from secondary).
This is a behavior change that applications relying on these APIs in secondary processes will encounter.
The release notes should explicitly state that control APIs are now primary-only.

**Suggested addition to release notes:**

```rst
Bonding control APIs (``rte_eth_bond_member_add()``,
``rte_eth_bond_mode_set()``, ``rte_eth_bond_8023ad_*()`` configuration functions, etc.)
now return ``-ENOTSUP`` when called from a secondary process.
Query operations remain available in both primary and secondary.
```

### Warnings

**1. `bond_ethdev_link_update()` performs link queries in secondary process**

The function iterates over active members and calls `link_update()` (either `rte_eth_link_get` or `rte_eth_link_get_nowait`)
on each member port from the secondary process.
The documentation states "Rx and Tx are not supported on a bonding device in a secondary process,"
but does not explicitly state whether querying member link status from secondary is supported.

If member devices do not support secondary-process link queries, this will fail.
The code logs errors but does not clearly document this limitation.
Consider adding a note in the programmer's guide section about secondary-process limitations
stating whether member link queries are expected to work or not.

**2. `secondary_dev_ops` omits `stats_reset` but includes `stats_get`**

The secondary ops table includes `stats_get` (read-only) but omits `stats_reset` (control operation).
This is correct per the design, but the inconsistency may confuse users who see stats available but cannot reset them.
Consider documenting in the release notes or programmer's guide that `rte_eth_stats_reset()` returns `-ENOTSUP` in secondary.

**3. Documentation does not explain `rte_eth_dev_stop()` returning `-ENOTSUP`**

The programmer's guide states:
> In a secondary process, ``rte_eth_dev_stop()`` returns ``-ENOTSUP``

However, `secondary_dev_ops` does not include a `dev_stop` callback, so `rte_eth_dev_stop()` will call the default ethdev stub which returns `-ENOTSUP`.
This is correct behavior, but the rationale (secondary should not stop a device it does not own) could be stated more explicitly.

### Info

**1. Consider logging a message in `bond_ethdev_link_update()` when called from secondary**

The function now executes link queries in both primary and secondary processes.
If this is intentional and supported, no change needed.
If member link queries may not be reliable from secondary, consider adding a debug log on first call from secondary
to document that the bonding device link state reflects member queries performed in the secondary context.

**2. `bond_check_primary()` logs an error for every rejected call**

Each control API call from secondary will produce an `RTE_BOND_LOG(ERR, ...)` message.
This is appropriate for debugging, but in a secondary process that mistakenly calls these APIs in a loop,
log spam could occur.
Consider whether `ERR` is the right level, or if `WARNING` or `DEBUG` might be more appropriate for expected denials.

---

## Summary

**Patch 1/2** is correct.

**Patch 2/2** has two errors:
1. Error propagation lost in `bond_ethdev_link_update()` after link query failure
2. Missing documentation of API restriction (control APIs now primary-only) in release notes