On Fri, 2025-04-11 at 09:37 +0200, 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.
>
> Signed-off-by: Nam Cao <nam...@linutronix.de>
> ---
> Cc: Petr Mladek <pmla...@suse.com>
> Cc: John Ogness <john.ogn...@linutronix.de>
> Cc: Sergey Senozhatsky <senozhat...@chromium.org>
> ---
> 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(-)
>
> diff --git a/include/linux/panic.h b/include/linux/panic.h
> index 54d90b6c5f47..3522f8c441f4 100644
> --- a/include/linux/panic.h
> +++ b/include/linux/panic.h
> @@ -3,6 +3,7 @@
> #define _LINUX_PANIC_H
>
> #include <linux/compiler_attributes.h>
> +#include <linux/stdarg.h>
> #include <linux/types.h>
>
> struct pt_regs;
> @@ -10,6 +11,8 @@ struct pt_regs;
> extern long (*panic_blink)(int state);
> __printf(1, 2)
> void panic(const char *fmt, ...) __noreturn __cold;
> +__printf(1, 0)
> +void vpanic(const char *fmt, va_list args) __noreturn __cold;
> void nmi_panic(struct pt_regs *regs, const char *msg);
> void check_panic_on_warn(const char *origin);
> extern void oops_enter(void);
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 4217a9f412b2..1b7eebe13f14 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -154,6 +154,7 @@ int vprintk_emit(int facility, int level,
>
> asmlinkage __printf(1, 0)
> int vprintk(const char *fmt, va_list args);
> +__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
>
> asmlinkage __printf(1, 2) __cold
> int _printk(const char *fmt, ...);
> @@ -213,6 +214,10 @@ int vprintk(const char *s, va_list args)
> {
> return 0;
> }
> +__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args)
> +{
> + return 0;
> +}
> static inline __printf(1, 2) __cold
> int _printk(const char *s, ...)
> {
>From the RV perspective this patch looks really good to me, although
you're doing a bit more than just RV here.
I hate to be the one telling you to split patches (22 is already scary
as it is!) but probably the vpanic and vprintk_deferred belong in
separate patches.
Feel free to mark the RV part (and also 1/22 2/22)
Reviewed-by: Gabriele Monaco <gmon...@redhat.com>
Thanks,
Gabriele
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index 3452b5e4b29e..c7c18c06911b 100644
> --- 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
> };
>
> @@ -64,6 +64,11 @@ void rv_put_task_monitor_slot(int slot);
> bool rv_reacting_on(void);
> int rv_unregister_reactor(struct rv_reactor *reactor);
> int rv_register_reactor(struct rv_reactor *reactor);
> +#else
> +bool rv_reacting_on(void)
> +{
> + return false;
> +}
> #endif /* CONFIG_RV_REACTORS */
>
> #endif /* CONFIG_RV */
> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> index 510c88bfabd4..15f9ed4e4bb6 100644
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
> @@ -19,45 +19,22 @@
> #ifdef CONFIG_RV_REACTORS
>
> #define DECLARE_RV_REACTING_HELPERS(name,
> type) \
> -static char
> REACT_MSG_##name[1024];
> \
> -
>
> \
> -static inline char *format_react_msg_##name(type curr_state, type
> event) \
> -
> {
> \
> - snprintf(REACT_MSG_##name,
> 1024, \
> - "rv: monitor %s does not allow event %s on state
> %s\n", \
> -
> #name,
> \
> -
> model_get_event_name_##name(event),
> \
> -
> model_get_state_name_##name(curr_state));
> \
> - return
> REACT_MSG_##name;
> \
> -
> }
> \
> -
>
> \
> -static void cond_react_##name(char
> *msg) \
> +static void cond_react_##name(type curr_state, type
> event) \
> {
> \
> - if
> (rv_##name.react)
> \
> -
> rv_##name.react(msg);
> \
> -
> }
> \
> -
>
> \
> -static bool
> rv_reacting_on_##name(void)
> \
> -
> {
> \
> - return
> rv_reacting_on();
> \
> + if (!rv_reacting_on() ||
> !rv_##name.react) \
> + return;
> \
> + rv_##name.react("rv: monitor %s does not allow event %s on
> state %s\n", \
> + #name,
> \
> + model_get_event_name_##name(event),
> \
> + model_get_state_name_##name(curr_state));
> \
> }
>
> #else /* CONFIG_RV_REACTOR */
>
> #define DECLARE_RV_REACTING_HELPERS(name,
> type) \
> -static inline char *format_react_msg_##name(type curr_state, type
> event) \
> -
> {
> \
> - return
> NULL;
> \
> -
> }
> \
> -
>
> \
> -static void cond_react_##name(char
> *msg) \
> +static void cond_react_##name(type curr_state, type
> event) \
> {
> \
> return;
> \
> -
> }
> \
> -
>
> \
> -static bool
> rv_reacting_on_##name(void)
> \
> -
> {
> \
> - return
> 0;
> \
> }
> #endif
>
> @@ -170,8 +147,7 @@ da_event_##name(struct da_monitor *da_mon, enum
> events_##name event) \
> return
> true; \
> }
> \
>
> \
> - if
> (rv_reacting_on_##name())
> \
> -
> cond_react_##name(format_react_msg_##name(curr_state, event));
> \
> + cond_react_##name(curr_state,
> event); \
>
> \
> trace_error_##name(model_get_state_name_##name(curr_state),
> \
>
> model_get_event_name_##name(event)); \
> @@ -202,8 +178,7 @@ static inline bool da_event_##name(struct
> da_monitor *da_mon, struct task_struct
> return
> true; \
> }
> \
>
> \
> - if
> (rv_reacting_on_##name())
> \
> -
> cond_react_##name(format_react_msg_##name(curr_state, event));
> \
> + cond_react_##name(curr_state,
> event); \
>
> \
> trace_error_##name(tsk-
> >pid, \
>
> model_get_state_name_##name(curr_state), \
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d8635d5cecb2..df799d784b61 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -277,17 +277,16 @@ static void panic_other_cpus_shutdown(bool
> crash_kexec)
> }
>
> /**
> - * panic - halt the system
> + * vpanic - halt the system
> * @fmt: The text string to print
> *
> * Display a message, then perform cleanups.
> *
> * This function never returns.
> */
> -void panic(const char *fmt, ...)
> +void vpanic(const char *fmt, va_list args)
> {
> static char buf[1024];
> - va_list args;
> long i, i_next = 0, len;
> int state = 0;
> int old_cpu, this_cpu;
> @@ -338,9 +337,7 @@ void panic(const char *fmt, ...)
>
> console_verbose();
> bust_spinlocks(1);
> - va_start(args, fmt);
> len = vscnprintf(buf, sizeof(buf), fmt, args);
> - va_end(args);
>
> if (len && buf[len - 1] == '\n')
> buf[len - 1] = '\0';
> @@ -477,7 +474,17 @@ void panic(const char *fmt, ...)
> mdelay(PANIC_TIMER_STEP);
> }
> }
> +EXPORT_SYMBOL(vpanic);
>
> +/* Identical to vpanic(), except it takes variadic arguments instead
> of va_list */
> +void panic(const char *fmt, ...)
> +{
> + va_list args;
> +
> + va_start(args, fmt);
> + vpanic(fmt, args);
> + va_end(args);
> +}
> EXPORT_SYMBOL(panic);
>
> #define TAINT_FLAG(taint, _c_true, _c_false,
> _module) \
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index a91bdf802967..28afdeb58412 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -71,7 +71,6 @@ int vprintk_store(int facility, int level,
> const char *fmt, va_list args);
>
> __printf(1, 0) int vprintk_default(const char *fmt, va_list args);
> -__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
>
> void __printk_safe_enter(void);
> void __printk_safe_exit(void);
> diff --git a/kernel/trace/rv/reactor_panic.c
> b/kernel/trace/rv/reactor_panic.c
> index 0186ff4cbd0b..2587f23db80b 100644
> --- a/kernel/trace/rv/reactor_panic.c
> +++ b/kernel/trace/rv/reactor_panic.c
> @@ -13,9 +13,13 @@
> #include <linux/init.h>
> #include <linux/rv.h>
>
> -static void rv_panic_reaction(char *msg)
> +static void rv_panic_reaction(const char *msg, ...)
> {
> - panic(msg);
> + va_list args;
> +
> + va_start(args, msg);
> + vpanic(msg, args);
> + va_end(args);
> }
>
> static struct rv_reactor rv_panic = {
> diff --git a/kernel/trace/rv/reactor_printk.c
> b/kernel/trace/rv/reactor_printk.c
> index 178759dbf89f..a15db3fc8b82 100644
> --- 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, ...)
> {
> - printk_deferred(msg);
> + va_list args;
> +
> + va_start(args, msg);
> + vprintk_deferred(msg, args);
> + va_end(args);
> }
>
> static struct rv_reactor rv_printk = {
> diff --git a/kernel/trace/rv/rv_reactors.c
> b/kernel/trace/rv/rv_reactors.c
> index 9501ca886d83..4ce6ebb9d095 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -490,7 +490,7 @@ void reactor_cleanup_monitor(struct
> rv_monitor_def *mdef)
> /*
> * Nop reactor register
> */
> -static void rv_nop_reaction(char *msg)
> +static void rv_nop_reaction(const char *msg, ...)
> {
> }
>
