[RFC PATCH] tools/power turbostat: Do not print negative LPI residency

[RFC PATCH] tools/power turbostat: Do not print negative LPI residency

[Chen Yu]

turbostat prints the abnormal SYS%LPI across suspend-to-idle:
SYS%LPI = 114479815993277.50

This is reproduced by:
Run a freeze cycle, e.g. "sleepgraph -m freeze -rtcwake 15".
Then do a reboot. After boot up, launch the suspend-idle-idle
and check the SYS%LPI field.

The slp_so residence counter is in LPIT table, and BIOS does not
clears this register across reset. The PMC expects the OS to calculate
the LPI residency based on the delta. However, there is an firmware
issue that the LPIT gets cleared to 0 during the second suspend
to idle after the reboot, which brings negative delta value.

Prints a simple 0 to indicate this error to not confuse the user.

Reported-by: Todd Brandt <todd.e.brandt@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 9a10512e3407..3fa5f9a0218a 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1472,8 +1472,16 @@ int delta_package(struct pkg_data *new, struct pkg_data *old)
 	old->pc8 = new->pc8 - old->pc8;
 	old->pc9 = new->pc9 - old->pc9;
 	old->pc10 = new->pc10 - old->pc10;
-	old->cpu_lpi = new->cpu_lpi - old->cpu_lpi;
-	old->sys_lpi = new->sys_lpi - old->sys_lpi;
+	if (new->cpu_lpi > old->cpu_lpi) {
+		old->cpu_lpi = new->cpu_lpi - old->cpu_lpi;
+	} else {
+		old->cpu_lpi = 0;
+	}
+	if (new->sys_lpi > old->sys_lpi) {
+		old->sys_lpi = new->sys_lpi - old->sys_lpi;
+	} else {
+		old->sys_lpi = 0;
+	}
 	old->pkg_temp_c = new->pkg_temp_c;
 
 	/* flag an error when rc6 counter resets/wraps */
-- 
2.25.1

Re: [RFC PATCH] tools/power turbostat: Do not print negative LPI residency

[kernel test robot]

Hi Chen,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on acpi/turbostat]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Yu/tools-power-turbostat-Do-not-print-negative-LPI-residency/20231022-135448
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
patch link:    https://lore.kernel.org/r/20231022055221.569634-1-yu.c.chen%40intel.com
patch subject: [RFC PATCH] tools/power turbostat: Do not print negative LPI residency
reproduce: (https://download.01.org/0day-ci/archive/20231022/202310221511.JAAHk5a1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310221511.JAAHk5a1-lkp@intel.com/

# many are suggestions rather than must-fix

WARNING:BRACES: braces {} are not necessary for any arm of this statement
#38: FILE: tools/power/x86/turbostat/turbostat.c:2046:
+	if (new->cpu_lpi > old->cpu_lpi) {
[...]
+	} else {
[...]

WARNING:BRACES: braces {} are not necessary for any arm of this statement
#43: FILE: tools/power/x86/turbostat/turbostat.c:2051:
+	if (new->sys_lpi > old->sys_lpi) {
[...]
+	} else {
[...]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH] tools/power turbostat: Do not print negative LPI residency

[Len Brown]

BIOS bugs:-(

I agree that printing 0 is an improvement over printing an insane
negative number.

But printing 0 suggests that there was no residency, and that could be
misleading...

Maybe we should output some kind of warning about the broken BIOS?

On Sun, Oct 22, 2023 at 1:53 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> turbostat prints the abnormal SYS%LPI across suspend-to-idle:
> SYS%LPI = 114479815993277.50
>
> This is reproduced by:
> Run a freeze cycle, e.g. "sleepgraph -m freeze -rtcwake 15".
> Then do a reboot. After boot up, launch the suspend-idle-idle
> and check the SYS%LPI field.
>
> The slp_so residence counter is in LPIT table, and BIOS does not
> clears this register across reset. The PMC expects the OS to calculate
> the LPI residency based on the delta. However, there is an firmware
> issue that the LPIT gets cleared to 0 during the second suspend
> to idle after the reboot, which brings negative delta value.
>
> Prints a simple 0 to indicate this error to not confuse the user.
>
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 9a10512e3407..3fa5f9a0218a 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -1472,8 +1472,16 @@ int delta_package(struct pkg_data *new, struct pkg_data *old)
>         old->pc8 = new->pc8 - old->pc8;
>         old->pc9 = new->pc9 - old->pc9;
>         old->pc10 = new->pc10 - old->pc10;
> -       old->cpu_lpi = new->cpu_lpi - old->cpu_lpi;
> -       old->sys_lpi = new->sys_lpi - old->sys_lpi;
> +       if (new->cpu_lpi > old->cpu_lpi) {
> +               old->cpu_lpi = new->cpu_lpi - old->cpu_lpi;
> +       } else {
> +               old->cpu_lpi = 0;
> +       }
> +       if (new->sys_lpi > old->sys_lpi) {
> +               old->sys_lpi = new->sys_lpi - old->sys_lpi;
> +       } else {
> +               old->sys_lpi = 0;
> +       }
>         old->pkg_temp_c = new->pkg_temp_c;
>
>         /* flag an error when rc6 counter resets/wraps */
> --
> 2.25.1
>


-- 
Len Brown, Intel