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
Re: [PATCH] bus: mhi: host: Add tracing support
Hi Krishna, kernel test robot noticed the following build warnings: [auto build test WARNING on 3006adf3be79cde4d14b1800b963b82b6e5572e0] url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/bus-mhi-host-Add-tracing-support/20231005-231430 base: 3006adf3be79cde4d14b1800b963b82b6e5572e0 patch link: https://lore.kernel.org/r/20231005-ftrace_support-v1-1-23a2f394fa49%40quicinc.com patch subject: [PATCH] bus: mhi: host: Add tracing support config: i386-randconfig-062-20231010 (https://download.01.org/0day-ci/archive/20231010/202310102355.6sea9ysi-...@intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231010/202310102355.6sea9ysi-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202310102355.6sea9ysi-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/bus/mhi/host/main.c:835:56: sparse: sparse: incorrect type in >> argument 3 (different base types) @@ expected unsigned long long >> [usertype] ptr @@ got restricted __le64 [usertype] ptr @@ drivers/bus/mhi/host/main.c:835:56: sparse: expected unsigned long long [usertype] ptr drivers/bus/mhi/host/main.c:835:56: sparse: got restricted __le64 [usertype] ptr >> drivers/bus/mhi/host/main.c:835:78: sparse: sparse: incorrect type in >> argument 4 (different base types) @@ expected int dword0 @@ got >> restricted __le32 @@ drivers/bus/mhi/host/main.c:835:78: sparse: expected int dword0 drivers/bus/mhi/host/main.c:835:78: sparse: got restricted __le32 >> drivers/bus/mhi/host/main.c:836:63: sparse: sparse: incorrect type in >> argument 5 (different base types) @@ expected int dword1 @@ got >> restricted __le32 @@ drivers/bus/mhi/host/main.c:836:63: sparse: expected int dword1 drivers/bus/mhi/host/main.c:836:63: sparse: got restricted __le32 drivers/bus/mhi/host/main.c:1004:85: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long long [usertype] ptr @@ got restricted __le64 [usertype] ptr @@ drivers/bus/mhi/host/main.c:1004:85: sparse: expected unsigned long long [usertype] ptr drivers/bus/mhi/host/main.c:1004:85: sparse: got restricted __le64 [usertype] ptr drivers/bus/mhi/host/main.c:1005:66: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected int dword0 @@ got restricted __le32 @@ drivers/bus/mhi/host/main.c:1005:66: sparse: expected int dword0 drivers/bus/mhi/host/main.c:1005:66: sparse: got restricted __le32 drivers/bus/mhi/host/main.c:1005:86: sparse: sparse: incorrect type in argument 4 (different base types) @@ expected int dword1 @@ got restricted __le32 @@ drivers/bus/mhi/host/main.c:1005:86: sparse: expected int dword1 drivers/bus/mhi/host/main.c:1005:86: sparse: got restricted __le32 >> drivers/bus/mhi/host/main.c:1246:34: sparse: sparse: incorrect type in >> argument 4 (different base types) @@ expected unsigned long long >> [usertype] tre_ptr @@ got restricted __le64 [usertype] ptr @@ drivers/bus/mhi/host/main.c:1246:34: sparse: expected unsigned long long [usertype] tre_ptr drivers/bus/mhi/host/main.c:1246:34: sparse: got restricted __le64 [usertype] ptr drivers/bus/mhi/host/main.c:1246:55: sparse: sparse: incorrect type in argument 5 (different base types) @@ expected int dword0 @@ got restricted __le32 @@ drivers/bus/mhi/host/main.c:1246:55: sparse: expected int dword0 drivers/bus/mhi/host/main.c:1246:55: sparse: got restricted __le32 drivers/bus/mhi/host/main.c:1246:74: sparse: sparse: incorrect type in argument 6 (different base types) @@ expected int dword1 @@ got restricted __le32 @@ drivers/bus/mhi/host/main.c:1246:74: sparse: expected int dword1 drivers/bus/mhi/host/main.c:1246:74: sparse: got restricted __le32 >> drivers/bus/mhi/host/main.c:834:80: sparse: sparse: non size-preserving >> pointer to integer cast drivers/bus/mhi/host/main.c:1245:75: sparse: sparse: non size-preserving pointer to integer cast vim +835 drivers/bus/mhi/host/main.c 799 800 int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, 801 struct mhi_event *mhi_event, 802 u32 event_quota) 803 { 804 struct mhi_ring_element *dev_rp, *local_rp; 805 struct mhi_ring *ev_ring = _event->ring; 806 struct mhi_event_ctxt *er_ctxt = 807 _cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; 808 struct mhi_chan
Re: [PATCH] bus: mhi: host: Add tracing support
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
Re: [PATCH] bus: mhi: host: Add tracing support
Hi Krishna, kernel test robot noticed the following build warnings: [auto build test WARNING on 3006adf3be79cde4d14b1800b963b82b6e5572e0] url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/bus-mhi-host-Add-tracing-support/20231005-231430 base: 3006adf3be79cde4d14b1800b963b82b6e5572e0 patch link: https://lore.kernel.org/r/20231005-ftrace_support-v1-1-23a2f394fa49%40quicinc.com patch subject: [PATCH] bus: mhi: host: Add tracing support config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231006/202310060033.z0ojejxe-...@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231006/202310060033.z0ojejxe-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202310060033.z0ojejxe-...@intel.com/ All warnings (new ones prefixed by >>): drivers/bus/mhi/host/main.c: In function 'mhi_process_ctrl_ev_ring': >> drivers/bus/mhi/host/main.c:834:74: warning: cast from pointer to integer of >> different size [-Wpointer-to-int-cast] 834 | trace_mhi_process_ctrl_ev_ring(mhi_cntrl->mhi_dev->name, (u64)(local_rp), | ^ drivers/bus/mhi/host/main.c: In function 'mhi_gen_tre': drivers/bus/mhi/host/main.c:1245:69: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 1245 | trace_mhi_gen_tre(mhi_cntrl->mhi_dev->name, mhi_chan->chan, (u64)(mhi_tre), | ^ -- drivers/bus/mhi/host/pm.c: In function 'mhi_pm_st_worker': >> drivers/bus/mhi/host/pm.c:758:24: warning: unused variable 'dev' >> [-Wunused-variable] 758 | struct device *dev = _cntrl->mhi_dev->dev; |^~~ vim +834 drivers/bus/mhi/host/main.c 799 800 int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, 801 struct mhi_event *mhi_event, 802 u32 event_quota) 803 { 804 struct mhi_ring_element *dev_rp, *local_rp; 805 struct mhi_ring *ev_ring = _event->ring; 806 struct mhi_event_ctxt *er_ctxt = 807 _cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; 808 struct mhi_chan *mhi_chan; 809 struct device *dev = _cntrl->mhi_dev->dev; 810 u32 chan; 811 int count = 0; 812 dma_addr_t ptr = le64_to_cpu(er_ctxt->rp); 813 814 /* 815 * This is a quick check to avoid unnecessary event processing 816 * in case MHI is already in error state, but it's still possible 817 * to transition to error state while processing events 818 */ 819 if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) 820 return -EIO; 821 822 if (!is_valid_ring_ptr(ev_ring, ptr)) { 823 dev_err(_cntrl->mhi_dev->dev, 824 "Event ring rp points outside of the event ring\n"); 825 return -EIO; 826 } 827 828 dev_rp = mhi_to_virtual(ev_ring, ptr); 829 local_rp = ev_ring->rp; 830 831 while (dev_rp != local_rp) { 832 enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp); 833 > 834 > trace_mhi_process_ctrl_ev_ring(mhi_cntrl->mhi_dev->name, (u64)(local_rp), 835 local_rp->ptr, local_rp->dword[0], 836 local_rp->dword[1], 837 mhi_state_str(MHI_TRE_GET_EV_STATE(local_rp))); 838 839 switch (type) { 840 case MHI_PKT_TYPE_BW_REQ_EVENT: 841 { 842 struct mhi_link_info *link_info; 843 844 link_info = _cntrl->mhi_link_info; 845 write_lock_irq(_cntrl->pm_lock); 846 link_info->target_link_speed = 847 MHI_TRE_GET_EV_LINKSPEED(local_rp); 848 link_info->target_link_width = 849 MHI_TRE_GET_EV_LINKWIDTH(local_rp); 850 write_unlock_irq(_cntrl->pm_lock); 851 dev_dbg(dev, "Received BW_REQ event\n"); 852 m
[PATCH] bus: mhi: host: Add tracing support
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 Usage: echo 1 > /sys/kernel/debug/tracing/events/mhi_host/enable cat /sys/kernel/debug/tracing/trace Signed-off-by: Krishna chaitanya chundru --- MAINTAINERS | 1 + drivers/bus/mhi/host/init.c | 3 + drivers/bus/mhi/host/internal.h | 1 + drivers/bus/mhi/host/main.c | 27 -- drivers/bus/mhi/host/pm.c | 7 +- include/trace/events/mhi_host.h | 207 6 files changed, 234 insertions(+), 12 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 35977b269d5e..4339c668a6ab 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13862,6 +13862,7 @@ F: Documentation/mhi/ F: drivers/bus/mhi/ F: drivers/pci/endpoint/functions/pci-epf-mhi.c F: include/linux/mhi.h +F: include/trace/events/mhi_host.h MICROBLAZE ARCHITECTURE M: Michal Simek diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c index f78aefd2d7a3..3afa90a204fd 100644 --- a/drivers/bus/mhi/host/init.c +++ b/drivers/bus/mhi/host/init.c @@ -20,6 +20,9 @@ #include #include "internal.h" +#define CREATE_TRACE_POINTS +#include + static DEFINE_IDA(mhi_controller_ida); const char * const mhi_ee_str[MHI_EE_MAX] = { diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h index 2e139e76de4c..a80a317a59a9 100644 --- a/drivers/bus/mhi/host/internal.h +++ b/drivers/bus/mhi/host/internal.h @@ -7,6 +7,7 @@ #ifndef _MHI_INT_H #define _MHI_INT_H +#include #include "../common.h" extern struct bus_type mhi_bus_type; 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)); if (state == MHI_STATE_SYS_ERR) { dev_dbg(dev, "System error detected\n"); pm_state = mhi_tryset_pm_state(mhi_cntrl, @@ -832,6 +831,11 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, while (dev_rp != local_rp) { enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp); + trace_mhi_process_ctrl_ev_ring(mhi_cntrl->mhi_dev->name, (u64)(local_rp), + local_rp->ptr, local_rp->dword[0], + local_rp->dword[1], + mhi_state_str(MHI_TRE_GET_EV_STATE(local_rp))); + switch (type) { case MHI_PKT_TYPE_BW_REQ_EVENT: { @@ -997,6 +1001,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, while (dev_rp != local_rp && event_quota > 0) { enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp); + trace_mhi_process_data_event_ring(mhi_cntrl->mhi_dev->name, local_rp->ptr, + local_rp->dword[0], local_rp->dword[1]); + chan = MHI_TRE_GET_EV_CHID(local_rp); WARN_ON(chan >= mhi_cntrl->max_chan); @@ -1235,6 +1242,8 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan, mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(info->len); mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, eot, eob, chain); + trace_mhi_gen_tre(mhi_cntrl->mhi_dev->name, mhi_chan->chan, (u64)(mhi_tre), + mhi_tre->ptr, mhi_tre->dword[0], mhi_tre->dword[1]); /* increment WP */ mhi_add_ring_element(mhi_cntrl, tre_ring); mhi_add_ring_element(mhi_cntrl, buf_ring); @@ -1327,9 +1336,8 @@ static int mhi_update_channel_state(struct mhi_controller *mhi_cntrl, enum mhi_cmd_type cmd = MHI_CMD_NOP; int ret; - dev_dbg(dev, "%d: Updating channel state to: %s\n", mhi_chan->chan, - TO_CH_STATE_TYPE_STR(to_state)); - + trace_mhi_update_channel_state_start(mhi_cntrl->mhi_dev->name, mhi_chan->chan, +TO_CH_STATE_TYPE_STR(to_state)); switch (to_state) { case MHI_CH_STATE_TYPE_RESET: