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

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 10/23/2023 1:11 AM, Krishna Chaitanya Chundru wrote:
> 
> On 10/20/2023 8:33 PM, Jeffrey Hugo wrote:
>> On 10/13/2023 3:52 AM, Krishna chaitanya chundru wrote:
>>> 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 <quic_krichai@quicinc.com>
>>> ---
>>> 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 
>>>
>>> ---
>>>   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 <monstr@monstr.eu>
>>> 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.

> 
>>
>>> +
>>>   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.

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

> 
>>> +
>>>   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.

> 
>>
>>>       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.

> 
> 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().