Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Thu, 5 Oct 2023 15:40:47 -0700
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Krishna chaitanya chundru <quic_krichai@quicinc.com>
CC:     Manivannan Sadhasivam <mani@kernel.org>,
        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>
Subject: Re: [PATCH] bus: mhi: host: Add tracing support
Message-ID: <20231005224047.GJ3553829@hu-bjorande-lv.qualcomm.com>
References: <20231005-ftrace_support-v1-1-23a2f394fa49@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231005-ftrace_support-v1-1-23a2f394fa49@quicinc.com>
Precedence: bulk

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.

[..]
> 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)

>  	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