| Newsgroups |
gmane.linux.ltp |
| Message-ID |
<[email protected]> |
Hi Cyril,
On Tue, 18 Aug 2026 16:12:41 +0200, Cyril Hrubis wrote:
> [PATCH 01/31] lib: Add tst_sysfs_assert
Most commit bodies in patches 1-20 and 25-31 only restate what the subject
already says. Could they explain why the coverage or API is needed? Patch 1
also has the typo "testscases", and the subjects of patches 14 and 15 do not
say that a test is being added.
--- [PATCH 1/31] ---
> size = read_file(path, buf, sizeof(buf));
> ...
> map = SAFE_CALLOC(max_id / 8 + 1, 1);
> size = read_file(path, buf, sizeof(buf));
> ...
> return parse_list(path, buf, size, count, max_id, map);
Could the list be read once, or could parse_list() receive and enforce the
allocated bitmap size? A dynamic list such as CPU online can gain a higher
ID between these reads, after which parse_list() writes beyond map.
--- [PATCH 6/31] ---
> TST_SYSFS_ASSERT_PARSE_LIST(&count, &max_id, SYS_CPU "/online");
> ...
> while (fgets(line, sizeof(line), f)) {
> if (sscanf(line, "cpu%u ", &cpu) == 1)
> cpu_count++;
> }
Could this retry unless the online list is stable around the /proc/stat
read? CPU hotplug between these independent snapshots produces different
counts even when both interfaces are correct.
--- [PATCH 7/31] ---
> snprintf(sub, sizeof(sub),
> SYS_CPU "/cpu%d/topology/package_cpus_list", cpu);
> TST_SYSFS_ASSERT_LIST_SUBSET(sub, SYS_CPU "/online");
Could package_cpus_list be compared with possible/present instead? Topology
sibling masks can contain offline CPUs, so normal CPU hotplug makes this
assertion fail.
> for (cpu = 0; cpu <= max_id; cpu++)
> check_cpu_topology(cpu, poss_max_id);
Could this iterate the parsed online mask rather than every ID through its
maximum? CPU IDs can have offline holes.
> TST_SYSFS_ASSERT_RANGELL(0, poss_max_id,
> SYS_CPU "/cpu%d/topology/physical_package_id",
> cpu);
Could this accept -1? The generic topology implementation exports that
sentinel when an architecture does not provide a physical package ID.
--- [PATCH 9/31] ---
> static const char *const control_allowed[] = {
> "on", "off", "forceoff", "notsupported", "notimplemented", NULL
> };
Could this also parse the numeric control form? Linux 7.2 returns the active
thread count as a decimal value when partial SMT is enabled.
--- [PATCH 11/31] ---
> while ((ent = SAFE_READDIR(d))) {
> if (strncmp(ent->d_name, "clockevent", 10))
> continue;
>
> nclockevents++;
> check_clockevent(ent->d_name);
> }
> ...
> if (nclockevents <= online_count) {
Could this validate only clockevent entries for online CPUs? Linux 7.2
registers clockeventN for every possible CPU, and current_device can be empty
for an offline CPU. Counting all entries against online CPUs therefore fails
on systems with offline CPUs.
--- [PATCH 12/31] ---
> d = SAFE_OPENDIR(ATA);
Could the class directory be checked before SAFE_OPENDIR()? Without libata,
its absence currently produces TBROK rather than the documented TCONF.
The same ordering appears for /sys/class/hwmon in patch 14,
/sys/class/leds in patch 15, /sys/class/wakeup in patch 16,
/sys/class/rtc in patch 17, and /sys/class/thermal in patch 18. Could those
also return TCONF when the corresponding class is unavailable?
--- [PATCH 13/31] ---
> min_bytes = TST_SYSFS_READ_LI(BDI "/%s/min_bytes", name);
> max_bytes = TST_SYSFS_READ_LI(BDI "/%s/max_bytes", name);
Could these use an unsigned 64-bit parser? Linux 7.2 exports both attributes
as u64, so valid values above LONG_MAX are rejected or misparsed, especially
on 32-bit systems.
--- [PATCH 14/31] ---
> TST_SYSFS_ASSERT_RANGELL(0, 1000000, HWMON "/%s/temp%d_input",
> hwmon, nr);
> ...
> if (min > max)
> tst_res(TFAIL, "temp%d_min (%ld) > temp%d_max (%ld)",
> nr, min, nr, max);
What kernel ABI guarantees these plausibility ranges and threshold
orderings? Hwmon values and writable thresholds are device-specific, and
the kernel does not enforce these policies. Valid hardware or configuration
can therefore fail the test.
--- [PATCH 16/31] ---
> TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX, WAKEUP "/%s/%s",
> name, counters[i]);
Could these counters use an unsigned-long parser? Linux 7.2 exports them with
"%lu", so a valid counter above LONG_MAX false-fails on 32-bit systems.
--- [PATCH 17/31] ---
> if (hctosys)
> check_system_time(rtc);
> ...
> if (!hctosys_found)
> check_system_time("rtc0");
Could this comparison be removed or made informational? hctosys only records
that an RTC initialized system time at boot. NTP can subsequently correct
system time without updating the RTC, and rtc0 is not implicitly
synchronized when no hctosys attribute is set.
> TST_SYSFS_READ_STR(date, sizeof(date), RTC "/%s/date", rtc);
> TST_SYSFS_READ_STR(time, sizeof(time), RTC "/%s/time", rtc);
Could this use RTC_RD_TIME or verify matching date reads around the time
read? A midnight rollover between these files combines the previous date
with the next day's time and creates a false failure of about 24 hours.
--- [PATCH 19/31] ---
> /*
> * Change the link-layer (MAC) address of an existing network device. Most
> * drivers require the device to be administratively down for this to
> * succeed.
> */
> int tst_netdev_set_hwaddr(const char *file, const int lineno, int strict,
> const char *ifname, const void *addr, size_t addrlen);
Could the two new public APIs and their macros use kernel-doc, including
parameter documentation, so they are included in the generated C API
reference?
--- [PATCH 20/31] ---
> * - carrier is a boolean (0 or 1) when readable
> *
> * carrier returns an error (EINVAL) when the interface is administratively
> * down, which the test tolerates.
Could the promised carrier check be implemented, or could this claim be
removed? check_iface() currently validates only type, MTU, addr_len, address,
and operstate.
--- [PATCH 23/31] ---
> SAFE_CLOSE(fd);
> read_state(state);
> assert_state("down", 0, 0, state);
> ...
> fd = open_tun();
> read_state(state);
> assert_state("up", 1, 1, state);
Could this poll for the expected operstate with a timeout? TUN updates
carrier synchronously, but netdev_state_change() schedules operstate updates
through linkwatch. These immediate reads can still observe the previous
operstate.
--- [PATCH 24/31] ---
> if (attached) {
> tst_res(TINFO, "Autoclear did not detach the loop device");
> tst_detach_device(loopdev);
> attached = 0;
> }
Could attached be cleared only when the fallback detach succeeds? If
tst_detach_device() fails, cleanup() skips the device and the system-wide
loop attachment is leaked.
--- [PATCH 25/31] ---
> TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX,
> QUEUE "/%s/discard_max_bytes", dev);
Could discard_max_bytes use an unsigned 64-bit parser and range? The block
queue ABI exports an unsigned 64-bit byte count, which can exceed LONG_MAX
on 32-bit systems.
> static const char *const schedulers[] = {
> "none", "mq-deadline", "kyber", "bfq", NULL
> };
Could the scheduler check validate only the bracketed single-selection
format? Elevators are registered dynamically, so vendor or future scheduler
names outside this fixed list are valid.
--- [PATCH 26/31] ---
> if (sscanf(line, "%u %u %*u %*s", &proc_major, &proc_minor) != 4)
> continue;
Could this compare the return value with 2? Assignment-suppressed conversions
do not count, so every valid /proc/partitions line is currently skipped and
the size comparison never runs.
--- [PATCH 27/31] ---
> nr = TST_SYSFS_READ_LI(HUGEPAGES "/%s/nr_hugepages", name);
> free = TST_SYSFS_READ_LI(HUGEPAGES "/%s/free_hugepages", name);
> resv = TST_SYSFS_READ_LI(HUGEPAGES "/%s/resv_hugepages", name);
> surp = TST_SYSFS_READ_LI(HUGEPAGES "/%s/surplus_hugepages", name);
Could these checks retry unless surrounding pool counters are stable?
Allocation, reservation, or pool resizing between the separate reads can
violate the asserted relationships even though every individual kernel
snapshot is consistent.
--- [PATCH 28/31] ---
> proc_val = tst_read_meminfo(proc_name);
> sys_val = TST_SYSFS_READ_LI(HUGEPAGES "/%s", sys_name);
> TST_EXP_EQ_LI(proc_val, sys_val);
Could this compare stable snapshots or retry on a concurrent change?
Hugepage allocation, reservation, and pool resizing can occur between the
/proc and sysfs reads, producing a false mismatch.
--- [PATCH 29/31] ---
> for (i = 0; i < ARRAY_SIZE(nonneg_counters); i++) {
> TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX, KSM "/%s",
> nonneg_counters[i]);
> }
Could pages_to_scan and the page counters use parsers matching their unsigned
kernel types? Linux 7.2 exports pages_to_scan as unsigned int and the counters
as unsigned long, so valid values above LONG_MAX false-fail on 32-bit systems.
--- [PATCH 30/31] ---
> +sys_swap01 sys_swap01
> +++ b/testcases/kernel/sysfs/kernel/mm/swap/sys_mm_swap01.c
Could the source/binary and runtest names be made consistent? This builds
sys_mm_swap01, while runtest invokes the nonexistent sys_swap01.
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