Re: [PATCH v7 2/8] riscv: Add breakpoint and watchpoint test for riscv

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/breakpoints: Makefile fails to match riscv64 native architecture
- [Low] selftests/breakpoints: global CFLAGS += -static affects other tests
- [Low] selftests/breakpoints: file descriptor leak in trigger_wp()
- [Low] selftests/breakpoints: shared variables lack volatile sig_atomic_t qualifier
--

commit d5a08206a858d8d5760badeff5fe2c51c8a98f2b
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Subject: riscv: Add breakpoint and watchpoint test for riscv

This commit adds a selftest for the RISC-V architecture using the ptrace
framework. It sets and unsets breakpoints and watchpoints, and uses signals 
to verify the triggers.

> diff --git a/tools/testing/selftests/breakpoints/Makefile b/tools/testing/selftests/breakpoints/Makefile
> index 0b8f5acf7c78f..b32c56b3c1db2 100644
> --- a/tools/testing/selftests/breakpoints/Makefile
> +++ b/tools/testing/selftests/breakpoints/Makefile
> @@ -12,5 +12,10 @@ ifneq (,$(filter $(ARCH),aarch64 arm64))
>  TEST_GEN_PROGS += breakpoint_test_arm64
>  endif
>  
> +ifneq (,$(filter $(ARCH),riscv))

[Severity: Medium]
Will this test be silently skipped during a native build on a RISC-V machine?

The native architecture reported by uname -m is "riscv64", but this filter
expects exactly "riscv". Since this leaf Makefile doesn't include the
kernel's subarch.include to normalize the architecture name, it looks like it
might fail to match the native architecture.

> +CFLAGS += -static $(KHDR_INCLUDES)

[Severity: Low]
Does appending to the global CFLAGS here force other unrelated tests in this
directory to inadvertently build statically on RISC-V?

Using a target-specific flag just for breakpoint_test_riscv might prevent
impacting tests like step_after_suspend_test.

> +TEST_GEN_PROGS += breakpoint_test_riscv
> +endif
> +
>  include ../lib.mk

[ ... ]

> diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> new file mode 100644
> index 0000000000000..0649940b709e0
> --- /dev/null
> +++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> @@ -0,0 +1,219 @@

[ ... ]

> +#ifndef noinline
> +#define noinline __attribute__((noinline))
> +#endif
> +
> +static int gfd;
> +sem_t ib_mtx, wp_mtx;
> +static int bp_triggered, wp_triggered;

[Severity: Low]
Can the compiler optimize and incorrectly cache these variables since they
are modified asynchronously in a signal handler but lack the volatile
sig_atomic_t qualifiers?

They are incremented in sig_handler_bp() and sig_handler_wp(), so standard
POSIX signal safety might require the proper qualifiers to avoid potential
hangs or incorrect test failures.

> +static int test_func_sink;
> +static const int wait_timeout_sec = 5;

[ ... ]

> +static int trigger_wp(void)
> +{
> +	struct sigaction sa;
> +	unsigned long test_data;

[ ... ]

> +	if (gfd < 0) {
> +		printf("Failed to setup watchpoint\n");
> +		return -1;
> +	}
> +
> +	ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
> +	test_data = 0xdeadbeef;
> +	ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
> +
> +	return 0;
> +}

[Severity: Low]
Does this success path leak the file descriptor allocated by setup_bp()?

It looks like close(gfd) is correctly called on the success path in
trigger_bp(), but appears to be missing here before returning.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930063919.608911-1-himanshu.chauhan@oss.qualcomm.com?part=2