Re: [LTP] [PATCH v14] thermal: add new test group

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

Hi Piotr,

> +static int nproc, temp_high, temp, trip, tz_counter;

`trip` is a single int, but cleanup() uses it to restore trip_point_1_temp
for ALL x86_pkg_temp zones. If there are multiple such zones, each
test_zone() call overwrites `trip` at the SAFE_FILE_SCANF, so only the
last zone's original value survives. The earlier zones get the wrong
value restored.

This needs to be a per-zone array, e.g. `static int *trip_orig;`
allocated alongside x86_pkg_temp_tz in setup(), with each zone's
original trip value saved individually.

> +	interrupt_init = calloc(nproc, sizeof(uint64_t));
> +	interrupt_later = calloc(nproc, sizeof(uint64_t));

[...]

> +	x86_pkg_temp_tz = calloc(tz_counter, sizeof(bool));

LTP has SAFE_CALLOC() in tst_safe_macros.h. Please use it instead of
raw calloc() — it handles the NULL check and calls tst_brk(TBROK) on
failure.

> +			tst_res(TDEBUG, "interrupts[%d]: %ld", i, interrupts[i]);

interrupts[i] is uint64_t but printed with %ld. Use PRIu64:

    tst_res(TDEBUG, "interrupts[%d]: %" PRIu64, i, interrupts[i]);

Same issue below in run():

> +				tst_res(TFAIL, "CPU %d interrupt counter: %ld (previous: %ld)",
> +					i, interrupt_later[i], interrupt_init[i]);

> +		SAFE_FILE_SCANF(path, "%s", line);

%s without a width limit can overflow line[8192]. Use "%8191s".

> +static void *cpu_workload(double run_time)
> +{

[...]

> +	return NULL;
> +}

cpu_workload() returns void * as if it were a pthread start routine, but
it is only called directly from the child process. It should return void.

> +	for (int i = 0; i < tz_counter; i++) {
> +		if (x86_pkg_temp_tz[i]) {
> +			read_interrupts(interrupt_init, nproc);
> +			test_zone(i);
> +			read_interrupts(interrupt_later, nproc);
> +			for (int i = 0; i < nproc; i++) {

Inner `i` shadows the outer loop variable `i`. Please rename to `j` or
`cpu` to avoid confusion.

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

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