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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] A stray `?` character before a closing brace causes a compilation error.
- [Low] Multi-line block comments use C++ style `//` instead of the required kernel `/* ... */` style.
- [Low] The printed error message for missing rc devices incorrectly repeats `CONFIG_RC_LOOPBACK`.
--

--- Patch [1]: [PATCH v2] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage ---
commit b838818703c3bdf7a467c66079361ca289b5a98e
Author: Sean Young <[email protected]>

selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

This patch ports the lirc_mode2 test to the prog_tests framework and extends
its coverage. It tests attaching, querying, and detaching BPF programs to
and from a lirc chardev.

> 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 0000000000000..8e485db70b78a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// test ir decoder
> +//
> +// Copyright (C) 2018 Sean Young <[email protected]>
> +
> +// A lirc chardev is a device representing a consumer IR (cir) device which
> +// can receive infrared signals from remote control and/or transmit IR.
> +//
> +// IR is sent as a series of pulses and space somewhat like morse code. The
> +// BPF program can decode this into scancodes so that rc-core can translate
> +// this into input key codes using the rc keymap.
> +//
> +// This test works by sending IR over rc-loopback, so the IR is processed by
> +// BPF and then decoded into scancodes. The lirc chardev must be the one
> +// associated with rc-loopback, see the output of ir-keytable(1).
> +//
> +// The following CONFIG options must be enabled for the test to succeed:
> +// CONFIG_RC_CORE=y
> +// CONFIG_BPF_RAWIR_EVENT=y
> +// CONFIG_RC_LOOPBACK=y
> +// CONFIG_LIRC=y

[Severity: Low]
Is it possible to use standard C-style multi-line comments (/* ... */) here
instead of C++ style comments? The BPF subsystem guidelines prefer the standard
kernel multi-line comment style.

[ ... ]

> +out:
> +	if (!found) {
> +		fprintf(stderr, "No rc devices found\n");
> +		fprintf(stderr, "Enable CONFIG_RC_LOOPBACK, CONFIG_RC_LOOPBACK, and CONFIG_LIRC\n");

[Severity: Low]
This isn't a bug, but there's a typo here where CONFIG_RC_LOOPBACK is listed
twice in the error message.

> +?	}

[Severity: Low]
Is this stray '?' character intended? It appears this will cause a compilation
error when building the selftests.

> +
> +	globfree(&gl);
> +	return found;
> +}

[ ... ]

> 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 0000000000000..98137f3c5c03d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/lirc_mode2.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// test ir decoder
> +//
> +// Copyright (C) 2018 Sean Young <[email protected]>

[Severity: Low]
Similarly here, should this be converted to a standard C-style multi-line
comment block?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1