Re: [LTP] [PATCH v8] cpufreq.c: add new test for cpufreq sysfs interface validation

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

Hi Piotr,

> +static void cleanup(void)
> +{
> +	char path[PATH_MAX];
> +
> +	if (!setup_done) {
> +		free(online);
> +		free(previous_scaling_max_freq);
> +		free(previous_scaling_min_freq);
> +		free(previous_scaling_governor);

Better to use goto at the end of the cleanup() function, instead
of defining free() in two different parts and duplicating code.

> +		return;
> +	}
> +
> +	SAFE_FILE_PRINTF("/sys/devices/system/cpu/intel_pstate/status", "%s", intel_pstate_status);
> +	SAFE_FILE_PRINTF("/sys/devices/system/cpu/intel_pstate/no_turbo", "%d", no_turbo);

For fixed sysfs files that we need to modify and restore, we should use
.save_restore instead.

> +static void setup(void)
> +{
> +	char path[PATH_MAX];
> +
> +	if (access("/sys/devices/system/cpu/intel_pstate/status", F_OK) == -1)
> +		tst_brk(TCONF, "intel_pstate driver not active");

Are you sure it's not a TBROK/TFAIL? We are checking for the driver
configuration inside the kernel at tst_test level. I expect that
.../cpu/intel_pstate/status is present at this point. If it's not,
we are probably facing a bug in the driver, isnt it?

[..]

> +
> +	SAFE_FILE_SCANF("/sys/devices/system/cpu/intel_pstate/no_turbo", "%d", &no_turbo);
> +	SAFE_FILE_SCANF("/sys/devices/system/cpu/intel_pstate/status", "%15s", intel_pstate_status);

And since we are scanning the status value later on, we probably don't
need the access(.../cpu/intel_pstate/status) verification. The test will
TBROK anyway here.

We should probably use .save_restore at this point, since we can safely
guard the test from having drivers issues at configuration level.
In particular, we should use .save_restore with TST_SR_TBROK (or
TST_SR_TCONF if you confirm that driver might not initialize the
sysfs under certain circumstances). 

> +		snprintf(path, sizeof(path), "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_available_governors", i);
> +		memset(contents, 0, sizeof(contents));
> +		SAFE_FILE_SCANF(path, "%255[^\n]", contents);
> +
> +		tst_res(TDEBUG, "Checking whether %s contains \"performance\" and \"schedutil\"", path);

Please change all of these double quotes inside tst_res() message parameter
single quotes (as Cyril mentioned already).

> +		SAFE_FILE_SCANF(path, "%255s", contents);
> +
> +		if (strstr(contents, "schedutil"))

Also, I'm a bit concern for the massive use of strstr() patterns.
Again, if drivers are bugged and show a string with garbage
data matching one of the "supposed" correct string, we pass the test
while drivers are still bugged.

You are also missing these two configurations which are needed
by the test:

- CONFIG_CPU_FREQ_GOV_SCHEDUTIL
- CONFIG_CPU_FREQ_GOV_PERFORMANCE

Otherwise, it's not possible to run:

> +      SAFE_FILE_PRINTF(path, "schedutil");

or setting CPU governor to performance.

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

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