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

[Petr Mladek]

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@xxxxxxxx>    # 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