Re: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> new file mode 100644
> index 000000000000..17a38cd03845
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c

[ ... ]

> +/* Load rc-loopback and find the lirc and input chardevs it created. */
> +static bool find_loopback_devices(char *lirc_path, char *input_path,
> +				  size_t path_sz)
> +{
> +	glob_t gl = {};
> +	bool found = false;
> +
> +	/* Ignore failure, we check for the resulting devices below. */
> +	system("modprobe rc-loopback > /dev/null 2>&1");

A subsystem pattern flags this as potentially concerning: this hand-rolls
a fire-and-forget shell invocation when test_progs.h already provides
SYS_NOFAIL(fmt, ...) at test_progs.h:444, which handles both the ignored
return value and the redirect to /dev/null. The line would become just
SYS_NOFAIL("modprobe rc-loopback"). SYS_NOFAIL() is used 119 times across
prog_tests/ for exactly this pattern.

The subsystem guide flags tests that reimplement existing helpers instead
of using them, though in this case raw system() is also widespread in
prog_tests/ so the behaviour is identical either way. Worth considering
for consistency?

> +
> +	if (glob("/sys/class/rc/rc*", 0, NULL, &gl)) {
> +		fprintf(stderr, "No rc devices found, is CONFIG_RC_LOOPBACK enabled?\n");
> +		return false;
> +	}
> +
> +	for (size_t i = 0; i < gl.gl_pathc; i++) {
[ ... ]
> +		if (!strstr(uevent, "DRV_NAME=rc-loopback"))
> +			continue;
> +
> +		snprintf(pattern, sizeof(pattern), "%s/lirc*/uevent", rcdir);
> +		if (!find_devname(pattern, lirc_path, path_sz))
> +			continue;
> +
> +		snprintf(pattern, sizeof(pattern), "%s/input*/event*/uevent", rcdir);
> +		if (!find_devname(pattern, input_path, path_sz))
> +			continue;
> +
> +		found = true;
> +		break;
> +	}
> +
> +	globfree(&gl);
> +	return found;
> +}

A subsystem pattern flags this as potentially concerning: the only
diagnostic is attached to the wrong condition, so the common unconfigured
case skips with no explanation.

glob("/sys/class/rc/rc*") fails only when there is no rc device at all
(CONFIG_RC_CORE=n or no IR hardware), yet that's where the message names
CONFIG_RC_LOOPBACK. The actual CONFIG_RC_LOOPBACK=n case (rc devices
exist, but none has DRV_NAME=rc-loopback) and the CONFIG_LIRC=n case (no
lirc*/uevent under the loopback rc device) both fall through the loop and
reach the silent 'return found' with found=false.

test_lirc_mode2() then calls test__skip() with no message (test__skip()
takes no argument), so the run reports lirc_mode2:SKIP with nothing
telling the reader which prerequisite was missing.

> +
> +void test_lirc_mode2(void)
> +{
> +	char lirc_path[PATH_MAX], input_path[PATH_MAX];
> +	int lircfd = -1, inputfd = -1, progfd, progfd2 = -1;
> +	struct lirc_mode2 *skel = NULL, *skel2 = NULL;
[ ... ]
> +
> +	if (getuid() != 0) {
> +		test__skip();
> +		return;
> +	}
> +
> +	if (!find_loopback_devices(lirc_path, input_path, sizeof(lirc_path))) {
> +		test__skip();
> +		return;
> +	}

A subsystem pattern flags this as potentially concerning: both skip paths
call bare test__skip() with no accompanying message, so the two very
different reasons for skipping are indistinguishable in the test output.

The first skips because the test is not running as root; the second skips
because find_loopback_devices() could not find an rc-loopback lirc/input
device pair (which can mean CONFIG_RC_LOOPBACK is not built, or modprobe
rc-loopback failed, or the sysfs walk did not match). A user seeing
lirc_mode2:SKIP has no way to tell which precondition was missing.

That said, bare test__skip() with no message is the overwhelmingly
dominant idiom in this directory (126 call sites use it with no message),
and test__skip() accepts no message parameter, so there is no in-framework
way to attach a reason. By the 'compare to the neighbours' rule this would
normally be consistent with established practice.

> diff --git a/tools/testing/selftests/bpf/progs/lirc_mode2.c b/tools/testing/selftests/bpf/progs/lirc_mode2.c
> new file mode 100644
> index 000000000000..98137f3c5c03
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/lirc_mode2.c

[ ... ]

> +SEC("lirc_mode2")
> +int bpf_decoder(unsigned int *sample)
> +{
> +	if (LIRC_IS_PULSE(*sample)) {
> +		unsigned int duration = LIRC_VALUE(*sample);
> +
> +		/*
> +		 * Flag bits picked deliberately low: rc-loopback simulates
> +		 * a receiver overflow for any pulse over MS_TO_US(50) (see
> +		 * loop_tx_ir() in rc-loopback.c), which would silently
> +		 * swallow the sample before it ever reaches this decoder.
> +		 */
> +		if (duration & 0x8000)
> +			bpf_rc_keydown(sample, 0x40, duration & 0x3fff, 0);
> +		if (duration & 0x4000)
> +			bpf_rc_pointer_rel(sample, (duration >> 7) & 0x7f,
> +					   duration & 0x7f);
> +	}
> +
> +	return 0;
> +}

Does the comment match what the code does? Looking at the deleted
progs/test_lirc_mode2_kern.c, the old decoder used flag bits 0x1000/0x2000
with masks 0xffff, >>8 & 0xff, and & 0xff. The new code uses 0x8000/0x4000
with masks 0x3fff, >>7 & 0x7f, and & 0x7f.

The comment claims the flag bits were "picked deliberately low" to stay
under rc-loopback's MS_TO_US(50) overflow threshold, but the bits were
moved UP: the largest encodable sample went from 0x3FFF (16383 us, ample
margin under the 50000 us threshold) to 0xBFFF (49151 us, 849 us of
margin), and with both flags set, 0xFFFF (65535 us) exceeds the threshold
and would be discarded by ir_raw_event_overflow() rather than decoded.

The two committed test vectors (0x8ead and 0x4081) are both safe, so the
test passes today. The concern is future-facing: the comment actively
misleads anyone adding a third vector into believing high-bit values are
the safe choice, when the new layout leaves only 849 us of headroom and
any value with both flag bits set is silently swallowed by the loopback
overflow simulation.

The new bit layout is genuinely cleaner than the old one (with flags at
0x1000/0x2000 and rel_x extracted as (duration >> 8) & 0xff, the 0x2000
flag bit landed inside rel_x, so flags and payload overlapped). Moving
flags to the top two bits and shrinking the payload to 14 bits removes
that overlap. Could the comment be updated to match what the code actually
does, and perhaps mention the 50 ms ceiling on encodable samples?

Also, the commit message enumerates five coverage additions but does not
mention the format change at all.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32401351183