Re: [PATCH v12 10/11] selftests/riscv: add SSE test module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] selftests/riscv: sse: deadlock between migrate_disable() and cpus_read_lock()
- [Medium] selftests/riscv: sse: fast test ignores errors and races on 1-jiffy timeout
- [Medium] selftests/riscv: sse: make install fails when riscv_sse_test.ko is not built
- [Medium] selftests/riscv: sse: run_sse_test.sh added without executable permissions
- [Medium] selftests/riscv: sse: stress test treats -EEXIST as failure instead of skip
- [Low] selftests/riscv: sse: swapped event ID arguments in sse_test_handler()
--

commit f2ce096e05cb9e91fc9386faf3d3f4d77029d337
Author: Clément Léger <cleger@rivosinc.com>

selftests/riscv: add SSE test module

This commit adds a RISC-V SSE selftest kernel module and a shell runner
script to exercise event injection, priority ordering, and stress scenarios
with SBI calls and self re-injection.

> diff --git a/tools/testing/selftests/riscv/sse/Makefile b/tools/testing/selftests/riscv/sse/Makefile
> new file mode 100644
> index 0000000000000..7e2677fdce09a
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/Makefile
> @@ -0,0 +1,5 @@
> +TEST_GEN_MODS_DIR := module
> +
> +TEST_PROGS := run_sse_test.sh
> +
> +include ../../lib.mk
> diff --git a/tools/testing/selftests/riscv/sse/module/Makefile b/tools/testing/selftests/riscv/sse/module/Makefile
> new file mode 100644
> index 0000000000000..eac4b1c6228bb
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/module/Makefile
> @@ -0,0 +1,22 @@
> +ifneq ($(CONFIG_RISCV_SBI_SSE),)
> +obj-m += riscv_sse_test.o
> +endif
> +
> +ifndef KERNELRELEASE
> +
> +TESTMODS_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
> +KDIR ?= /lib/modules/$(shell uname -r)/build
> +
> +# Ensure that KDIR exists, otherwise skip the compilation
> +modules:
> +ifneq ("$(wildcard $(KDIR))", "")
> +	$(Q)$(MAKE) -C $(KDIR) modules KBUILD_EXTMOD=$(TESTMODS_DIR)
> +endif

[Severity: Medium]
Does make install fail for the riscv selftests when riscv_sse_test.ko is not
built?

