| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <98602d57-5b52-1180-4a71-26223b9b4738@quicinc.com>
Date: Wed, 31 Jan 2024 07:20:05 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Manivannan Sadhasivam <mani@...nel.org>
CC: Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu
<mhiramat@...nel.org>,
<linux-kernel@...r.kernel.org>, <mhi@...ts.linux.dev>,
<linux-arm-msm@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>,
<quic_vbadigan@...cinc.com>, <quic_ramkri@...cinc.com>,
<quic_nitegupt@...cinc.com>, <quic_skananth@...cinc.com>,
<quic_parass@...cinc.com>
Subject: Re: [PATCH v9] bus: mhi: host: Add tracing support
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@...cinc.com>
>
> Few nitpicks below.
>
I will make suggested changes in next patch.
- Krishna Chaitanya.
>> Reviewed-by: "Steven Rostedt (Google)" <rostedt@...dmis.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
>
Powered by blists - more mailing lists