Messages in this thread | | | Date | Wed, 31 Jan 2024 07:20:05 +0530 | Subject | Re: [PATCH v9] bus: mhi: host: Add tracing support | From | Krishna Chaitanya Chundru <> |
| |
On 1/30/2024 1:41 PM, Manivannan Sadhasivam wrote: > On Fri, Jan 05, 2024 at 05:53:03PM +0530, Krishna chaitanya chundru wrote: >> This change adds ftrace support for following functions which >> helps in debugging the issues when there is Channel state & MHI >> state change and also when we receive data and control events: >> 1. mhi_intvec_mhi_states >> 2. mhi_process_data_event_ring >> 3. mhi_process_ctrl_ev_ring >> 4. mhi_gen_tre >> 5. mhi_update_channel_state >> 6. mhi_tryset_pm_state >> 7. mhi_pm_st_worker >> >> Change the implementation of the arrays which has enum to strings mapping >> to make it consistent in both trace header file and other files. >> >> Where ever the trace events are added, debug messages are removed. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > Few nitpicks below. > I will make suggested changes in next patch.
- Krishna Chaitanya. >> Reviewed-by: "Steven Rostedt (Google)" <rostedt@goodmis.org> >> --- >> Changes in v9: >> - Change the implementations of some array so that the strings to enum mapping >> - is same in both trace header and other files as suggested by steve. >> - Link to v8: https://lore.kernel.org/r/20231207-ftrace_support-v8-1-7f62d4558555@quicinc.com >> >> Changes in v8: >> - Pass the structure and derefernce the variables in TP_fast_assign as suggested by steve >> - Link to v7: https://lore.kernel.org/r/20231206-ftrace_support-v7-1-aca49a04268b@quicinc.com >> >> Changes in v7: >> - change log format as pointed by mani. >> - Link to v6: https://lore.kernel.org/r/20231204-ftrace_support-v6-1-9b206546dac2@quicinc.com >> >> Changes in v6: >> - use 'rp' directly as suggested by jeffrey. >> - Link to v5: https://lore.kernel.org/r/20231127-ftrace_support-v5-1-eb67daead4f1@quicinc.com >> >> Changes in v5: >> - Use DECLARE_EVENT_CLASS for multiple events as suggested by steve. >> - Instead of converting to u64 to print address, use %px to print the address to avoid >> - warnings in some platforms. >> - Link to v4: https://lore.kernel.org/r/20231111-ftrace_support-v4-1-c83602399461@quicinc.com >> >> Changes in v4: >> - Fix compilation issues in previous patch which happended due to rebasing. >> - In the defconfig FTRACE config is not enabled due to that the compilation issue is not >> - seen in my workspace. >> - Link to v3: https://lore.kernel.org/r/20231111-ftrace_support-v3-1-f358d2911a74@quicinc.com >> >> Changes in v3: >> - move trace header file from include/trace/events to drivers/bus/mhi/host/ so that >> - we can include driver header files. >> - Use macros directly in the trace events as suggested Jeffrey Hugo. >> - Reorder the structure in the events as suggested by steve to avoid holes in the buffer. >> - removed the mhi_to_physical function as this can give security issues. >> - removed macros to define strings as we can get those from driver headers. >> - Link to v2: https://lore.kernel.org/r/20231013-ftrace_support-v2-1-6e893ce010b5@quicinc.com >> >> Changes in v2: >> - Passing the raw state into the trace event and using __print_symbolic() as suggested by bjorn. >> - Change mhi_pm_st_worker to mhi_pm_st_transition as suggested by bjorn. >> - Fixed the kernel test rebot issues. >> - Link to v1: https://lore.kernel.org/r/20231005-ftrace_support-v1-1-23a2f394fa49@quicinc.com >> --- >> drivers/bus/mhi/common.h | 38 +++--- >> drivers/bus/mhi/host/init.c | 63 +++++---- >> drivers/bus/mhi/host/internal.h | 40 ++++++ >> drivers/bus/mhi/host/main.c | 19 ++- >> drivers/bus/mhi/host/pm.c | 7 +- >> drivers/bus/mhi/host/trace.h | 275 ++++++++++++++++++++++++++++++++++++++++ >> 6 files changed, 378 insertions(+), 64 deletions(-) >> > > [...] > >> +TRACE_EVENT(mhi_gen_tre, >> + >> + TP_PROTO(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, >> + struct mhi_ring_element *mhi_tre), >> + >> + TP_ARGS(mhi_cntrl, mhi_chan, mhi_tre), >> + >> + TP_STRUCT__entry( >> + __string(name, mhi_cntrl->mhi_dev->name) >> + __field(int, ch_num) >> + __field(void *, wp) >> + __field(__le64, tre_ptr) >> + __field(__le32, dword0) >> + __field(__le32, dword1) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(name, mhi_cntrl->mhi_dev->name); >> + __entry->ch_num = mhi_chan->chan; >> + __entry->wp = mhi_tre; >> + __entry->tre_ptr = mhi_tre->ptr; >> + __entry->dword0 = mhi_tre->dword[0]; >> + __entry->dword1 = mhi_tre->dword[1]; >> + ), >> + >> + TP_printk("%s: Chan: %d Tre: 0x%p Tre buf: 0x%llx dword0: 0x%08x dword1: 0x%08x\n", > > Use caps for printing the acronyms everywhere. Like TRE, DWORD etc... > >> + __get_str(name), __entry->ch_num, __entry->wp, __entry->tre_ptr, >> + __entry->dword0, __entry->dword1) >> +); >> + >> +TRACE_EVENT(mhi_intvec_states, >> + >> + TP_PROTO(struct mhi_controller *mhi_cntrl, int dev_ee, int dev_state), >> + >> + TP_ARGS(mhi_cntrl, dev_ee, dev_state), >> + >> + TP_STRUCT__entry( >> + __string(name, mhi_cntrl->mhi_dev->name) >> + __field(int, local_ee) >> + __field(int, state) >> + __field(int, dev_ee) >> + __field(int, dev_state) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(name, mhi_cntrl->mhi_dev->name); >> + __entry->local_ee = mhi_cntrl->ee; >> + __entry->state = mhi_cntrl->dev_state; >> + __entry->dev_ee = dev_ee; >> + __entry->dev_state = dev_state; >> + ), >> + >> + TP_printk("%s: local ee: %s state: %s device ee: %s state: %s\n", > > "Local EE... State:...Device EE.." > >> + __get_str(name), >> + __print_symbolic(__entry->local_ee, MHI_EE_LIST), >> + __print_symbolic(__entry->state, MHI_STATE_LIST), >> + __print_symbolic(__entry->dev_ee, MHI_EE_LIST), >> + __print_symbolic(__entry->state, MHI_STATE_LIST)) >> +); >> + > > [...] > >> +DECLARE_EVENT_CLASS(mhi_update_channel_state, >> + >> + TP_PROTO(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, int state), >> + >> + TP_ARGS(mhi_cntrl, mhi_chan, state), >> + >> + TP_STRUCT__entry( >> + __string(name, mhi_cntrl->mhi_dev->name) >> + __field(int, ch_num) >> + __field(int, state) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(name, mhi_cntrl->mhi_dev->name); >> + __entry->ch_num = mhi_chan->chan; >> + __entry->state = state; >> + ), >> + >> + TP_printk("%s: chan%d: Updating state to: %s\n", >> + __get_str(name), __entry->ch_num, >> + __print_symbolic(__entry->state, MHI_CH_STATE_TYPE_LIST)) > > So same trace will get printed for both mhi_channel_command_start() and > mhi_channel_command_end()? > > - Mani >
| |