|WARNING| [v3,6/6] power: remove unused auto-detection uncore

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-28

# Review of DPDK Patch Series

## Patch 1/6: examples/l3fwd-power: fix uncore deinit for non-legacy

### Errors

**Uncore deinitialization cleanup missing on error path**

In `deinit_power_library()` around line 2365, if uncore deinitialization fails (line 2346-2365), the function returns early without restoring CPU resume latency (lines 2368-2373). This leaves the system in an inconsistent state where latency settings are not restored.

```c
/* Current code returns early on uncore deinit failure */
for (pkg = 0; pkg < max_pkg; pkg++) {
    max_die = rte_power_uncore_get_num_dies(pkg);
    for (die = 0; die < max_die; die++) {
        ret = rte_power_uncore_exit(pkg, die);
        if (ret == -1) {
            /* Returns here without restoring cpu_resume_latency */
            return ret;
        }
    }
}

/* This code is never reached if the above fails */
if (app_mode == APP_MODE_LEGACY && cpu_resume_latency != -1) {
    /* ... restore latency ... */
}
```

Fix: collect errors but continue cleanup, or move latency restore before uncore exit.

---

## Patch 2/6: examples/l3fwd-power: enable power QoS for all modes

No issues found.

---

## Patch 3/6: examples/l3fwd-power: fix uncore help and log info

No issues found.

---

## Patch 4/6: examples/l3fwd-power: relocate uncore initialization

### Errors

**Missing error cleanup path in `power_uncore_init()`**

In `power_uncore_init()` (lines 2192-2261), if any `rte_power_uncore_init()` call succeeds but a later frequency setting call fails (lines 2234-2257), the successfully initialized pkg/die pairs are not deinitialized before returning -1. This leaks initialized uncore state.

```c
for (pkg = 0; pkg < max_pkg; pkg++) {
    for (die = 0; die < max_die; die++) {
        ret = rte_power_uncore_init(pkg, die);
        if (ret == -1) {
            /* ... */
            return ret;  /* OK - nothing initialized yet for this die */
        }
        /* Set frequency - if this fails, prior pkg/die are left initialized */
        if (g_uncore_cfg.uncore_choice == UNCORE_MIN) {
            ret = rte_power_uncore_freq_min(pkg, die);
            if (ret == -1) {
                /* BUG: returns without deinitializing prior successful inits */
                return ret;
            }
        }
        /* ... similar for UNCORE_MAX and UNCORE_IDX ... */
    }
}
```

Fix: on error, loop through all successfully initialized pkg/die pairs and call `rte_power_uncore_exit()` before returning.

**Redundant check for `freq_array_len <= 0`**

Line 2240: `rte_power_uncore_get_num_freqs()` returns `int`, checked against `<= 0`. The function returns the number of frequencies available, which is a count that should be non-negative. A return of exactly zero indicates no frequencies available, which is a valid (though unusual) state to handle. The check should be `< 0` for error detection, with `== 0` handled separately if needed.

However, checking the API: if this function returns a negative value on error and a count >= 0 on success, then `<= 0` is correct. Only flag if the API documentation shows zero is a valid success return that should be handled differently. Without seeing the function contract, assume the code is acceptable.

---

## Patch 5/6: power: support automatic detection of uncore driver

### Errors

**Missing `#include <string.h>` for `strcmp()`**

Line 2+: includes `<errno.h>` but does not include `<string.h>`. The new function `power_uncore_driver_name2env()` at line 52 uses `strcmp()`, which requires `<string.h>`. This will cause a compilation failure (implicit declaration warning/error).

Fix: Add `#include <string.h>` at the top of the file.

**Error path resource leak in `power_uncore_probe_driver()`**

In `power_uncore_probe_driver()` (lines 60-80), if `ops->init(0, 0)` succeeds but `power_uncore_driver_name2env(ops->name)` returns `UINT32_MAX` (line 69), the function continues without calling `ops->exit(0, 0)` for that driver. The next iteration may succeed and call `exit()` on a *different* driver, leaving the first driver initialized.

```c
RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next) {
    ret = ops->init(0, 0);
    if (ret == 0) {
        uint32_t env = power_uncore_driver_name2env(ops->name);
        if (env == UINT32_MAX)
            continue;  /* BUG: ops is left initialized */
        global_uncore_env = env;
        global_uncore_ops = ops;
        ops->exit(0, 0);  /* Only called if env != UINT32_MAX */
        break;
    }
}
```

Fix: call `ops->exit(0, 0)` before `continue` when `env == UINT32_MAX`.

---

## Patch 6/6: power: remove unused auto-detection uncore

No issues found.

---

## Summary

**Errors (must fix):**

1. **Patch 1**: Uncore deinitialization error path skips CPU resume latency restoration
2. **Patch 4**: `power_uncore_init()` missing cleanup on error path (uncore driver state leak)
3. **Patch 5**: Missing `#include <string.h>` for `strcmp()`
4. **Patch 5**: Resource leak in `power_uncore_probe_driver()` when driver init succeeds but name lookup fails

**Style/Process:**
All patches comply with DPDK coding standards. No style issues found.