|WARNING| [8/8] doc: add bonding features matrix

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review - Bonding PMD Fixes

I have reviewed the 8-patch series. Below are my findings organized by severity.

---

## Patch 1/8: net/bonding: fix TLB member ordering with unusable member

### Errors

**Use of bare variable access on potentially shared state** (line 36):
The new `tlb_unusable[]` array is global static state that may be accessed concurrently by the link status callback and the TLB rebalance callback.
Since the callback runs every millisecond and link events are asynchronous, this access is not synchronized.
Use `rte_atomic_load_explicit()` and `rte_atomic_store_explicit()` with appropriate memory ordering (likely `relaxed` for simple flag checks).

```c
/* Current code: */
static bool tlb_unusable[RTE_MAX_ETHPORTS];
if (!tlb_unusable[member_id]) {
    tlb_unusable[member_id] = true;
    ...
}

/* Should be: */
static _Atomic bool tlb_unusable[RTE_MAX_ETHPORTS];
if (!rte_atomic_load_explicit(&tlb_unusable[member_id], rte_memory_order_relaxed)) {
    rte_atomic_store_explicit(&tlb_unusable[member_id], true, rte_memory_order_relaxed);
    ...
}
```

The `tlb_last_obytets[]` array already exists and is similarly accessed unsafely, but addressing that is outside the scope of this patch.

### Warnings

**Release notes not updated** (Patch 1):
This is a bug fix that changes behavior (members can now be skipped in TLB ordering).
Add a note to `doc/guides/rel_notes/release_26_11.rst` explaining the fix.

---

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

### No Issues Found

The patch correctly checks the return value of `rte_eth_stats_get()` and skips the accumulation if it fails.
The `continue` avoids accumulating stale data from the uninitialized local `member_stats` variable.

---

## Patch 3/8: net/bonding: skip unavailable members in device info

### Warnings

**Release notes not updated**:
This changes the behavior of `rte_eth_dev_info_get()` to skip failing members rather than aborting the whole query.
Document in `doc/guides/rel_notes/release_26_11.rst`.

---

## Patch 4/8: net/bonding: use atomic link status accessors

### No Issues Found

The patch correctly replaces direct accesses to `dev->data->dev_link` with `rte_eth_linkstatus_get()` and `rte_eth_linkstatus_set()`, which are the documented way to access the link status atomically.

---

## Patch 5/8: net/bonding: restrict control operations in secondary process

### Errors

**Secondary process check missing in some API functions**:
The patch adds `rte_eal_process_type() != RTE_PROC_PRIMARY` checks to many API functions, but there are more functions in `rte_eth_bond_api.c` and `rte_eth_bond_8023ad.c` that may also manipulate primary-process-only state and are not guarded:

- `rte_eth_bond_mode_set()` / `rte_eth_bond_mode_get()` (mode_get is probably safe, but mode_set modifies state)
- `rte_eth_bond_8023ad_conf_get()` (probably safe)
- Others that modify internal state

Review all API entry points and ensure those that write to non-shared memory or start/stop callbacks are restricted.

### Warnings

**Release notes not updated**:
This changes the API contract: secondary processes can no longer call most control operations.
Document in `doc/guides/rel_notes/release_26_11.rst`.

---

## Patch 6/8: net/bonding: add extended statistics

### Warnings

**xstats_reset missing from secondary_dev_ops**:
The patch adds `xstats_get` and `xstats_get_names` to `secondary_dev_ops` but omits `xstats_reset`.
If secondary processes can read xstats, they should also be able to reset them (or explicitly NOT be able to).
Add `xstats_reset` or document why it is excluded.

**Release notes already updated correctly** (no issue):
The patch correctly updates `doc/guides/rel_notes/release_26_11.rst` with a description of the new feature.

---

## Patch 7/8: test/bonding: add extended statistics test

### No Issues Found

The test correctly:
- Uses `TEST_ASSERT_*` macros for all checks
- Queries xstats names and values with NULL to get counts
- Validates that undersized queries return the required count
- Verifies name generation matches the expected pattern
- Confirms per-member attribution of a burst received on one member
- Resets and confirms counters are cleared
- Frees all mbufs before returning

The test is well-structured and covers the xstats contract thoroughly.

---

## Patch 8/8: doc: add bonding features matrix

### Info

**bonding.ini shows only self-implemented features**:
The patch correctly documents only the features the bonding PMD implements itself, not the offloads it inherits by intersecting member capabilities.
This is appropriate for the features matrix, which describes the driver's native capabilities.

---

## Final Check Summary

### Must Fix (Errors)
1. **Patch 1**: `tlb_unusable[]` array needs atomic access (shared between TLB callback and link event callback)
2. **Patch 5**: Review all API functions for secondary process restrictions; some control functions are still missing checks

### Should Fix (Warnings)
1. **Patch 1**: Add release notes entry for TLB member skip behavior
2. **Patch 3**: Add release notes entry for device info skipping unavailable members
3. **Patch 5**: Add release notes entry for secondary process API restrictions
4. **Patch 6**: Add `xstats_reset` to `secondary_dev_ops` or document exclusion

### Consider (Info)
- Patch 8: bonding.ini appropriately documents self-implemented features only

---

## Positive Observations

- Patch 2, 4, and 7 are correct as written
- Patch 6 correctly updates release notes for the new xstats feature
- Patch 7 provides thorough xstats test coverage using the correct test infrastructure
- The series as a whole improves robustness by handling error cases that were previously ignored