Re: [RFC PATCH 10/12] rv: Add KUnit tests for some DA/HA monitors

[Nam Cao <namcao@linutronix.de>]

Gabriele Monaco <gmonaco@redhat.com> writes:
> +#ifdef CONFIG_RV_MONITORS_KUNIT_TEST
> +#include <kunit/test.h>
> +
> +/*
> + * rv_prepare_test - Disable the monitor for a kunit test
      ^^^^^^^^^^^^^^^
      wrong name

> +obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) += rv_monitors_test.o
> +# Stubbing rv_react triggers the error
> +CFLAGS_rv_monitors_test.o += -Wno-suggest-attribute=format

I try removing this flag, but my compiler does not produce any
warning. Which warning did you see?

If it is not a hassle, I would prefer to address the warning in C code.
Grep tells me the rest of the kernel does not use this, how do other
subsystems not suffer from this warning?

> +void rv_test_opid(struct kunit *test)
> +{
> +	struct rv_kunit_ctx *ctx = test->priv;
> +
> +	da_prepare_test(test, &rv_this);
> +
> +	/*
> +	 * The handlers are called with an additional level of preemption,
> +	 * ensure we start from 0 but apply it here to avoid warnings.
> +	 */
> +	KUNIT_ASSERT_TRUE(test, preemptible());
> +	guard(preempt)();
> +
> +	/* Wakeup with preemption and interrupts enabled */
> +	handle_sched_waking(NULL, NULL);
> +	RV_KUNIT_EXPECT_REACTION(test, ctx);
> +
> +	/* Need resched with interrupts enabled */
> +	scoped_guard(preempt)
> +		handle_sched_need_resched(NULL, NULL, 0, TIF_NEED_RESCHED);

From my understanding, this last one is testing that need_resched with
interrupt enabled does not invoke the reactor? And if the monitor is
broken and the reactor is invoked, we would have no test failure here,
but instead we have test failure the next time
RV_KUNIT_EXPECT_REACTION() is called. And if this is the last test, we
would not see a failure?

If so, should we perhaps have something like RV_KUNIT_EXPECT_NO_REACTION()
here? So that if the monitor is broken and the reactor is called, then
the kunit test will fail exactly where the failure is.

Reading the patch further down below, we actually do have that
macro! Shouldn't we use it here?

> +#ifdef CONFIG_RV_MON_SCO
> +extern void rv_test_sco(struct kunit *test);
> +#else
> +static inline void rv_test_sco(struct kunit *test)
> +{
> +	kunit_skip(test, "Monitor not enabled\n");
> +}
> +#endif

Instead of these, I would prefer we have something like

#define KUNIT_CASE_IF(test_name, enabled) \
	{ .run_case = enabled ? test_name : some_stub, ... }

and

static struct kunit_case rv_trigger_test_cases[] = {
	KUNIT_CASE_IF(rv_test_sco, CONFIG_RV_MON_SCO),
        ...
	{}
};

or something like that. But no big deal.

Nam