Re: [PATCH 02/10] rv: Let the reactors take care of buffers

[Nam Cao <namcao@linutronix.de>]

Hi Gabriele,

On Wed, Mar 12, 2025 at 08:58:56AM +0100, Gabriele Monaco wrote:
> On Tue, 2025-03-11 at 18:05 +0100, Nam Cao wrote:
> > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> > index 510c88bfabd4..c55d45544a16 100644
> > --- a/include/rv/da_monitor.h
> > +++ b/include/rv/da_monitor.h
> > @@ -16,58 +16,11 @@
> >  #include <linux/bug.h>
> >  #include <linux/sched.h>
> >  
> > -#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)							\
> > -
> > {												\
> > -	if
> > (rv_##name.react)									\
> > -
> > 		rv_##name.react(msg);								\
> > -
> > }												\
> > -
> > 												\
> > -static bool
> > rv_reacting_on_##name(void)								\
> > -
> > {												\
> > -	return
> > rv_reacting_on();								\
> > -}
> > -
> > -#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)							\
> > -
> > {												\
> > -
> > 	return;											\
> > -
> > }												\
> > -
> > 												\
> > -static bool
> > rv_reacting_on_##name(void)								\
> > -
> > {												\
> > -	return
> > 0;										\
> > -}
> > -#endif
> > -
> 
> I don't think you need to remove those helper functions, why not just
> having format_react_msg_ prepare the arguments for react?

I'm not sure what you mean. Making format_react_msg_* a macro that is
preprocessed into arguments? Then make cond_react_*() a variadic function,
so that we can "pass" format_react_msg_* to it?

Going that way would also need a vreact() variant, as cond_react_*() cannot
pass on the variadic arguments to react().

Instead, is it cleaner to do the below?

diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
index 510c88bfabd4..e185ebf894a4 100644
--- a/include/rv/da_monitor.h
+++ b/include/rv/da_monitor.h
@@ -19,22 +19,14 @@
 #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);								\
+	if (!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));				\
 }												\
 												\
 static bool rv_reacting_on_##name(void)								\
@@ -45,12 +37,7 @@ static bool rv_reacting_on_##name(void)								\
 #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;											\
 }												\
@@ -171,7 +158,7 @@ da_event_##name(struct da_monitor *da_mon, enum events_##name event)				\
 	}											\
 												\
 	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));					\
@@ -203,7 +190,7 @@ static inline bool da_event_##name(struct da_monitor *da_mon, struct task_struct
 	}											\
 												\
 	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),				\

Best regards,
Nam