On Fri, Mar 14, 2025 at 12:10:29PM +0100, Clément Léger wrote:
...
> +static void sse_test_inject_local(uint32_t event_id)
> +{
> + int cpu;
> + uint64_t timeout;
> + struct sbiret ret;
> + struct sse_local_per_cpu *cpu_args, *cpu_arg;
> + struct sse_foreign_cpu_test_arg *handler_arg;
> +
> + cpu_args = calloc(NR_CPUS, sizeof(struct sbi_sse_handler_arg));
> +
> + report_prefix_push("local_dispatch");
> + for_each_online_cpu(cpu) {
> + cpu_arg = &cpu_args[cpu];
> + cpu_arg->handler_arg.event_id = event_id;
> + cpu_arg->args.stack = sse_alloc_stack();
> + cpu_arg->args.handler = sse_foreign_cpu_handler;
> + cpu_arg->args.handler_data = (void *)&cpu_arg->handler_arg;
> + cpu_arg->state = SBI_SSE_STATE_UNUSED;
> + }
> +
> + on_cpus(sse_register_enable_local, cpu_args);
> + for_each_online_cpu(cpu) {
> + cpu_arg = &cpu_args[cpu];
> + ret = cpu_arg->ret;
> + if (ret.error) {
> + report_fail("CPU failed to register/enable event: %ld", ret.error);
> + goto cleanup;
> + }
> +
> + handler_arg = &cpu_arg->handler_arg;
> + WRITE_ONCE(handler_arg->expected_cpu, cpu);
> + /* For handler_arg content to be visible for other CPUs */
> + smp_wmb();
> + ret = sbi_sse_inject(event_id, cpus[cpu].hartid);
> + if (ret.error) {
> + report_fail("CPU failed to inject event: %ld", ret.error);
> + goto cleanup;
> + }
> + }
> +
> + for_each_online_cpu(cpu) {
> + handler_arg = &cpu_args[cpu].handler_arg;
> + smp_rmb();
> +
> + timeout = sse_event_get_complete_timeout();
> + while (!READ_ONCE(handler_arg->done) || timer_get_cycles() < timeout) {
I pointed this out in the last review, the || should be a &&. We don't
want to keep waiting until we reach the timeout if we get the done signal
earlier.
> + /* For handler_arg update to be visible */
> + smp_rmb();
> + cpu_relax();
> + }
> + report(READ_ONCE(handler_arg->done), "Event handled");
> + WRITE_ONCE(handler_arg->done, false);
> + }
> +
> +cleanup:
> + on_cpus(sbi_sse_disable_unregister_local, cpu_args);
> + for_each_online_cpu(cpu) {
> + cpu_arg = &cpu_args[cpu];
> + ret = READ_ONCE(cpu_arg->ret);
> + if (ret.error)
> + report_fail("CPU failed to disable/unregister event: %ld", ret.error);
> + }
> +
> + for_each_online_cpu(cpu) {
> + cpu_arg = &cpu_args[cpu];
> + sse_free_stack(cpu_arg->args.stack);
> + }
> +
> + report_prefix_pop();
> +}
> +
> +static void sse_test_inject_global_cpu(uint32_t event_id, unsigned int cpu,
> + struct sse_foreign_cpu_test_arg *test_arg)
> +{
> + unsigned long value;
> + struct sbiret ret;
> + uint64_t timeout;
> + enum sbi_sse_state state;
> +
> + WRITE_ONCE(test_arg->expected_cpu, cpu);
> + /* For test_arg content to be visible for other CPUs */
> + smp_wmb();
> + value = cpu;
> + ret = sbi_sse_write_attrs(event_id, SBI_SSE_ATTR_PREFERRED_HART, 1, &value);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "Set preferred hart"))
> + return;
> +
> + ret = sbi_sse_enable(event_id);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "Enable event"))
> + return;
> +
> + ret = sbi_sse_inject(event_id, cpu);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "Inject event"))
> + goto disable;
> +
> + smp_rmb();
> + timeout = sse_event_get_complete_timeout();
> + while (!READ_ONCE(test_arg->done) || timer_get_cycles() < timeout) {
same comment
> + /* For shared test_arg structure */
> + smp_rmb();
> + cpu_relax();
> + }
> +
> + report(READ_ONCE(test_arg->done), "event handler called");
> + WRITE_ONCE(test_arg->done, false);
> +
> + timeout = sse_event_get_complete_timeout();
> + /* Wait for event to be back in ENABLED state */
> + do {
> + ret = sse_event_get_state(event_id, &state);
> + if (ret.error)
> + goto disable;
> + cpu_relax();
> + } while (state != SBI_SSE_STATE_ENABLED || timer_get_cycles() < timeout);
same comment
Otherwise,
Reviewed-by: Andrew Jones <andrew.jones@xxxxxxxxx>
Thanks,
drew
