Messages in this thread | | | Date | Fri, 13 Oct 2023 15:23:33 +0530 | Subject | Re: [PATCH] bus: mhi: host: Add tracing support | From | 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
| |