On 10/27/2023 8:59 PM, Jeffrey Hugo wrote:

On 10/23/2023 1:11 AM, Krishna Chaitanya Chundru wrote:

quoted

On 10/20/2023 8:33 PM, Jeffrey Hugo wrote:

quoted

On 10/13/2023 3:52 AM, Krishna chaitanya chundru wrote:

quoted

This change adds ftrace support for following functions which
helps in debugging the issues when there is Channel state & MHI
state change and also when we receive data and control events:
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

Signed-off-by: Krishna chaitanya chundru <redacted>
---
Changes in v2:
- Passing the raw state into the trace event and using 
__print_symbolic() as suggested by bjorn.
- Change mhi_pm_st_worker to mhi_pm_st_transition as suggested by 
bjorn.
- Fixed the kernel test rebot issues.
- Link to v1: 
https://lore.kernel.org/r/20231005-ftrace_support-v1-1-23a2f394fa49@quicinc.com (local) 

---
  MAINTAINERS                     |   1 +
  drivers/bus/mhi/host/init.c     |   3 +
  drivers/bus/mhi/host/internal.h |   1 +
  drivers/bus/mhi/host/main.c     |  32 +++--
  drivers/bus/mhi/host/pm.c       |   6 +-
  include/trace/events/mhi_host.h | 287 
++++++++++++++++++++++++++++++++++++++++
  6 files changed, 317 insertions(+), 13 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 [off-list ref]

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 <linux/wait.h>
  #include "internal.h"
  +#define CREATE_TRACE_POINTS
+#include <trace/events/mhi_host.h>

This feels redundant to me.  A few lines ago we included internal.h, 
and internal.h includes trace/events/mhi_host.h

As Steve mentioned, this is mandatory step for creating trace points 
& trace events.

I understand this creates the trace points, and that needs to be done 
in C code.  It dtill seems redundant because we are including the 
header twice (and I am aware trace has the special multi-header read 
functionality for this).

The duplicate include still feels weird, but I have not come up with a 
better way to structure this.

We will use this way for now then, we will check in parallel if there 
is  a way to avoid this and change it in the future.

quoted

quoted

quoted

+
  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 <trace/events/mhi_host.h>
  #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..fcdb728ba49f 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c

@@ -246,6 +246,11 @@ static void *mhi_to_virtual(struct mhi_ring 

*ring, dma_addr_t addr)
      return (addr - ring->iommu_base) + ring->base;
  }
  +dma_addr_t mhi_to_physical(struct mhi_ring *ring, void *addr)
+{
+    return (addr - ring->base) + ring->iommu_base;
+}

This seems to be poorly named since we are using the iommu_base 
which suggests we are converting to an IOVA.

Why do we need this though?  This seems like it might be a security 
issue, or at the very least, not preferred, and I'm struggling to 
figure out what value this provides to you are I when looking at the 
log.

I will rename the function to reflect it is converting to IOVA.

We MHI TRE we write the IOVA address, to correlate between TRE events 
in the MHI ring and event we are processing  we want to log the IOVA 
address.

As we are logging only IOVA address which is provided in the 
devicetree and not the original physical address we are not expecting 
any security issues here.

Correct me if I was wrong.

The IOVA is not provided by DT, it is a runtime allocated value 
provided by the IOMMU, if present.  If not present, then it is a 
physical address.

Remember, x86 does not use devicetree.

While the IOVA (with an iommu) is not technically a physical address, 
but is treated as such by the device.  I can imagine an attacker doing 
bad things if they get a hold of the value.

Sure we will remove it.

Still, you haven't indicated why this is useful.

The TRE ring elements has address in the IOVA format when we want to 
correlate the address with the TRE elements in the dumps it will easier 
with this way.

Anyway we will not expose this as you suggested as it might expose 
physical address in some platforms.

quoted

quoted

quoted

+
  static void mhi_add_ring_element(struct mhi_controller *mhi_cntrl,
                   struct mhi_ring *ring)
  {

@@ -491,11 +496,9 @@ 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, 
mhi_cntrl->ee,
+                      mhi_cntrl->dev_state, ee, state);

Why are we removing the debug message when adding this trace? The 
commit text doesn't say.  (Looks like you do this several times, 
assume this comment applies to all isntances)

I will add this in the commit text in my next patch.

Just a query is recommended to keep both debug message and trace 
events. If yes we will not remove the debug messages.

I think it would be preferred to have one mechanism or the other, not 
both.  It seems like you are doing an incomplete conversion.

For now we will remove the dbg message where we added the traces and 
will mention that in the commit.

quoted

quoted

quoted

      if (state == MHI_STATE_SYS_ERR) {
          dev_dbg(dev, "System error detected\n");
          pm_state = mhi_tryset_pm_state(mhi_cntrl,

@@ -832,6 +835,12 @@ 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,
+                           mhi_to_physical(ev_ring, local_rp),
+                           local_rp->ptr, local_rp->dword[0],
+                           local_rp->dword[1],
+                           MHI_TRE_GET_EV_STATE(local_rp));

Why not just pass in the local_rp as a single parameter and have the 
trace implementation decode it?  (Looks like you do this several 
times, assume this comment applies to all isntances)

MHI_TRE_GET_EV_STATE definition is present in 
drivers/bus/mhi/common.h which is common for both EP & MHI driver.

If we keep this macro definition again in mhi_host.h it will be 
redundant one.

What is wrong with including the right header over in the trace to get 
the definition?  I didn't ask for it to be redefined.

If the struct definition for local_rp changes, it will probably break 
this, which will require changes to the definition and use of 
trace_mhi_process_ctrl_ev_ring().  If trace_mhi_process_ctrl_ev_ring() 
just takes the struct and decodes it, the decode logic just needs to 
be updated (in one place) when the struct definition changes.

quoted

And we are only using this way only for this trace log. So we are 
using the macro to get the state information.

No, you do the same thing for trace_mhi_process_data_event_ring() and 
trace_mhi_gen_tre().

Mani, can we move common.h to include  folder from the drivers folder so 
that we can include that file in the mhi_host.h for trace events?

- Krishna Chaitanya.