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

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

[linuxtestproject.agent]

Hi Piotr,

On Tue, 16 Jun 2026, Piotr Kubaj wrote:
> rfim: add new test for verifying RFIM sysfs interface

> /*\
>  * 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.
>  */

The test sets .needs_root = 1 but the doc comment does not explain
why root is required. Other tests in the same directory document
this (e.g. "The test needs root because reading /dev/cpu/N/msr
needs CAP_SYS_RAWIO / root."). Could a line be added explaining
that root is needed for write-access checks on sysfs attributes?

> } 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");

The first two branches of this if/else-if/else chain use braces but
the final else does not. Kernel coding style requires braces on all
branches when any branch needs them.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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

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

[Piotr Kubaj]

Validate presence and permissions of the RFIM attributes exposed by the
Intel processor_thermal driver under the proc_thermal PCI device.

The kernel creates the dvfs, fivr and dlvr attribute groups
independently, each gated on its own per-device feature bit, so the test
detects which of the three groups the hardware presents and checks only
those. This covers platforms that expose both FIVR and DLVR (e.g. Meteor
Lake) as well as those with FIVR but no DVFS (e.g. Tiger Lake).

Read-only attributes (mode 0444) must accept O_RDONLY and reject a write
open with EACCES; read-write attributes (mode 0644) must accept O_RDWR.
The open() itself is what exercises access, since sysfs can refuse it in
kernfs even when the mode bits would allow it.

The test works only on Intel platforms with RFIM and needs root for the
read-write attributes, whose write bit is owner-only.

Signed-off-by: Piotr Kubaj <piotr.kubaj@intel.com>
---
Address feedback.
Additionaly:
1. I found that there are machines without DVFS, so don't require it.
2. It's possible for both FIVR and DLVR to be present, so don't assume
they are exclusive.

Regarding TST_EXP_FAIL(), I found that TST_EXP_FD() and TST_EXP_FAIL2()
fit better here.

 runtest/power_management_tests               |   1 +
 testcases/kernel/power_management/.gitignore |   1 +
 testcases/kernel/power_management/rfim01.c   | 163 +++++++++++++++++++
 3 files changed, 165 insertions(+)
 create mode 100644 testcases/kernel/power_management/rfim01.c

diff --git a/runtest/power_management_tests b/runtest/power_management_tests
index 4da57ee72..d05c95608 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
+rfim01 rfim01
 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..74f6df14f 100644
--- a/testcases/kernel/power_management/.gitignore
+++ b/testcases/kernel/power_management/.gitignore
@@ -1 +1,2 @@
 high_freq_hwp_cap_cppc
