|WARNING| [v4,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/166994

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None.

### Warnings

None.

### Info

The patch correctly moves uncore deinitialization to all app modes, fixing a resource leak where uncore frequency was not restored on exit in non-legacy modes. The logic is sound: uncore initialization was already performed for all modes, so deinitialization should match.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch enables power QoS for all modes, which is reasonable since power QoS doesn't depend on app mode. No issues found.

---

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

### Errors

None.

### Warnings

None.

### Info

Documentation and log message corrections. No code issues.

---

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

### Errors

None.

### Warnings

**Missing error handling propagation:**
In `power_uncore_init()`, when `rte_power_uncore_get_num_freqs()` returns <= 0, the function logs an error and returns -1. However, in the outer loop, if the first package/die succeeds but a later one fails, the previously initialized packages are not cleaned up. This creates a resource leak on the partial-success path.

The pattern should be:
```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) {
            /* Clean up previously initialized pkg/die */
            goto cleanup;
        }
        /* ... configure frequency ... */
        if (ret == -1)
            goto cleanup;  /* Same cleanup needed */
    }
}
return 0;

cleanup:
    /* Call rte_power_uncore_exit for all initialized pkg/die */
    ...
```

Currently, the code returns -1 immediately on any failure, leaving earlier initialized uncore resources active.

**`freq_array_len` comparison with `freq_idx`:**
The check `if (g_uncore_cfg.freq_idx > (uint32_t)(freq_array_len - 1))` is safe because `freq_array_len` is already verified to be > 0 just above. However, the cast `(uint32_t)(freq_array_len - 1)` is unnecessary since both sides are unsigned after the > 0 check. This is not an error, but the simpler `if (g_uncore_cfg.freq_idx >= (uint32_t)freq_array_len)` is clearer.

### Info

The refactoring itself (moving uncore init logic to `init_power_library()`) is a good cleanup that improves maintainability.

---

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

### Errors

**Resource leak on probing failure:**
In `power_uncore_probe_driver()`, the function iterates through all registered uncore drivers, calls `ops->init(0, 0)` to probe, and calls `ops->exit(0, 0)` only when the probe succeeds. If a probe returns 0 (success) but the `power_uncore_driver_name2env()` returns `UINT32_MAX` (driver name not found in the table), the code continues to the next driver **without calling `ops->exit(0, 0)`**. This leaves the successfully initialized uncore driver for package-0, die-0 in an initialized state.

Corrected pattern:
```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) {
            ops->exit(0, 0);  /* Must clean up */
            continue;
        }
        global_uncore_env = env;
        global_uncore_ops = ops;
        ops->exit(0, 0);
        break;
    }
}
```

### Warnings

None.

### Info

The feature itself (probing for uncore drivers instead of hardcoding `intel_uncore`) is a valuable improvement.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch correctly removes redundant auto-detection code in `rte_power_uncore_init()` now that `rte_power_set_uncore_env()` handles probing. The logic is sound: applications must call `rte_power_set_uncore_env()` first, so the per-pkg/die init path does not need its own probing loop.

---

## Summary

**Errors (must fix):**
- **PATCH 5**: Resource leak in `power_uncore_probe_driver()` when a driver initializes successfully but its name is not found in the environment table (missing `ops->exit(0, 0)` before `continue`).

**Warnings (should fix):**
- **PATCH 4**: Missing cleanup on partial initialization failure in `power_uncore_init()` (if a later pkg/die fails to initialize, earlier ones remain active).

**General:**
The series fixes a real bug (uncore not deinitialized on app exit in non-legacy modes) and adds useful functionality (driver probing). The correctness issues are fixable with proper cleanup paths.