| Message ID | 20220419163859.2228874-11-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce In Field Scan driver | expand |
On Tue, 19 Apr 2022 09:38:58 -0700 Tony Luck <tony.luck@intel.com> wrote: > +TRACE_EVENT(ifs_status, > + > + TP_PROTO(union ifs_scan activate, union ifs_status status), Really, you want to pass the structure in by value, so that we have two copies? One to get to this function and then one to write to the ring buffer? -- Steve > + > + TP_ARGS(activate, status), > + > + TP_STRUCT__entry( > + __field( u64, status ) > + __field( u8, start ) > + __field( u8, stop ) > + ), > + > + TP_fast_assign( > + __entry->start = activate.start; > + __entry->stop = activate.stop; > + __entry->status = status.data; > + ), > + > + TP_printk("start: %.2x, stop: %.2x, status: %llx", > + __entry->start, > + __entry->stop, > + __entry->status) > +); > + > +#endif /* _TRACE_IFS_H */
>> +TRACE_EVENT(ifs_status, >> + >> + TP_PROTO(union ifs_scan activate, union ifs_status status), > > Really, you want to pass the structure in by value, so that we have two > copies? One to get to this function and then one to write to the ring > buffer? These "structures" are just bitfield helpers for a u64 that is passed into WRMSR (in the case of activate) and received back from RDMSR in the case of status. So this is really just a pair of u64 arguments, with the compiler handling the bit field extractions into the ring buffer. Here are the definitions: union ifs_scan { u64 data; struct { u32 start :8; u32 stop :8; u32 rsvd :16; u32 delay :31; u32 sigmce :1; }; }; union ifs_status { u64 data; struct { u32 chunk_num :8; u32 chunk_stop_index :8; u32 rsvd1 :16; u32 error_code :8; u32 rsvd2 :22; u32 control_error :1; u32 signature_error :1; }; }; Would it be better to do the bit extractions of the start/stop fields first? -Tony
On Thu, 21 Apr 2022 04:26:39 +0000 "Luck, Tony" <tony.luck@intel.com> wrote: > >> +TRACE_EVENT(ifs_status, > >> + > >> + TP_PROTO(union ifs_scan activate, union ifs_status status), > > > > Really, you want to pass the structure in by value, so that we have two > > copies? One to get to this function and then one to write to the ring > > buffer? > > These "structures" are just bitfield helpers for a u64 that is passed into > WRMSR (in the case of activate) and received back from RDMSR in > the case of status. > > So this is really just a pair of u64 arguments, with the compiler handling > the bit field extractions into the ring buffer. I was just wondering if passing by reference would be better, but as you stated, they are just two u64 arguments. > > Here are the definitions: > > union ifs_scan { > u64 data; > struct { > u32 start :8; > u32 stop :8; > u32 rsvd :16; > u32 delay :31; > u32 sigmce :1; > }; > }; > > union ifs_status { > u64 data; > struct { > u32 chunk_num :8; > u32 chunk_stop_index :8; > u32 rsvd1 :16; > u32 error_code :8; > u32 rsvd2 :22; > u32 control_error :1; > u32 signature_error :1; > }; > }; > > Would it be better to do the bit extractions of the start/stop fields first? No, I'm just paranoid about passing structures / unions in by value and not reference. If you are fine with this, then I'm OK too. -- Steve
diff --git a/MAINTAINERS b/MAINTAINERS index 9e372a960fa5..b488ff628a43 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9867,6 +9867,7 @@ R: Ashok Raj <ashok.raj@intel.com> R: Tony Luck <tony.luck@intel.com> S: Maintained F: drivers/platform/x86/intel/ifs +F: include/trace/events/intel_ifs.h INTEL INTEGRATED SENSOR HUB DRIVER M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index 246eff250563..c6fa9385dda0 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -24,6 +24,9 @@ static atomic_t siblings_out; static int cpu_sibl_ct; static bool scan_enabled = true; +#define CREATE_TRACE_POINTS +#include <trace/events/intel_ifs.h> + struct ifs_work { struct work_struct w; struct device *dev; @@ -217,6 +220,8 @@ static void ifs_work_func(struct work_struct *work) rdmsrl(MSR_SCAN_STATUS, status.data); + trace_ifs_status(activate, status); + /* Some cases can be retried, give up for others */ if (!can_restart(status)) break; diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h new file mode 100644 index 000000000000..0611f370cb37 --- /dev/null +++ b/include/trace/events/intel_ifs.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM intel_ifs + +#if !defined(_TRACE_IFS_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_IFS_H + +#include <linux/ktime.h> +#include <linux/tracepoint.h> + +TRACE_EVENT(ifs_status, + + TP_PROTO(union ifs_scan activate, union ifs_status status), + + TP_ARGS(activate, status), + + TP_STRUCT__entry( + __field( u64, status ) + __field( u8, start ) + __field( u8, stop ) + ), + + TP_fast_assign( + __entry->start = activate.start; + __entry->stop = activate.stop; + __entry->status = status.data; + ), + + TP_printk("start: %.2x, stop: %.2x, status: %llx", + __entry->start, + __entry->stop, + __entry->status) +); + +#endif /* _TRACE_IFS_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h>