Re: [LTP] [PATCH v6] high_freq_hwp_cap_cppc.c: new test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Piotr,

Thanks for the new test. A few issues below.

> +/*
> + * Copyright (C) 2026 Intel - http://www.intel.com/
> + */

The copyright line should include an author name and email, e.g.:

  Copyright (c) 2026 Piotr Kubaj <piotr.kubaj@intel.com>

> +		if (i)
> +			SAFE_FILE_SCANF(path, "%d", &online);

SAFE_FILE_SCANF with "%d" expects a pointer to int, but `online` is
bool (typically 1 byte). This is undefined behavior and will likely
corrupt the stack on little-endian. Please use `int online = 1;`
instead.

This should be spotted by any static analyzer or clangd..and even
gcc shows this error.

> +		snprintf(path, sizeof(path), "/dev/cpu/%d/msr", i);
> +		int fd = SAFE_OPEN(path, O_RDONLY);
> +
> +		SAFE_PREAD(1, fd, &msr_highest_perf, sizeof(msr_highest_perf), 0x771);

SAFE_CLOSE(fd) right after it has been used. There's no need to keep it
open after pread().

> +		msr_highest_perf &= (1ULL << 8) - 1;

0x771 and the 8-bit mask are magic numbers. Named defines would
help readability, e.g.:

  #define MSR_HWP_CAPABILITIES  0x771
  #define HIGHEST_PERF_MASK     0xFF

The test fails because msr device is not available, so you need:

  .needs_drivers = (const char *const []) {                                                                                                        
      "msr",                                                                                                                                       
      NULL                                                                                                                                         
  },

And probably the CONFIG_X86_MSR=y check as well.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp