|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

## Patch Series: net/bonding: TLB member ordering, stats handling, and extended statistics

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

#### Errors

None.

#### Warnings

1. **Missing error check on `rte_eth_stats_get()`**
   ```c
   ret = rte_eth_link_get_nowait(member_id, &link);
   if (ret == 0)
       ret = rte_eth_stats_get(member_id, &stats);
   ```
   If `rte_eth_link_get_nowait()` succeeds but `rte_eth_stats_get()` fails, `ret` will be negative but the code checks `ret == 0` first. The stats query failure is caught in the outer `if (ret < 0)` block, but the logic is convoluted. Consider separate error checks for clarity.

2. **Potential stale data in `stats` structure**
   If `rte_eth_link_get_nowait()` fails but `rte_eth_stats_get()` is not called, the local `stats` variable is uninitialized when accessed in `bandwidth_left()`. However, the patch does skip the member when `ret < 0`, so this is safe. No change needed, but the initialization order could be clearer.

#### Info

- The padding logic for `tlb_members_order` when `member_count < active_count` correctly prevents stale port IDs from being used by transmit.
- The log-on-state-change pattern is appropriate for a 1ms callback.

---

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

#### Errors

None.

#### Warnings

None.

#### Info

- The `continue` correctly skips members that fail stats query rather than accumulating uninitialized counters.

---

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

#### Errors

None.

#### Warnings

1. **Log level inconsistency**
   ```c
   RTE_BOND_LOG(WARNING, "Skipping device (port %u) info: %s", ...);
   ```
   vs
   ```c
   RTE_BOND_LOG(ERR, "No member device info available");
   ```
   If skipping a single member is a `WARNING`, zero queried members is an `ERR`. This is consistent and appropriate.

---

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

#### Errors

None.

#### Warnings

1. **Redundant `rte_eth_linkstatus_get()` calls**
   In `bond_ethdev_link_update()`, the code reads the bond link at the start via `rte_eth_linkstatus_get(ethdev, &link)`, then modifies the local copy, but the `goto done` path at the end always calls `rte_eth_linkstatus_set()` even when no change occurred. This is not a correctness bug but adds unnecessary atomic writes.

2. **Race window in `bond_ethdev_lsc_event_callback()`**
   ```c
   rte_eth_linkstatus_get(bonding_eth_dev, &bond_link);
   if (bond_link.link_status) {
       // schedule delayed callback
   }
   ```
   Between the `get` and the check, another thread could modify the link status. If the application relies on precise event ordering, this could cause a stale-link decision. However, this is a pre-existing race made visible by the refactor, not introduced by this patch.

#### Info

- The new `bond_ethdev_link_down()` helper is clean and avoids code duplication.
- Using `rte_eth_linkstatus_get/set()` instead of direct `dev->data->dev_link` access is the correct pattern.

---

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

#### Errors

None.

#### Warnings

None.

#### Info

- Correctly restricts control-plane operations to the primary process.
- The `secondary_dev_ops` struct exposes only read-only operations.

---

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

#### Errors

None.

#### Warnings

1. **Missing `xstats_reset` in `secondary_dev_ops`**
   ```c
   static const struct eth_dev_ops secondary_dev_ops = {
       ...
       .xstats_get           = bond_ethdev_xstats_get,
       .xstats_get_names     = bond_ethdev_xstats_get_names,
       ...
   };
   ```
   Secondary processes can query xstats but cannot reset them. This is consistent with the read-only philosophy of the secondary process, but the release notes do not mention this restriction. Consider documenting it.

2. **`rte_eth_stats_get()` failure silently zeroed**
   ```c
   if (rte_eth_stats_get(member_id, &member_stats) < 0)
       memset(&member_stats, 0, sizeof(member_stats));
   ```
   This hides member unavailability from xstats consumers. An application cannot distinguish a member with no traffic from a member that failed to report stats. Consider adding a dedicated "query_failed" xstat per member, or logging the failure (though the latter would spam if polled frequently).

#### Info

- The per-member xstats naming (`rx_memberN_*`, `tx_memberN_*`) is consistent with conventions in other PMDs.
- Dropping `RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS` is correct since the bonding device never had per-queue stats.

---

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

#### Errors

None.

#### Warnings

None.

#### Info

- Test coverage is thorough: checks count agreement, undersized query rejection, name generation, per-member attribution, and reset.

---

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

#### Errors

None.

#### Warnings

None.

#### Info

- The `bonding.ini` file correctly reflects the features the bonding PMD implements directly.
- Omitting member-inherited offloads from the feature matrix is the right choice.

---

## Summary

### Errors: 0

### Warnings: 5
1. PATCH 1: Convoluted error-check flow in `bond_ethdev_update_tlb_member_cb()` could be clearer.
2. PATCH 4: Redundant `rte_eth_linkstatus_set()` on no-change paths.
3. PATCH 4: Pre-existing race window in `bond_ethdev_lsc_event_callback()` made visible by refactor.
4. PATCH 6: Missing `xstats_reset` in secondary process ops (intentional but undocumented).
5. PATCH 6: `rte_eth_stats_get()` failure silently zeroed in xstats, hiding member unavailability.

### Info: Multiple observations on clean, correct patterns.

---

## Overall Assessment

This is a high-quality patch series addressing real correctness bugs (TLB ordering with unusable members, stats accumulation from failed queries, non-atomic link status access) and adding useful observability (per-member xstats). The code is well-structured, and the test coverage is strong. The warnings are minor and mostly concern areas where the patch improves the status quo but does not fully close all corner cases (e.g., the LSC callback race, which predates this series).

**Recommendation:** Accept with consideration of the warnings for follow-up work.