Re: [PATCH v4 0/6] power: uncore power improvements and auto-detection

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 29 Jul 2026 10:51:43 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> This series improves the uncore power management in l3fwd-power
> and adds automatic detection of uncore drivers in the power library.
> 
> - Fix uncore deinitialization for non-legacy modes.
> - Enable power QoS for all modes (not just legacy).
> - Fix uncore help text and log messages.
> - Relocate uncore initialization from arg parsing to init_power_library().
> - Support automatic probing of uncore drivers in AUTO_DETECT and delete
>   useless code for auto-detection uncore environment.
> 
> ---
>  v4:
>  - Fix '!global_uncore_ops' to 'global_uncore_ops == NULL' from AI review.
>  v3 link:
>   https://inbox.dpdk.org/dev/20260728120208.1858832-1-lihuisong@huawei.com/
> 
>  v3:
>  - update feature to release_26_11.rst.
>  - add a new patch to delte useless for automatic detection environment.
>  v2 link:
>   https://inbox.dpdk.org/dev/20260526081138.1434947-1-lihuisong@huawei.com/
> 
>  v2:
>  - Remove the patch which add global uncore init and deinit interface.
>  - Remove the last patch in l3fwd-power about these new interface.
>    Will send them after this series.
>  v1 link:
>   https://inbox.dpdk.org/dev/20260512023513.460169-1-lihuisong@huawei.com/
> 
> Huisong Li (6):
>   examples/l3fwd-power: fix uncore deinit for non-legacy
>   examples/l3fwd-power: enable power QoS for all modes
>   examples/l3fwd-power: fix uncore help and log info
>   examples/l3fwd-power: relocate uncore initialization
>   power: support automatic detection of uncore driver
>   power: remove unused auto-detection uncore
> 
>  doc/guides/rel_notes/release_26_11.rst        |   6 +
>  .../sample_app_ug/l3_forward_power_man.rst    |   2 +-
>  examples/l3fwd-power/main.c                   | 256 +++++++++---------
>  lib/power/rte_power_uncore.c                  |  78 +++---
>  4 files changed, 185 insertions(+), 157 deletions(-)
> 

Most of the developers are off enjoying summer vacation.
But AI is still around and finds some things here:
Note: some of what it complains about is noise.


Applied to main (26.11-rc0) and reviewed against source. Applies cleanly.

Note on check-git-log: it reports "Wrong 'Fixes' reference" for both
tags. That is a shallow-clone artifact. Both references are correct
against full history:
  10db2a5b8724 ("examples/l3fwd-power: add options for uncore frequency")
  3b3af56d3c9c ("power: fix uncore configuration")


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

Warning: new file-scope variable g_uncore_cfg is neither static nor
prefixed. It is used only in main.c. Make it static and drop the g_
prefix, which is not DPDK style.

  static struct uncore_cfg {
          enum uncore_choice uncore_choice;
          uint32_t freq_idx;
  } uncore_cfg;

Info: the out-of-range message carries over an off-by-one. The test
rejects freq_idx > freq_array_len - 1 but the message says "choose a
value from 0 to %d" with freq_array_len. Since the line is being
rewritten anyway, print freq_array_len - 1.


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

Error: resource leak in power_uncore_probe_driver().

          ret = ops->init(0, 0);
          if (ret == 0) {
                  uint32_t env = power_uncore_driver_name2env(ops->name);
                  if (env == UINT32_MAX)
                          continue;
                  ...
                  ops->exit(0, 0);

On the env == UINT32_MAX path the driver has already been initialized
successfully but ops->exit(0, 0) is never called before moving to the
next driver. For intel_uncore that leaves f_cur_min and f_cur_max open
and the original min/max frequencies never written back. Restructure so
exit() runs on every successful init:

          ret = ops->init(0, 0);
          if (ret != 0)
                  continue;
          env = power_uncore_driver_name2env(ops->name);
          ops->exit(0, 0);
          if (env == UINT32_MAX)
                  continue;
          global_uncore_env = env;
          global_uncore_ops = ops;
          break;

Not reachable with the two in-tree drivers, since both "intel-uncore"
and "amd-hsmp" are in uncore_env_str, but the branch is deliberate and
is wrong as written.

Error: function return type must be on its own line.

  static uint32_t power_uncore_driver_name2env(char *name)
  static int power_uncore_probe_driver(void)

should be

  static uint32_t
  power_uncore_driver_name2env(const char *name)

  static int
  power_uncore_probe_driver(void)

The parameter should also be const char *; ops->name is only read.

Warning: the release note is filed under "New Features" but the commit
carries Fixes: and Cc: stable@dpdk.org. Pick one. Either this is a
backportable fix, in which case drop the New Features entry, or it is a
feature, in which case drop Fixes and Cc: stable. As written the stable
maintainers receive a patch that advertises itself as a new feature.

Info: the AUTO_DETECT path now returns -ENODEV while every other error
path in rte_power_set_uncore_env() returns -1. Not wrong, but the error
convention is now inconsistent within one function.


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

Warning: behaviour change to an exported stable symbol with no release
note. Previously rte_power_uncore_init() fell back to auto-detection
when no environment had been set; it now returns -1 with "Please set
uncore environment first." Applications relying on the implicit
fallback break at runtime with no build-time signal. Add a release note
describing the new requirement.