Re: [PATCH] rfim: add new test for verifying RFIM sysfs interface

Hi Piotr,

> diff --git a/runtest/power_management_tests b/runtest/power_management_tests
> index 4da57ee72..8ebbcff84 100644
> --- a/runtest/power_management_tests
> +++ b/runtest/power_management_tests
> @@ -1,5 +1,6 @@
>  #POWER_MANAGEMENT
>  high_freq_hwp_cap_cppc high_freq_hwp_cap_cppc
> +rfim rfim

Use rfim01 instead. We might want to add more tests in the future.

>  runpwtests03 runpwtests03.sh
>  runpwtests04 runpwtests04.sh
>  runpwtests06 runpwtests06.sh
> diff --git a/testcases/kernel/power_management/.gitignore b/testcases/kernel/power_management/.gitignore
> index 03f0c83e4..ecc2931fa 100644
> --- a/testcases/kernel/power_management/.gitignore
> +++ b/testcases/kernel/power_management/.gitignore
> @@ -1 +1,2 @@
>  high_freq_hwp_cap_cppc
> +rfim
> diff --git a/testcases/kernel/power_management/rfim.c b/testcases/kernel/power_management/rfim.c
> new file mode 100644
> index 000000000..06bc144a2
> --- /dev/null
> +++ b/testcases/kernel/power_management/rfim.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 Piotr Kubaj <[email protected]>
> + */
> +
> +/*\
> + * Validate presence and permissions of RFIM attributes.
> + * The test checks first validity of general RFIM attributes,
> + * and then checks either DLVR or FIVR, depending on hardware.
> + */
> +
> +#include "tst_test.h"
> +
> +#define RFIM_ROOT "/sys/bus/pci/devices/0000:00:04.0"
> +
> +enum rfim_variant {
> +	RFIM_FIVR,
> +	RFIM_DLVR,
> +};
> +
> +static enum rfim_variant variant;
> +
> +static void setup(void)
> +{
> +	struct stat stats;
> +
> +	if (!stat(RFIM_ROOT"/dlvr", &stats)) {
> +		if (S_ISDIR(stats.st_mode))
> +			variant = RFIM_DLVR;
> +		else
> +			tst_brk(TBROK, "%s exists but is not a directory", RFIM_ROOT"/dlvr");
> +	} else if (!stat(RFIM_ROOT"/fivr", &stats)) {
> +		if (S_ISDIR(stats.st_mode))
> +			variant = RFIM_FIVR;
> +		else
> +			tst_brk(TBROK, "%s exists but is not a directory", RFIM_ROOT"/fivr");
> +	} else
> +		tst_brk(TCONF, "Neither %s nor %s exists", RFIM_ROOT"/dlvr", RFIM_ROOT"/fivr");

Avoid too many nesting. Use if() -> TBROK or TCONF, instead of having else
everywhere. First, handle the error, then proceed with the test.

> +}
> +
> +static void check_read_only(const char *path)
> +{
> +	tst_res(TDEBUG, "Checking whether %s is read-only", path);
> +
> +	if (access(path, F_OK)) {
> +		tst_res(TFAIL | TERRNO, "%s does not exist", path);
> +		return;
> +	}
> +
> +	int fd = open(path, O_RDONLY);
> +
> +	if (fd == -1) {
> +		tst_res(TFAIL | TERRNO, "%s can't be read", path);
> +		return;
> +	}
> +	close(fd);
> +
> +	fd = open(path, O_WRONLY);
> +	if (fd != -1) {
> +		close(fd);
> +		tst_res(TFAIL, "%s is writable", path);
> +		return;
> +	}
> +
> +	tst_res(TPASS, "%s is read-only", path);
> +}

This function can be easily replaced by access().

TST_EXP_PASS(access(path, R_OK));
TST_EXP_FAIL(access(path, W_OK), ..);

> +
> +static void check_read_write(const char *path)
> +{
> +	tst_res(TDEBUG, "Checking whether %s is read-write", path);
> +
> +	if (access(path, F_OK)) {
> +		tst_res(TFAIL | TERRNO, "%s does not exist", path);

TERRNO already show the message. Either you use TERRNO or you specify
a message error by hand, not both. This is valid in general for all
other messages as well.

> +		return;
> +	}
> +
> +	int fd = open(path, O_RDWR);
> +
> +	if (fd != -1) {
> +		close(fd);
> +		tst_res(TPASS, "%s is read-write", path);
> +	} else
> +		tst_res(TFAIL | TERRNO, "%s is not read-write", path);
> +}

This one as well can be replaced with access().

> +
> +static void run(void)
> +{
> +	const char * const fivr_nodes[] = {
> +		RFIM_ROOT"/fivr/vco_ref_code_lo",
> +		RFIM_ROOT"/fivr/vco_ref_code_hi",
> +		RFIM_ROOT"/fivr/spread_spectrum_pct",
> +		RFIM_ROOT"/fivr/spread_spectrum_clk_enable",
> +		RFIM_ROOT"/fivr/rfi_vco_ref_code",
> +		RFIM_ROOT"/fivr/fivr_fffc_rev",
> +		NULL
> +	};
> +
> +	const char * const ro_general_nodes[] = {
> +		RFIM_ROOT"/dvfs/ddr_data_rate_point_0",
> +		RFIM_ROOT"/dvfs/ddr_data_rate_point_1",
> +		RFIM_ROOT"/dvfs/ddr_data_rate_point_2",
> +		RFIM_ROOT"/dvfs/ddr_data_rate_point_3",
> +		NULL
> +	};
> +
> +	const char * const ro_dlvr_nodes[] = {
> +		RFIM_ROOT"/dlvr/dlvr_hardware_rev",
> +		RFIM_ROOT"/dlvr/dlvr_freq_mhz",
> +		RFIM_ROOT"/dlvr/dlvr_pll_busy",
> +		NULL
> +	};
> +
> +	const char * const rw_dlvr_nodes[] = {
> +		RFIM_ROOT"/dlvr/dlvr_freq_select",
> +		RFIM_ROOT"/dlvr/dlvr_rfim_enable",
> +		RFIM_ROOT"/dlvr/dlvr_spread_spectrum_pct",
> +		RFIM_ROOT"/dlvr/dlvr_control_mode",
> +		RFIM_ROOT"/dlvr/dlvr_control_lock",
> +		NULL
> +	};
> +
> +	const char * const rw_general_nodes[] = {
> +		RFIM_ROOT"/dvfs/rfi_restriction_run_busy",
> +		RFIM_ROOT"/dvfs/rfi_restriction_err_code",
> +		RFIM_ROOT"/dvfs/rfi_restriction_data_rate_base",
> +		RFIM_ROOT"/dvfs/rfi_restriction_data_rate",
> +		NULL
> +	};

Please define them out as static arrays.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato-IBi9RG/[email protected]

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