In tools/testing/selftests/riscv/sse/Makefile, TEST_GEN_MODS_DIR is set
unconditionally to module. However, in
tools/testing/selftests/riscv/sse/module/Makefile, building riscv_sse_test.o
is skipped when CONFIG_RISCV_SBI_SSE is disabled or when KDIR (which defaults
to the build host's /lib/modules/$(shell uname -r)/build) does not exist.

When no .ko is produced in module/, INSTALL_MODS_RULE in
tools/testing/selftests/lib.mk runs:

    rsync -a --copy-unsafe-links module/*.ko $(INSTALL_PATH)/module

which fails with exit status 23 because module/*.ko matches no files. Since
sse is the final target in RISCV_SUBTARGETS in
tools/testing/selftests/riscv/Makefile, this causes the top-level riscv
selftests install target to fail.

[ ... ]

> diff --git a/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c b/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c
> new file mode 100644
> index 0000000000000..cc5c2e46f2fdb
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c

[ ... ]

> +static int sse_test_wait_not_running_on_cpu(u32 evt, unsigned int cpu)
> +{
> +	struct sse_test_wait_status wait = { .evt = evt };
> +	unsigned long timeout;
> +	int ret = 0;
> +
> +	if (sse_event_is_global(evt))
> +		return sse_test_wait_not_running(evt);
> +
> +	wait.attr_buf = kmalloc_obj(*wait.attr_buf, GFP_KERNEL);
> +	if (!wait.attr_buf)
> +		return -ENOMEM;
> +
> +	timeout = jiffies + HZ;
> +	do {
> +		ret = smp_call_function_single(cpu, sse_test_read_status_local,
> +					       &wait, true);
> +		if (ret || wait.ret) {
> +			ret = ret ?: wait.ret;
> +			break;
> +		}
> +		if (wait.status != SBI_SSE_STATE_RUNNING)
> +			break;
> +		usleep_range(100, 200);
> +	} while (time_before(jiffies, timeout));
> +
> +	if (!ret && wait.status == SBI_SSE_STATE_RUNNING) {
> +		sse_err("Timed out waiting for event %x on CPU %u\n", evt, cpu);
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	kfree(wait.attr_buf);
> +
> +	return ret;
> +}

[ ... ]

> +static int sse_test_handler(u32 evt, void *arg, struct pt_regs *regs)
> +{
> +	int ret = 0;
> +	struct fast_test_arg *targ = arg;
> +	u32 test_evt;
> +	int cpu;
> +
> +	atomic_inc(&sse_test_handler_count);
> +
> +	/* Pairs with the argument publication in sse_run_fast_test_cpu(). */
> +	if (!smp_load_acquire(&targ->args_ready)) {
> +		sse_err("Received SSE event %x before its test arguments were published\n",
> +			evt);
> +		ret = -EINVAL;
> +		goto complete;
> +	}
> +
> +	test_evt = READ_ONCE(targ->evt);
> +	cpu = READ_ONCE(targ->cpu);
> +
> +	if (evt != test_evt) {
> +		sse_err("Received SSE event id %x instead of %x\n", test_evt, evt);
> +		ret = -EINVAL;
> +	}

[Severity: Low]
Are test_evt and evt swapped in the sse_err() format arguments in
sse_test_handler()? evt is the received event ID and test_evt is the
expected one.

> +
> +	if (!sse_event_is_global(evt) && cpu != smp_processor_id()) {
> +		sse_err("Received SSE event %d on CPU %d instead of %d\n", evt, smp_processor_id(),
> +			cpu);
> +		ret = -EINVAL;
> +	}
> +
> +complete:
> +	WRITE_ONCE(targ->args_ready, false);
> +	/* Publish handler-side checks before waking the waiting CPU. */
> +	smp_store_release(&targ->completion, true);
> +
> +	return ret;
> +}
> +
> +static int sse_run_fast_test_cpu(struct fast_test_arg *test_arg,
> +				 struct sse_event *event, u32 evt, int cpu)
> +{
> +	unsigned long timeout;
> +	int ret;
> +
> +	WRITE_ONCE(test_arg->completion, false);
> +	WRITE_ONCE(test_arg->args_ready, false);
> +	WRITE_ONCE(test_arg->evt, evt);
> +	WRITE_ONCE(test_arg->cpu, cpu);
> +	/* Publish all arguments before firmware can inject on another hart. */
> +	smp_store_release(&test_arg->args_ready, true);
> +
> +	ret = sse_test_inject_event(event, evt, cpu);
> +	if (ret) {
> +		sse_err("event %s injection failed, err %d\n",
> +			sse_evt_name(evt), ret);
> +		return ret;
> +	}
> +
> +	timeout = jiffies + HZ / 100;
> +	/* We can not use <linux/completion.h> since they are not NMI safe */
> +	/* Pairs with the handler's completion publication. */
> +	while (!smp_load_acquire(&test_arg->completion) &&
> +	       time_before(jiffies, timeout))
> +		cpu_relax();
> +	/* Acquire the handler's checks even if the loop observed a timeout. */
> +	if (!smp_load_acquire(&test_arg->completion)) {
> +		sse_err("Failed to wait for event %s completion on CPU %d\n",
> +			sse_evt_name(evt), cpu);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return sse_test_wait_not_running_on_cpu(evt, cpu);
> +}
> +
> +static void sse_run_fast_test(struct fast_test_arg *test_arg,
> +			      struct sse_event *event, u32 evt)
> +{
> +	int cpu;
> +
> +	if (sse_event_is_global(evt)) {
> +		/* Keep the selected target online through injection and completion. */
> +		cpu_hotplug_disable();
> +		for_each_online_cpu(cpu) {
> +			if (sse_run_fast_test_cpu(test_arg, event, evt, cpu))
> +				break;
> +		}
> +		cpu_hotplug_enable();
> +		return;
> +	}
> +
> +	guard(cpus_read_lock)();
> +	for_each_online_cpu(cpu) {
> +		if (sse_run_fast_test_cpu(test_arg, event, evt, cpu))
> +			return;
> +	}
> +}
> +
> +static void sse_test_injection_fast(void)
> +{
> +	int i, ret = 0, j;
> +	u32 evt;
> +	struct sse_event *event;

[ ... ]

> +		for (j = 0; j < RUN_LOOP_COUNT; j++)
> +			sse_run_fast_test(&fast_test_arg, event, evt);
> +		pr_info("Finished testing event %s\n", sse_evt_name(evt));

[Severity: Medium]
What happens in sse_test_injection_fast() when sse_run_fast_test_cpu()
times out or returns an error?

In sse_run_fast_test_cpu(), timeout = jiffies + HZ / 100 is only 1 jiffy on
CONFIG_HZ_100=y kernels (which can expire on the very next timer tick) and
2 jiffies on CONFIG_HZ_250=y kernels. If this wait times out,
sse_run_fast_test_cpu() returns -ETIMEDOUT without calling
sse_test_wait_not_running_on_cpu(evt, cpu) to wait for the event to leave
SBI_SSE_STATE_RUNNING.

Because sse_run_fast_test() returns void, sse_test_injection_fast() continues
running all RUN_LOOP_COUNT (1000) iterations even after a failure. On the
next iteration, sse_run_fast_test_cpu() immediately rewrites the shared
static fast_test_arg while the timed-out handler on the target CPU may still
be executing and accessing fast_test_arg. If sse_test_wait_not_running()
times out (1 second per iteration), running all 1000 iterations can also
stall module load for up to 1000 seconds.

Also, if sse_test_wait_not_running_on_cpu() fails with a non-timeout error
(such as -ENOMEM or an error from smp_call_function_single() or
sse_event_attr_read()), neither sse_test_wait_not_running_on_cpu() nor
sse_run_fast_test_cpu() calls sse_err(), so the error will not be logged as
a failure in dmesg. Should sse_run_fast_test() propagate errors to break out
of the RUN_LOOP_COUNT loop, use msecs_to_jiffies() for the completion
timeout, and ensure non-timeout errors are reported?

[ ... ]

> +static void sse_test_injection_priority_arg(struct priority_test_arg *args, unsigned int args_size,
> +					    sse_event_handler_fn handler, const char *test_name)
> +{
> +	unsigned int i;
> +	unsigned long timeout;
> +	int ret;
> +	int target_cpu;
> +	struct sse_event *event;
> +	struct priority_test_arg *arg, *first_arg = NULL, *prev_arg = NULL;
> +
> +	pr_info("Starting SSE priority test (%s)\n", test_name);
> +	/* Keep the complete priority chain on one CPU. */
> +	migrate_disable();
> +	target_cpu = smp_processor_id();
> +
> +	for (i = 0; i < args_size; i++) {
> +		arg = &args[i];
> +
> +		if (!sse_test_can_inject_event(arg->evt))
> +			continue;
> +
> +		WRITE_ONCE(arg->called, false);
> +		WRITE_ONCE(arg->next_evt_arg, NULL);
> +		WRITE_ONCE(arg->event, NULL);
> +		WRITE_ONCE(arg->enable_attempted, false);
> +
> +		event = sse_test_event_register(arg->evt, arg->prio, handler,
> +						(void *)arg);

[ ... ]

> +release_events:
> +
> +	for (i = 0; i < args_size; i++) {
> +		arg = &args[i];
> +		event = arg->event;
> +		if (!event)
> +			continue;
> +
> +		ret = sse_test_wait_not_running_on_cpu(arg->evt, target_cpu);
> +		if (ret)
> +			sse_err("Event %s did not complete, err %d\n",
> +				sse_evt_name(arg->evt), ret);
> +
> +		if (arg->enable_attempted) {
> +			ret = sse_event_disable(event);
> +			if (ret) {
> +				sse_err("Failed to disable event %s, err %d\n",
> +					sse_evt_name(arg->evt), ret);
> +				sse_test_queue_cleanup(event);
> +				WRITE_ONCE(arg->event, NULL);
> +				WRITE_ONCE(arg->enable_attempted, false);
> +				continue;
> +			}
> +		}
> +
> +		ret = sse_test_event_unregister(event);
> +		if (ret) {
> +			sse_err("Failed to unregister event %s, err %d\n",
> +				sse_evt_name(arg->evt), ret);
> +			continue;
> +		}
> +
> +		WRITE_ONCE(arg->event, NULL);
> +		WRITE_ONCE(arg->enable_attempted, false);
> +	}
> +
> +	pr_info("Finished SSE priority test (%s)\n", test_name);
> +out:
> +	migrate_enable();
> +}

[Severity: High]
Can calling sse_event_register(), sse_event_set_target_cpu(),
sse_event_enable(), sse_event_disable(), and sse_event_unregister() inside a
migrate_disable() section in sse_test_injection_priority_arg() deadlock with
CPU hotplug?

In drivers/firmware/riscv/riscv_sbi_sse.c, each of those SSE APIs acquires
sse_mutex and cpus_read_lock():

    scoped_guard(mutex, &sse_mutex) {
        ...
        scoped_guard(cpus_read_lock) {
            ...
        }
    }

In sse_test_injection_priority_arg(), migrate_disable() pins the task to
target_cpu without holding cpus_read_lock() or cpu_hotplug_disable() across
the test. If a concurrent _cpu_down(target_cpu) starts between two SSE API
calls:

1. The hotplug thread acquires cpus_write_lock(), runs sse_cpu_teardown() on
   target_cpu (disabling/unregistering local events and migrating global
   events off target_cpu), and then waits in sched_cpu_wait_empty() ->
   balance_hotplug_wait() for !rq_has_pinned_tasks(rq) on target_cpu.

2. In sse_test_injection_priority_arg(), the next call to sse_event_*() tries
   to acquire cpus_read_lock() while still holding migrate_disable() on
   target_cpu, blocking behind cpus_write_lock() while keeping
   rq->nr_pinned > 0.

Should sse_test_injection_priority_arg() acquire cpu_hotplug_disable()
before migrate_disable() (and release it after migrate_enable()) so that
target_cpu cannot be offlined while the task is pinned and acquiring
cpus_read_lock()?

[ ... ]

> +static int sse_stress_test_layer(int layer)
> +{
> +	struct sse_event *event;
> +	int inject_count, handler_count;
> +	int ret, target_cpu, unregister_ret;

[ ... ]

> +	event = sse_test_event_register(stress_ctx.evt_id, 0,
> +					stress_handlers[layer], &stress_ctx);
> +	if (IS_ERR(event)) {
> +		sse_err("Failed to register stress event, err %ld\n",
> +			PTR_ERR(event));
> +		return PTR_ERR(event);
> +	}

[ ... ]

> +static void sse_stress_test_all_layers(void)
> +{
> +	int i, ret;
> +
> +	pr_info("Starting SSE stress tests: duration=%d ms, interval=%d ns\n",
> +		STRESS_DURATION_MS, STRESS_INJECT_NS);
> +
> +	for (i = 0; i < ARRAY_SIZE(stress_handlers); i++) {
> +		ret = sse_stress_test_layer(i);
> +		if (ret) {
> +			sse_err("Stress layer %d failed, err %d\n", i, ret);
> +			break;
> +		}
> +
> +		msleep(100);
> +	}
> +
> +	pr_info("Finished SSE stress tests\n");
> +}

[Severity: Medium]
Should sse_stress_test_layer() skip rather than fail when
sse_test_event_register() returns -EEXIST?

The commit message notes that if firmware reports injectable events but the
test cannot acquire or handle any of them, it should report SKIP instead of
passing or failing. Both sse_test_injection_fast() and
sse_test_injection_priority_arg() check for PTR_ERR(event) == -EEXIST and
skip with pr_info().

However, in sse_stress_test_layer(), -EEXIST is logged with sse_err() (which
emits the FAILED: prefix) and returned to sse_stress_test_all_layers(),
which logs another sse_err(). Because run_sse_test.sh checks for FAILED:
before SKIP:, an already-registered local software-injected event causes the
test run to fail instead of skipping.

[ ... ]

> diff --git a/tools/testing/selftests/riscv/sse/run_sse_test.sh b/tools/testing/selftests/riscv/sse/run_sse_test.sh
> new file mode 100644
> index 0000000000000..e70a2fd14b05e
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/run_sse_test.sh
> @@ -0,0 +1,59 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

[Severity: Medium]
Should tools/testing/selftests/riscv/sse/run_sse_test.sh be committed with
mode 100755 instead of 100644?

Because run_sse_test.sh is listed in TEST_PROGS without executable
permissions, running it directly fails with permission denied, and
tools/testing/selftests/kselftest/runner.sh (in run_one()) warns that the
file is not executable and falls back to invoking the interpreter without
forwarding $kselftest_cmd_args to the script.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029074853.35467-1-zhangzhanpeng.jasper@bytedance.com?part=10