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

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

Hi Piotr,

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

`interrupts` is uint64_t*, so we should use %ul instead.

> +static void run(void)
> +{
> +	for (int i = 0; i < tz_counter; i++) {
> +		if (x86_pkg_temp_tz[i])
> +			test_zone(i);
> +	}
> +	read_interrupts(interrupt_later, nproc);
> +
> +	for (int i = 0; i < nproc; i++) {
> +		if (interrupt_later[i] < interrupt_init[i])
> +			tst_res(TFAIL, "CPU %d interrupt counter: %ld (previous: %ld)",
> +				i, interrupt_later[i], interrupt_init[i]);

We always consider TFAIL when counter decreases, but we never consider when
it increases. Is there a reason for that?

> +	}
> +
> +	if (temp <= temp_high)
> +		tst_res(TFAIL, "Zone temperature is not rising as expected");
> +	else
> +		tst_res(TPASS, "x86 package thermal interrupt triggered");
> +}

I also have other questions in here. Why are we considering only the
last zone temperature? Is there a reason for it, or we should save all
temperature for all zones and eventually verify temperature increased
specifically for each one of them?

Because in a single socket system (I guess) we have one single
temperature for all the zones, but on i.e. dual socket server, this
test would verify that only the last zone has increased temperature
above the higher level. And this is wrong, according to the goal of
this test. We want to verify that kernel is correctly working for all 
systems, correctly increasing the thermal counter for each thermal
zone. If this is correct, `temp` and `temp_high` should be an array,
where each item is associated to a zone, and it should be processed
only at the end for TPASS/TFAIL.


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

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