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:
