|WARNING| [v2,4/4] ethdev: fix promoted flow metadata symbols

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-24

# DPDK Patch Review

## Patch 1/4: eal: fix macro for versioned experimental symbol

### Errors

None.

### Warnings

None.

### Info

- The patch adds a missing semicolon after `__asm__` block in `RTE_VERSION_EXPERIMENTAL_SYMBOL` macro, fixing a compilation error with clang
- The fix is straightforward and correct

---

## Patch 2/4: build: support function versioning for drivers

### Errors

None.

### Warnings

None.

### Info

- Adds `use_function_versioning` support to drivers (mirroring the library implementation)
- The implementation correctly checks for Windows compatibility and sets the `-DRTE_USE_FUNCTION_VERSIONING` flag
- Code follows the existing pattern used in `lib/meson.build`

---

## Patch 3/4: net/mlx5: fix stabilized function versions

### Errors

1. **Incorrect function versioning usage**: The `RTE_DEFAULT_SYMBOL` and `RTE_VERSION_EXPERIMENTAL_SYMBOL` macros are being used incorrectly. These macros expect function **definitions**, but the patch is attempting to call the default symbol from within the experimental symbol wrapper.

```c
/* BAD - experimental version calls the default version by name */
RTE_VERSION_EXPERIMENTAL_SYMBOL(int, rte_pmd_mlx5_driver_event_cb_register,
                (rte_pmd_mlx5_driver_event_callback_t cb, void *opaque))
{
    return rte_pmd_mlx5_driver_event_cb_register(cb, opaque);  /* ERROR: recursive call to wrong symbol */
}
```

The issue is that `rte_pmd_mlx5_driver_event_cb_register(cb, opaque)` inside the `_exp` version will resolve to the experimental version itself (leading to recursion) or be ambiguous. According to the DPDK versioning pattern, you need to call the versioned symbol explicitly (e.g., `rte_pmd_mlx5_driver_event_cb_register_v26`) or implement the logic directly in each versioned function.

The correct pattern (based on DPDK `rte_function_versioning.h` semantics) is:

```c
/* Internal implementation - not exported */
static int
rte_pmd_mlx5_driver_event_cb_register_impl(rte_pmd_mlx5_driver_event_callback_t cb, void *opaque)
{
    /* actual implementation here */
}

/* DPDK 26 stable version */
RTE_DEFAULT_SYMBOL(26, int, rte_pmd_mlx5_driver_event_cb_register,
           (rte_pmd_mlx5_driver_event_callback_t cb, void *opaque))
{
    return rte_pmd_mlx5_driver_event_cb_register_impl(cb, opaque);
}

/* Experimental version (25.11 compatibility) */
RTE_VERSION_EXPERIMENTAL_SYMBOL(int, rte_pmd_mlx5_driver_event_cb_register,
                (rte_pmd_mlx5_driver_event_callback_t cb, void *opaque))
{
    return rte_pmd_mlx5_driver_event_cb_register_impl(cb, opaque);
}
```

Or, if the implementations are identical (as appears to be the case here), a single shared implementation function should be used.

**This error applies to all four functions in this patch:**
- `rte_pmd_mlx5_driver_event_cb_register`
- `rte_pmd_mlx5_driver_event_cb_unregister`
- `rte_pmd_mlx5_disable_steering`
- `rte_pmd_mlx5_enable_steering`

2. **Missing export macros on implementation**: The original implementation (now the `v26` default symbol) should be the one containing the full function body. The current patch structure moves the implementation into the `RTE_DEFAULT_SYMBOL` macro invocation, which is correct, but the experimental version then tries to call a non-existent function. The versioning macros generate the symbol versions; they don't create callable function names you can use internally.

### Warnings

None.

---

## Patch 4/4: ethdev: fix promoted flow metadata symbols

### Errors

1. **Same incorrect versioning pattern as Patch 3**: The experimental version attempts to call `rte_flow_dynf_metadata_register()` by name, which will not work correctly with symbol versioning.

```c
/* BAD */
RTE_VERSION_EXPERIMENTAL_SYMBOL(int, rte_flow_dynf_metadata_register, (void))
{
    return rte_flow_dynf_metadata_register();  /* ERROR: ambiguous/recursive call */
}
```

Should be:

```c
static int
rte_flow_dynf_metadata_register_impl(void)
{
    /* actual implementation */
}

RTE_DEFAULT_SYMBOL(26, int, rte_flow_dynf_metadata_register, (void))
{
    return rte_flow_dynf_metadata_register_impl();
}

RTE_VERSION_EXPERIMENTAL_SYMBOL(int, rte_flow_dynf_metadata_register, (void))
{
    return rte_flow_dynf_metadata_register_impl();
}
```

2. **Global variable symbol versioning is unconventional**: The patch uses `RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_flow_dynf_metadata_offs, 19.11)` for global variables. While this may be technically valid for the linker, it is unusual in DPDK. Global variable symbols are typically exported with `RTE_EXPORT_SYMBOL` only, without versioning, because:
   - Variables don't have function-call semantics that can be versioned
   - The memory location of a global variable is what matters, not multiple "versions"
   - Symbol versioning is primarily for functions where the signature or behavior changes

However, if the stated goal is to maintain `EXPERIMENTAL` version tags on these variables for one more release (until 26.11), and this linker approach works, it may be acceptable as a transitional measure. The commit message acknowledges this is a temporary workaround.

**Recommendation**: Verify with a DPDK maintainer whether global variable symbol versioning in this manner is acceptable. The standard approach would be to keep the variables stable and just ensure any code using them can handle both old and new semantics. The commit message references a mailing list discussion suggesting this approach was agreed upon, so this may be acceptable despite being unconventional.

### Warnings

1. **Variable symbol versioning is non-standard**: As noted above, using symbol versioning on global variables (`rte_flow_dynf_metadata_offs`, `rte_flow_dynf_metadata_mask`) is unusual. If the approach was pre-approved (per the referenced mailing list thread), this is acceptable, but it should be noted as a special case.

2. **Missing release notes**: The patch fixes an ABI compatibility issue (Bugzilla 1957) but does not update release notes. This is a significant bug fix that affects application compatibility and should be documented in `doc/guides/rel_notes/release_26_07.rst` (or the current release file).

---

## Summary

### Critical Issues (must fix before merge)

**Patch 3 and Patch 4**: Incorrect use of function versioning macros. The experimental and default symbol versions both need to call a shared internal implementation function, not call each other by the public symbol name. The current code will not produce the intended symbol versions and may cause undefined behavior (recursion or linker errors).

### Recommended Changes

- Refactor versioned functions to use a shared `_impl` function
- Add release notes entry for Patch 4 (ABI compatibility fix for Bugzilla 1957)
- Confirm with maintainers that global variable symbol versioning (Patch 4) is the intended approach

### What Works Well

- Patch 1 is a correct and minimal fix
- Patch 2 correctly extends function versioning support to drivers
- The overall goal of restoring ABI compatibility is appropriate and well-motivated