Re: [LTP] power_management: rewrite runpwtests04.sh in C

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Sun, 17 May 2026 05:00:12 +0900, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C
>
> Convert the cpuidle sysfs smoke test from shell to C using the modern
> LTP test API.
>
> The new implementation preserves the original test semantics
> while removing shell dependencies.

The commit body needs a sentence on *why* the conversion is being done
now — what problem or goal motivates it (e.g. old API retirement, CI
coverage gap, etc.).

[...]

> +static void setup(void)
> +{
> +	if (access(CPUIDLE_PATH, R_OK))
> +		tst_brk(TCONF, "%s is not available", CPUIDLE_PATH);
> +}

The runtest file was not updated. `runpwtests04 runpwtests04.sh` in
runtest/power_management_tests still points to the deleted shell script.
Replace it with `cpuidle01 cpuidle01`.

---
Note:

Our agent completed the review of the patch. The full review can be
found at: https://github.com/linux-test-project/ltp-agent/actions/runs/25997280181

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Mon, 25 May 2026 00:42:20 +0900, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

[...]

> +static void verify_cpuidle(unsigned int i)
> +{
> +	int fd;
> +	char path[PATH_MAX];
> +	char buf[32];
> +
> +	snprintf(path, sizeof(path), "%s/%s", CPUIDLE_PATH, tcases[i].name);
> +
> +	fd = SAFE_OPEN(path, O_RDONLY);

SAFE_OPEN() on a non-existent file aborts the test with TBROK.  The
files `current_governor_ro` and `current_driver` may not exist on every
system that has a cpuidle directory (e.g. no cpuidle driver loaded, or
kernel lacks CONFIG_CPU_IDLE_GOV_*).  Use open() directly and convert
ENOENT to TCONF:

    fd = open(path, O_RDONLY);
    if (fd < 0) {
        if (errno == ENOENT)
            tst_res(TCONF, "%s not available", path);
        else
            tst_res(TFAIL | TERRNO, "open(%s) failed", path);
        return;
    }

> +static struct tcases {
> +	const char *name;
> +} tcases[] = {

Tag and variable share the same name.  LTP convention uses the singular
for the struct tag: `struct tcase { ... } tcases[] = {`.

[...]

The new test is named `pwtests01` while the shell test it replaces was
`runpwtests04`.  The commit message does not explain the rename.  If
`pwtests01` is intentional (e.g. start of a new naming series), please
document it briefly.

---
Note:

Our agent completed the review of the patch. The full review can be
found at: https://github.com/linux-test-project/ltp/actions

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Mon, May 25, 2026, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

> +static void verify_cpuidle(unsigned int i)
> +{
> +	int fd;
> +	char path[PATH_MAX];
> +	char buf[32];
> +
> +	snprintf(path, sizeof(path), "%s/%s", CPUIDLE_PATH, tcases[i].name);
> +
> +	fd = SAFE_OPEN(path, O_RDONLY);
> +
> +	SAFE_READ(0, fd, buf, sizeof(buf));
> +	SAFE_CLOSE(fd);

If SAFE_READ() triggers tst_brk(), execution jumps to cleanup but
there is no cleanup registered, so fd is never closed.

The LTP convention is to track file descriptors in a static variable
initialized to -1 and close them in a .cleanup handler:

  static int fd = -1;

  static void verify_cpuidle(unsigned int i)
  {
          ...
          fd = SAFE_OPEN(path, O_RDONLY);
          SAFE_READ(0, fd, buf, sizeof(buf));
          SAFE_CLOSE(fd);

          tst_res(TPASS, "%s read successfully", path);
  }

  static void cleanup(void)
  {
          if (fd != -1)
                  SAFE_CLOSE(fd);
  }

  static struct tst_test test = {
          .setup = setup,
          .cleanup = cleanup,
          ...
  };

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Fri, 12 Jun 2026 21:20:44 +0900, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

> diff --git a/testcases/kernel/power_management/pwtests01.c
> b/testcases/kernel/power_management/pwtests01.c
> new file mode 100644

The old shell script runpwtests04.sh is not deleted by this patch.
Since the Makefile installs all shell scripts via
`INSTALL_TARGETS := *.py *.sh`, the orphaned script will still be
installed even though its runtest entry was removed.

Should runpwtests04.sh (and potentially pm_include.sh if no other
script sources it) be deleted in this patch?

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Sat, 13 Jun 2026 02:13:46 +0900, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

> The new implementation preserves the original test semantics while
> removing shell dependencies.

This claim is inaccurate in at least two ways:

1. The original shell script calls check_arch from pm_include.sh,
which gates the test to x86/x86_64 and reports TCONF on other
architectures. The new C test drops this gate entirely, relying
on runtime sysfs presence instead. That is arguably better
(runtime detection over compile-time assumptions), but it is a
semantic change that should be mentioned.

2. When /sys/devices/system/cpu/cpuidle does not exist, the
original script skips the loop and falls through to PASS. The
new test calls tst_brk(TCONF) in setup. Again an improvement,
but not "preserved semantics".

Consider rewording the commit body to acknowledge these
intentional behavioral changes rather than claiming equivalence.

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Wed, 8 Jul 2026, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

> The new implementation preserves the original test semantics while
> removing shell dependencies.

The original shell test only checked current_governor_ro and
current_driver for readability. The new test also covers
available_governors, current_governor (read), and adds a write
rejection check against current_governor. That is an extension,
not a preservation of semantics. Please update the commit message
body to reflect what is actually added.

> +static void verify_cpuidle(unsigned int i)
> +{
> +	int ret;
> +	char path[PATH_MAX];
> +	char buf[32];
> +
> +	snprintf(path, sizeof(path), "%s/%s", CPUIDLE_PATH, tcases[i].name);
> +
> +	if (tcases[i].is_writable)
> +		fd = open(path, O_RDWR);
> +	else
> +		fd = open(path, O_RDONLY);

open() is a tested subject here -- its return code drives the
TCONF/TFAIL decision. Bare syscalls that are the subject of the
test must be wrapped in TEST(), not called directly. The same
applies to read() and write() below.

> +	ret = read(fd, buf, sizeof(buf));
> +
> +	if (ret < 0)
> +		tst_res(TFAIL | TERRNO, "%s read() failed", path);
> +	else if (ret == 0)
> +		tst_res(TFAIL, "%s read() returned 0", path);
> +	else
> +		tst_res(TPASS, "%s read() successfully", path);

read() is the primary subject of this test and must be wrapped in
TEST() so that TST_RET and TST_ERR are populated.

> +	if (tcases[i].is_writable) {
> +		if (write(fd, "invalid_governor", 16) < 0) {

Same for write().

> +static struct tst_test test = {
> +	.cleanup = cleanup,
> +	.needs_root = 1,

The doc comment only mentions readability. It does not explain why
root is required. Ground Rule 4 requires the reason to appear in
the /*\ ... */ block. Root is needed here for writing to
current_governor and for the save_restore of that path; please
document that.

> + * Verify that selected cpuidle sysfs files are readable.

The test also verifies that writing an invalid governor name to
current_governor is rejected with EINVAL. The description should
cover both aspects.

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[Andrea Cervesato via ltp]

Hi Jinseok,

> 
> open() is a tested subject here -- its return code drives the
> TCONF/TFAIL decision. Bare syscalls that are the subject of the
> test must be wrapped in TEST(), not called directly. The same
> applies to read() and write() below.

This is a false positive.

> 
> > +	ret = read(fd, buf, sizeof(buf));
> > +
> > +	if (ret < 0)
> > +		tst_res(TFAIL | TERRNO, "%s read() failed", path);
> > +	else if (ret == 0)
> > +		tst_res(TFAIL, "%s read() returned 0", path);
> > +	else
> > +		tst_res(TPASS, "%s read() successfully", path);
> 
> read() is the primary subject of this test and must be wrapped in
> TEST() so that TST_RET and TST_ERR are populated.

This is false positive.

> 
> > +	if (tcases[i].is_writable) {
> > +		if (write(fd, "invalid_governor", 16) < 0) {
> 
> Same for write().

This is false poitive.

> 
> > +static struct tst_test test = {
> > +	.cleanup = cleanup,
> > +	.needs_root = 1,
> 
> The doc comment only mentions readability. It does not explain why
> root is required. Ground Rule 4 requires the reason to appear in
> the /*\ ... */ block. Root is needed here for writing to
> current_governor and for the save_restore of that path; please
> document that.
> 
> > + * Verify that selected cpuidle sysfs files are readable.
> 
> The test also verifies that writing an invalid governor name to
> current_governor is rejected with EINVAL. The description should
> cover both aspects.
> 
> Verdict - Needs revision

The rest is correct.

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

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

[LTP] [PATCH v7] power_management: rewrite runpwtests04.sh in C

[Jinseok Kim]

As part of the ongoing effort to reduce shell-based tests in LTP,
rewrite the cpuidle sysfs smoke test in C using the modern LTP test API.

The new implementation preserves the original test semantics while
removing shell dependencies.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
Changes in v7:
- Replace the array-based test with dedicated helper functions.
- Use TST_EXP_FAIL2() for the invalid governor test.
- Clarify the test description.
- Link to v6: https://lore.kernel.org/ltp/20260707150920.6489-1-always.starving0@gmail.com

Changes in v6:
- Expand test coverage to include 4 core cpuidle sysfs ABIs.
- Add verification of read() system call return values (bytes read).
- Add negative testing for 'current_governor' to verify EINVAL return value.
- Replace manual access() check with LTP .save_restore.
- Link to v5: https://lore.kernel.org/ltp/20260612171415.988-1-always.starving0@gmail.com

Changes in v5:
- Remove runpwtests04.sh
- Link to v4: https://lore.kernel.org/ltp/20260612122045.14962-1-always.starving0@gmail.com

Changes in v4:
- Fix patch application failure reported by CI.
- Link to v3: https://lore.kernel.org/ltp/20260611145911.3752-1-always.starving0@gmail.com

Changes in v3:
- Replace SAFE_OPEN() with open() to convert ENOENT to TCONF.
- Add a cleanup function.
- Link to v2: https://lore.kernel.org/ltp/20260524154221.2064-1-always.starving0@gmail.com

Changes in v2:
- Update runtest entry
- Clarify commit message
- Link to v1: https://lore.kernel.org/ltp/20260516200015.12689-1-always.starving0@gmail.com
---
 runtest/power_management_tests                |   2 +-
 testcases/kernel/power_management/.gitignore  |   1 +
 testcases/kernel/power_management/cpuidle01.c | 107 ++++++++++++++++++
 .../kernel/power_management/runpwtests04.sh   |  58 ----------
 4 files changed, 109 insertions(+), 59 deletions(-)
 create mode 100644 testcases/kernel/power_management/cpuidle01.c
 delete mode 100755 testcases/kernel/power_management/runpwtests04.sh

diff --git a/runtest/power_management_tests b/runtest/power_management_tests
index 4da57ee72..0f2656b52 100644
--- a/runtest/power_management_tests
+++ b/runtest/power_management_tests
@@ -1,5 +1,5 @@
 #POWER_MANAGEMENT
 high_freq_hwp_cap_cppc high_freq_hwp_cap_cppc
+cpuidle01 cpuidle01
 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..e237df1c8 100644
--- a/testcases/kernel/power_management/.gitignore
+++ b/testcases/kernel/power_management/.gitignore
@@ -1 +1,2 @@
 high_freq_hwp_cap_cppc
+cpuidle01
diff --git a/testcases/kernel/power_management/cpuidle01.c b/testcases/kernel/power_management/cpuidle01.c
new file mode 100644
index 000000000..abbf507a0
--- /dev/null
+++ b/testcases/kernel/power_management/cpuidle01.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2026 Jinseok Kim <always.starving0@gmail.com>
+ */
+
+/*\
+ * Basic cpuidle sysfs smoke test.
+ *
+ * Verify that selected cpuidle sysfs files are readable and contain
+ * non-empty values. Also verify that current_governor rejects an
+ * invalid governor.
+ *
+ * Root privileges are required to write to current_governor and to save
+ * and restore its original value.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "tst_test.h"
+
+#define CPUIDLE_PATH "/sys/devices/system/cpu/cpuidle"
+#define AVAILABLE_GOVERNORS CPUIDLE_PATH "/available_governors"
+#define CURRENT_DRIVER CPUIDLE_PATH "/current_driver"
+#define CURRENT_GOVERNOR CPUIDLE_PATH "/current_governor"
+#define CURRENT_GOVERNOR_RO CPUIDLE_PATH "/current_governor_ro"
+
+static void verify_readable(const char *path)
+{
+	int ret;
+	char buf[32];
+
+	int fd = open(path, O_RDONLY);
+
+	if (fd < 0) {
+		if (errno == ENOENT)
+			tst_res(TCONF, "%s is not available", path);
+		else
+			tst_res(TFAIL | TERRNO, "open(%s) failed", path);
+		return;
+	}
+
+	ret = read(fd, buf, sizeof(buf));
+
+	if (ret < 0)
+		tst_res(TFAIL | TERRNO, "%s read() failed", path);
+	else if (ret == 0)
+		tst_res(TFAIL, "%s read() returned 0", path);
+	else
+		tst_res(TPASS, "%s read() successfully", path);
+
+	SAFE_CLOSE(fd);
+}
+
+static void check_available_governors(void)
+{
+	verify_readable(AVAILABLE_GOVERNORS);
+}
+
+static void check_current_driver(void)
+{
+	verify_readable(CURRENT_DRIVER);
+}
+
+static void check_current_governor_ro(void)
+{
+	verify_readable(CURRENT_GOVERNOR_RO);
+}
+
+static void check_current_governor(void)
+{
+	int fd;
+
+	verify_readable(CURRENT_GOVERNOR);
+
+	fd = open(CURRENT_GOVERNOR, O_WRONLY);
+
+	if (fd < 0) {
+		if (errno == ENOENT)
+			tst_res(TCONF, "%s is not available", CURRENT_GOVERNOR);
+		else
+			tst_res(TFAIL | TERRNO, "open(%s) failed", CURRENT_GOVERNOR);
+		return;
+	}
+
+	TST_EXP_FAIL2(write(fd, "invalid_governor", 16), EINVAL);
+
+	SAFE_CLOSE(fd);
+}
+
+static void run(void)
+{
+	check_available_governors();
+	check_current_driver();
+	check_current_governor();
+	check_current_governor_ro();
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.save_restore = (const struct tst_path_val[]) {
+		{ CPUIDLE_PATH "/current_governor", NULL, TST_SR_TCONF },
+		{}
+	},
+};
diff --git a/testcases/kernel/power_management/runpwtests04.sh b/testcases/kernel/power_management/runpwtests04.sh
deleted file mode 100755
index 6565320d2..000000000
--- a/testcases/kernel/power_management/runpwtests04.sh
+++ /dev/null
@@ -1,58 +0,0 @@
-#! /bin/sh
-#
-# Copyright (c) International Business Machines  Corp., 2001
-# Author: Nageswara R Sastry <nasastry@in.ibm.com>
-#
-# This program is free software;  you can redistribute it and#or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
-# or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
-# for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program;  if not, write to the Free Software Foundation,
-# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-#
-
-export TCID="Power_Management04"
-export TST_TOTAL=1
-
-. test.sh
-. pm_include.sh
-
-check_cpuidle_sysfs_files() {
-	RC=0
-	if [ -d /sys/devices/system/cpu/cpuidle ] ; then
-		for files in current_governor_ro current_driver
-		do
-			cat /sys/devices/system/cpu/cpuidle/${files} \
-				>/dev/null 2>&1
-			if [ $? -ne 0 ] ; then
-				echo "${0}: FAIL: cat ${files}"
-				RC=1
-			fi
-		done
-	fi
-	if [ ${RC} -eq 0 ] ; then
-		echo "${0}: PASS: Checking cpu idle sysfs files"
-	else
-		echo "${0}: FAIL: Checking cpu idle sysfs files"
-	fi
-	return $RC
-}
-
-# Checking test environment
-check_arch
-
-# Checking cpuidle sysfs interface files
-if check_cpuidle_sysfs_files ; then
-	tst_resm TPASS "CPUIDLE sysfs tests passed"
-else
-    tst_resm TFAIL "CPUIDLE sysfs tests failed"
-fi
-
-tst_exit
--
2.43.0

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

Re: [LTP] power_management: rewrite runpwtests04.sh in C

[linuxtestproject.agent]

Hi Jinseok,

On Wed, Jul 29, 2026, Jinseok Kim wrote:
> power_management: rewrite runpwtests04.sh in C

> The new implementation preserves the original test semantics while
> removing shell dependencies.

The C version does more than the shell test: it also reads
available_governors and current_governor and adds an invalid-governor
write-rejection check, whereas runpwtests04.sh only read
current_governor_ro and current_driver. Could the commit message be
reworded to reflect the added coverage rather than "preserves the
original test semantics"?

> +++ b/testcases/kernel/power_management/.gitignore
> @@ -1 +1,2 @@
>  high_freq_hwp_cap_cppc
> +cpuidle01

Should the entries stay sorted? Appending cpuidle01 after
high_freq_hwp_cap_cppc leaves the file out of alphabetical order
(c before h). Leaf-directory entries are also normally anchored with a
leading '/' (e.g. /cpuidle01), though the existing entry is unanchored
too.

> +static struct tst_test test = {
> +	.test_all = run,
> +	.needs_root = 1,
> +	.save_restore = (const struct tst_path_val[]) {
> +		{ CPUIDLE_PATH "/current_governor", NULL, TST_SR_TCONF },
> +		{}
> +	},
> +};

TST_SR_TCONF expands to TST_SR_TCONF_MISSING | TST_SR_TCONF_RO, so
tst_sys_conf_save() ends the whole test with TCONF at setup when
current_governor is missing or read-only. On a system that exposes only
current_governor_ro, the readable checks for current_governor_ro and
current_driver (which the shell test still ran) would be skipped
entirely. Is gating the full test on current_governor intended, or would
TST_SR_SKIP / a per-file check keep the other checks running?

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