Re: [PATCH v2 03/22] rv: Let the reactors take care of buffers

Petr Mladek
2025-04-15T09:32:48Z

On Fri 2025-04-11 09:37:19, Nam Cao wrote:
> Each RV monitor has one static buffer to send to the reactors. If multiple
> errors are detected simultaneously, the one buffer could be overwritten.
>
> Instead, leave it to the reactors to handle buffering.
>
> include/linux/panic.h | 3 +++
> include/linux/printk.h | 5 ++++
> include/linux/rv.h | 9 +++++--
> include/rv/da_monitor.h | 45 +++++++-------------------------
> kernel/panic.c | 17 ++++++++----
> kernel/printk/internal.h | 1 -
> kernel/trace/rv/reactor_panic.c | 8 ++++--
> kernel/trace/rv/reactor_printk.c | 8 ++++--
> kernel/trace/rv/rv_reactors.c | 2 +-
> 9 files changed, 50 insertions(+), 48 deletions(-)

For the changes in the printk and panic code:

Reviewed-by: Petr Mladek <pmladek@suse.com> # printk, panic

I have just briefly looked at the changes in the rv code.
I wonder if a __printf(1, 2) declaration might be needed
in the printk and panic reactors code, see below.

> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -38,7 +38,7 @@ union rv_task_monitor {
> struct rv_reactor {
> const char *name;
> const char *description;
> - void (*react)(char *msg);
> + __printf(1, 2) void (*react)(const char *msg, ...);
> };
> #endif
>
> @@ -50,7 +50,7 @@ struct rv_monitor {
> void (*disable)(void);
> void (*reset)(void);
> #ifdef CONFIG_RV_REACTORS
> - void (*react)(char *msg);
> + __printf(1, 2) void (*react)(const char *msg, ...);
> #endif
> };
>
> --- a/kernel/trace/rv/reactor_printk.c
> +++ b/kernel/trace/rv/reactor_printk.c
> @@ -12,9 +12,13 @@
> #include <linux/init.h>
> #include <linux/rv.h>
>
> -static void rv_printk_reaction(char *msg)
> +static void rv_printk_reaction(const char *msg, ...)

I wonder whether "make W=1 kernel/trace/rv/reactor_printk.o" would
start complaining about that this function is a candidate for
‘gnu_printf’ format attribute.

I am not sure. Maybe it is enough that this function is later assigned to
the .react callback in struct rv_reactor.

I wanted to tried it myself. But I was not able to compile the
code in linux-next. I got something like:

./include/linux/rv.h: In function ‘rv_ltl_valid_state’:
./include/linux/rv.h:55:43: error: ‘struct ltl_monitor’ has no member named ‘states’
55 | for (int i = 0; i < ARRAY_SIZE(mon->states); ++i) {
| ^~
...

I am actually not sure against which tree I should apply this patchset.
It did apply on linux-next after skipping the 1st patch.
But it does not compile there.

And there are more conflicts when I tried to apply it
on Linus' master.

> {
> - printk_deferred(msg);
> + va_list args;
> +
> + va_start(args, msg);
> + vprintk_deferred(msg, args);
> + va_end(args);
> }

The __printf statement might be missing also in the other two
reactors (panic, nop).

Best Regards,
Petr