linux-kernel - Re: [PATCH] bus: mhi: host: Add tracing support

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4c23ee39-dc28-8883-caf6-21a316873cbd@quicinc.com>
Date:   Fri, 13 Oct 2023 15:23:33 +0530
From:   Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To:     Bjorn Andersson <quic_bjorande@...cinc.com>
CC:     Manivannan Sadhasivam <mani@...nel.org>,
        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] bus: mhi: host: Add tracing support


On 10/6/2023 4:10 AM, Bjorn Andersson wrote:
> On Thu, Oct 05, 2023 at 03:55:20PM +0530, Krishna chaitanya chundru wrote:
>> This change adds ftrace support for following:
>> 1. mhi_intvec_threaded_handler
>> 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
> This is not the best "problem description".
>
>> Usage:
>> 	echo 1 > /sys/kernel/debug/tracing/events/mhi_host/enable
>> 	cat /sys/kernel/debug/tracing/trace
> This does not need to be included in the commit message, how to use the
> tracing framework is documented elsewhere.
Removed this now.
> [..]
>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>> index dcf627b36e82..499590437e9b 100644
>> --- a/drivers/bus/mhi/host/main.c
>> +++ b/drivers/bus/mhi/host/main.c
>> @@ -491,11 +491,10 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>>   
>>   	state = mhi_get_mhi_state(mhi_cntrl);
>>   	ee = mhi_get_exec_env(mhi_cntrl);
>> -	dev_dbg(dev, "local ee: %s state: %s device ee: %s state: %s\n",
>> -		TO_MHI_EXEC_STR(mhi_cntrl->ee),
>> -		mhi_state_str(mhi_cntrl->dev_state),
>> -		TO_MHI_EXEC_STR(ee), mhi_state_str(state));
>>   
>> +	trace_mhi_intvec_threaded_handler(mhi_cntrl->mhi_dev->name, TO_MHI_EXEC_STR(mhi_cntrl->ee),
>> +					  mhi_state_str(mhi_cntrl->dev_state),
>> +					  TO_MHI_EXEC_STR(ee), mhi_state_str(state));
> All these helper functions that translates a state to a string, pass the
> raw state into the trace event and use __print_symbolic() in your
> TP_printk() instead.
>
> This will allow you to read the state, but you can have tools act of the
> numerical value.
>
>
> (This comment applies to all the trace events)
changed the events as you suggested.
>>   	if (state == MHI_STATE_SYS_ERR) {
>>   		dev_dbg(dev, "System error detected\n");
>>   		pm_state = mhi_tryset_pm_state(mhi_cntrl,
> [..]
>> diff --git a/include/trace/events/mhi_host.h b/include/trace/events/mhi_host.h
> [..]
>> +
>> +TRACE_EVENT(mhi_pm_st_worker,
> Why is this trace event called "worker", isn't the event a
> "mhi_pm_state_transition"?
>
> Don't just name your trace event based on the function that triggers
> them, but what they represent and make sure they carry useful
> information to understand the system.
>
> If you want to trace the flow through your functions, you can use e.g.
> ftrace.
>
> Regards,
> Bjorn

Changed as you suggested.

- KC