Re: [PATCH] bus: mhi: host: Add tracing support

[Krishna Chaitanya Chundru]

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