|WARNING| [v6,24/24] eal: deprecate rte_atomicNN functions

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

I'll review this patch series against the AGENTS.md guidelines, focusing on correctness bugs, C coding style, API requirements, and other violations.

## Patch 01/24: bpf: use C11 atomics in BPF_ST_ATOMIC_REG

**No issues found.** The conversion correctly replaces deprecated `rte_atomicNN` operations with C11 equivalents using `seq_cst` ordering, preserving the original semantics.

---

## Patch 02/24: net/bonding: use stdatomic

**Errors:**

1. **Incorrect `rte_atomic_compare_exchange_weak_explicit()` usage in timer update loop (lines 1354-1356)**
   - The CAS weak loop reads `old_marker_timer` once before the loop, but `rte_atomic_compare_exchange_weak_explicit()` updates the expected-value pointer on failure
   - After the first failed CAS, `old_marker_timer` holds the current value from the last load, which is re-read on the next iteration
   - This is a subtle pattern error: the loop should either use `_strong` or re-load explicitly
   - **Fix**: Change to `rte_atomic_compare_exchange_strong_explicit()` since the loop body is trivial (just `timer_set()`), or add an explicit reload after failed CAS

**Suggested fix:**
```c
old_marker_timer = rte_atomic_load_explicit(&port->rx_marker_timer,
                                            rte_memory_order_acquire);
do {
    if (!timer_is_expired(&old_marker_timer)) {
        wrn = WRN_RX_MARKER_TO_FAST;
        goto free_out;
    }
    timer_set(&marker_timer, mode4->rx_marker_timeout);
} while (!rte_atomic_compare_exchange_strong_explicit(&port->rx_marker_timer,
            &old_marker_timer, marker_timer,
            rte_memory_order_release, rte_memory_order_acquire));
```

---

## Patch 03/24: net/nbl: remove unused rte_atomic16 field

**No issues found.** Removes unused field.

---

## Patch 04/24: net/ena: replace use of rte_atomicNN

**No issues found.** Statistics conversion to non-atomic is correct (as documented, stats don't require exactness under contention in DPDK PMD model).

---

## Patch 05/24: net/failsafe: convert to stdatomic

**Warnings:**

1. **`FS_ATOMIC_P` implementation (line 326)**
   - Uses `rte_atomic_exchange_explicit(..., acquire)` for a "lock acquire" operation
   - The return value of `exchange` is discarded, but if this is truly a lock, it should check whether the exchange succeeded (i.e., previous value was 0)
   - However, reviewing the usage: `FS_ATOMIC_P(a)` unconditionally sets to 1, `FS_ATOMIC_V(a)` sets to 0 with release
   - This appears to be a simple flag set, not a test-and-set lock
   - **The naming is misleading** but the code is functionally correct for the intended use (credit handshake flag)

---

## Patch 06/24: net/enic: do not use deprecated rte_atomic64

**No issues found.** Conversion to plain uint64_t for statistics is correct per DPDK PMD model.

---

## Patch 07/24: net/pfe: use ethdev linkstatus helpers

**No issues found.** Replaces open-coded atomic link status update with ethdev helpers.

---

## Patch 08/24: net/sfc: replace rte_atomic with stdatomic

**No issues found.** Simple flag conversion and use of ethdev link helpers.

---

## Patch 09/24: crypto/ccp: replace use of rte_atomic64 with stdatomic

**No issues found.** Type change and atomic operation updates are straightforward.

---

## Patch 10/24: bus/dpaa: replace rte_atomic16 with stdatomic

**No issues found.** Simple allocation flag using exchange pattern is correct.

---

## Patch 11/24: bus/fslmc: replace rte_atomic16 with stdatomic

**No issues found.** Similar allocation flag pattern, correct CAS usage.

---

## Patch 12/24: net/netvsc: replace rte_atomic32 with stdatomic

**No issues found.** RNDIS transaction ID and buffer refcount conversions are correct.

---

## Patch 13/24: event/sw: convert from rte_atomic32 to stdatomic

**No issues found.** Inflight credit tracking conversion uses appropriate memory ordering.

---

## Patch 14/24: bus/vmbus: convert from rte_atomic to stdatomic

**No issues found.** Ring buffer producer conversion is well-documented and correctly implements the two-cursor design with appropriate memory ordering.

---

## Patch 15/24: common/dpaax: use stdatomic instead of rte_atomic

**Warnings:**

1. **`atomic_inc_and_test` and `atomic_dec_and_test` semantics (lines 308-311)**
   - Linux kernel versions return true when result is zero after operation
   - The previous code matched this with `rte_atomic32_inc_and_test()` and `rte_atomic32_dec_and_test()`
   - New code: `fetch_add(...) == 0` tests if value was zero *before* increment
   - This is inverted from the kernel semantics
   - **Fix needed** to match kernel behavior:
   ```c
   #define atomic_inc_and_test(v)  (rte_atomic_fetch_add_explicit((v), 1, \
                                    rte_memory_order_seq_cst) + 1 == 0)
   #define atomic_dec_and_test(v)  (rte_atomic_fetch_sub_explicit((v), 1, \
                                    rte_memory_order_seq_cst) - 1 == 0)
   ```

---

## Patch 16/24: net/bnx2x: convert from rte_atomic32 to stdatomic

**No issues found.** Simple flag conversion with seq_cst.

---

## Patch 17/24: bus/fslmc: replace rte_atomic32 with stdatomic

**No issues found.** Atomic wrapper conversion is straightforward.

---

## Patch 18/24: drivers/event: replace rte_atomic32 in selftests

**No issues found.** Test counter conversion uses relaxed for simple drain checking, release on dequeue side.

---

## Patch 19/24: net/hinic: replace rte_atomic32 with stdatomic

**No issues found.** DMA pool tracking conversion is correct.

---

## Patch 20/24: net/txgbe: replace rte_atomic32 with stdatomic

**No issues found.** SW-FW mailbox lock conversion uses correct acquire/release pairing.

---

## Patch 21/24: net/vhost: use stdatomic instead of rte_atomic32

**No issues found.** The Dekker-style handshake is correctly implemented with seq_cst on both legs.

---

## Patch 22/24: vdpa/ifc: replace rte_atomic32 with stdatomic

**No issues found.** Simple flag conversions with seq_cst (could be relaxed, but seq_cst is safe).

---

## Patch 23/24: test/atomic: suppress deprecation warnings for legacy APIs

**No issues found.** Appropriate use of diagnostic pragmas.

---

## Patch 24/24: eal: deprecate rte_atomicNN functions

**No issues found.** Marks APIs as deprecated and removes architecture-specific implementations.

---

## Summary

**Errors (must fix):**

1. **Patch 02/24 (bonding)**: `rx_marker_timer` CAS loop uses weak exchange incorrectly - should use strong or add explicit reload
2. **Patch 15/24 (dpaax)**: `atomic_inc_and_test`/`atomic_dec_and_test` have inverted semantics compared to Linux kernel API they emulate

**Warnings:**

1. **Patch 05/24 (failsafe)**: `FS_ATOMIC_P` naming is misleading (suggests lock acquire but is just a set); not a bug but could confuse readers

All other patches are correct. The series successfully removes all in-tree usage of deprecated `rte_atomicNN` APIs.