Received-SPF: pass (google.com: domain of linux-kernel+bounces-45612-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <98602d57-5b52-1180-4a71-26223b9b4738@quicinc.com>
Date: Wed, 31 Jan 2024 07:20:05 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v9] bus: mhi: host: Add tracing support
Content-Language: en-US
To: Manivannan Sadhasivam <mani@kernel.org>
CC: Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu
	<mhiramat@kernel.org>,
        <linux-kernel@vger.kernel.org>, <mhi@lists.linux.dev>,
        <linux-arm-msm@vger.kernel.org>, <linux-trace-kernel@vger.kernel.org>,
        <quic_vbadigan@quicinc.com>, <quic_ramkri@quicinc.com>,
        <quic_nitegupt@quicinc.com>, <quic_skananth@quicinc.com>,
        <quic_parass@quicinc.com>
References: <20240105-ftrace_support-v9-1-a2dca64cc6ea@quicinc.com>
 <20240130081152.GH32821@thinkpad>
From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
In-Reply-To: <20240130081152.GH32821@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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
>