Re: [EXT] Re: [PATCH 2/6] qla2xxx: Add a generic tracing framework

[Arun Easi <aeasi@marvell.com>]

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

On Tue, 19 Jul 2022, 2:40pm, Steven Rostedt wrote:

> On Tue, 19 Jul 2022 14:06:11 -0700
> Arun Easi <aeasi@marvell.com> wrote:
> 
> > It appears by calling the above two interfaces, I get a separate instance 
> > of "qla" only traces, but with only the "qla"-only instance being enabled, 
> > leaving the main (/sys/kernel/tracing/events/qla/enable) one disabled. No 
> > issues there, but when I enable both of them, I get garbage values on one.
> > 
> > /sys/kernel/tracing/instances/qla2xxx/trace:
> >              cat-56106   [012] ..... 2419873.470098: ql_dbg_log: qla2xxx [0000:05:00.0]-1054:14:  Entered (null).
> >              cat-56106   [012] ..... 2419873.470101: ql_dbg_log: qla2xxx [0000:05:00.0]-1000:14:  Entered ×+<96>²Ü<98>^H.
> >              cat-56106   [012] ..... 2419873.470102: ql_dbg_log: qla2xxx [0000:05:00.0]-1006:14:  Prepare to issue mbox cmd=0xde589000.
> > 
> > /sys/kernel/tracing/trace:
> >              cat-56106   [012] ..... 2419873.470097: ql_dbg_log: qla2xxx [0000:05:00.0]-1054:14:  Entered qla2x00_get_firmware_state.
> >              cat-56106   [012] ..... 2419873.470100: ql_dbg_log: qla2xxx [0000:05:00.0]-1000:14:  Entered qla2x00_mailbox_command.
> >              cat-56106   [012] ..... 2419873.470102: ql_dbg_log: qla2xxx [0000:05:00.0]-1006:14:  Prepare to issue mbox cmd=0x69.
> > 
> > It appears that only one should be enabled at a time. Per my read of 
> > Documentation/trace/ftrace.rst, the main directory and instances have 
> > separate trace buffers, so I am a bit confused with the above output.
> 
> That's because it uses va_list, and va_list can only be used once.

Ah, that makes sense.

> 
> I just added helpers for va_list use cases:
> 
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20220705224453.120955146-40goodmis.org_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=P-q_Qkt75qFy33SvdD2nAxAyN87eO1d-mFO-lqNOomw&m=cDBXba_lNDGk7c1Qm4elDe1Co-RV8zR-c1A9xV7vc4nMcQO7iRle72qNxljHsDNA&s=74PzgMj6nmfRCLXd3oMIqt-_DZbQeukUl0nK7MzfUjs&e= 
> 
> But it will likely suffer the same issue, but I can easily fix that with
> this on top:
> 
> Not even compiled tested:
> 
> -- Steve
> 
> diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h
> index 0f51f6b3ab70..3c554a585320 100644
> --- a/include/trace/stages/stage6_event_callback.h
> +++ b/include/trace/stages/stage6_event_callback.h
> @@ -40,7 +40,12 @@
>  
>  #undef __assign_vstr
>  #define __assign_vstr(dst, fmt, va)					\
> -	vsnprintf(__get_str(dst), TRACE_EVENT_STR_MAX, fmt, *(va))
> +	do {								\
> +		va_list __cp_va;					\
> +		va_copy(__cp_va, *(va));				\
> +		vsnprintf(__get_str(dst), TRACE_EVENT_STR_MAX, fmt, __cp_va); \
> +		va_end(__cp_va);					\
> +	} while (0)
>  
>  #undef __bitmask
>  #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
> 

This will be nice.

Thanks for your help, Steve.

Regards,
-Arun