+rfim01
diff --git a/testcases/kernel/power_management/rfim01.c b/testcases/kernel/power_management/rfim01.c
new file mode 100644
index 000000000..c24ea60ff
--- /dev/null
+++ b/testcases/kernel/power_management/rfim01.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 Piotr Kubaj <piotr.kubaj@intel.com>
+ */
+
+/*\
+ * Validate presence and permissions of RFIM attributes.
+ * The kernel exposes the dvfs, fivr and dlvr attribute groups
+ * independently, gated on per-device feature bits, so the test checks
+ * whichever of the three groups the hardware presents.
+ *
+ * Permissions are verified by opening each node: read-only attributes
+ * must accept O_RDONLY and reject a write open, while read-write ones
+ * must accept O_RDWR. For sysfs nodes the mode bits alone are not
+ * authoritative - the kernel can still refuse the open in the file's
+ * own syscall handler - so open() is what actually exercises access.
+ *
+ * The test requires root because the read-write RFIM attributes are
+ * root-owned and created mode 0644, so only the owner holds the write
+ * bit. An unprivileged user could read them but would get EACCES when
+ * opening them O_RDWR, which check_read_write() would misreport as a
+ * failure rather than reflecting the actual access policy. The read-only
+ * attributes are mode 0444 and world-readable.
+ */
+
+#include "tst_test.h"
+
+#define RFIM_ROOT "/sys/bus/pci/devices/0000:00:04.0"
+
+static bool have_dvfs, have_fivr, have_dlvr;
+
+static 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
+};
+
+static const char * const ro_dvfs_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
+};
+
+static 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
+};
+
+static 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
+};
+
+static const char * const rw_dvfs_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
+};
+
+static bool has_group(const char *name)
+{
+	char path[PATH_MAX];
+	struct stat stats;
+
+	snprintf(path, sizeof(path), "%s/%s", RFIM_ROOT, name);
+
+	if (stat(path, &stats)) {
+		if (errno == ENOENT)
+			return false;
+		tst_brk(TBROK | TERRNO, "stat(%s)", path);
+	}
+
+	if (!S_ISDIR(stats.st_mode))
+		tst_brk(TBROK, "%s exists but is not a directory", path);
+
+	return true;
+}
+
+static void setup(void)
+{
+	have_dvfs = has_group("dvfs");
+	have_fivr = has_group("fivr");
+	have_dlvr = has_group("dlvr");
+
+	if (!have_dvfs && !have_fivr && !have_dlvr)
+		tst_brk(TCONF, "No RFIM attribute groups present under %s", RFIM_ROOT);
+}
+
+static void check_read_only(const char *path)
+{
+	int fd = TST_EXP_FD(open(path, O_RDONLY));
+
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+
+	TST_EXP_FAIL2(open(path, O_WRONLY), EACCES, "%s should reject writes", path);
+}
+
+static void check_read_write(const char *path)
+{
+	int fd = TST_EXP_FD(open(path, O_RDWR));
+
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+}
+
+static void run(void)
+{
+	if (have_dvfs) {
+		tst_res(TINFO, "Checking DVFS");
+		for (int i = 0; ro_dvfs_nodes[i]; i++)
+			check_read_only(ro_dvfs_nodes[i]);
+
+		for (int i = 0; rw_dvfs_nodes[i]; i++)
+			check_read_write(rw_dvfs_nodes[i]);
+	}
+
+	if (have_dlvr) {
+		tst_res(TINFO, "Checking DLVR");
+		for (int i = 0; ro_dlvr_nodes[i]; i++)
+			check_read_only(ro_dlvr_nodes[i]);
+
+		for (int i = 0; rw_dlvr_nodes[i]; i++)
+			check_read_write(rw_dlvr_nodes[i]);
+	}
+
+	if (have_fivr) {
+		tst_res(TINFO, "Checking FIVR");
+		for (int i = 0; fivr_nodes[i]; i++)
+			check_read_write(fivr_nodes[i]);
+	}
+}
+
+static struct tst_test test = {
+	.min_kver = "6.4",
+	.needs_cpu_vendor = "GenuineIntel",
+	.needs_kconfigs = (const char *const []) {
+		"CONFIG_INT340X_THERMAL",
+		NULL
+	},
+	.needs_root = 1,
+	.supported_archs = (const char *const []) {
+		"x86",
+		"x86_64",
+		NULL
+	},
+	.setup = setup,
+	.test_all = run
+};
-- 
2.47.3

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.


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

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

[linuxtestproject.agent]

Hi Piotr,

On Thu, 18 Jun 2026, Piotr Kubaj wrote:
> rfim: add new test for verifying RFIM sysfs interface

> +#define RFIM_ROOT "/sys/bus/pci/devices/0000:00:04.0"

The PCI BDF is hardcoded here. The proc_thermal driver matches by
PCI device ID (PCI_DEVICE_ID_INTEL_*_THERMAL), not by slot. While
00:04.0 is the typical BDF on current Intel client SoCs, could this
be discovered dynamically instead -- e.g. by scanning
/sys/bus/pci/devices/*/driver for a proc_thermal link, or by looking
up the device via its class or vendor:device pair?

This would prevent a silent TCONF on any future platform where the
thermal device appears at a different BDF.

> +static void check_read_only(const char *path)
> +{
> +	int fd = TST_EXP_FD(open(path, O_RDONLY));
> +
> +	if (fd >= 0)
> +		SAFE_CLOSE(fd);

LTP convention is to use "fd != -1" rather than "fd >= 0" for fd
validity checks. Same applies in check_read_write().

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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