Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support

[Bjorn Andersson]

On Tue, Apr 30, 2024 at 04:18:27PM -0700, Chris Lew wrote:
> On 4/29/2024 12:55 AM, Sudeepgoud Patil wrote:
> > Introduce tracepoint support for smp2p providing useful logging
> > for communication between clients.
> > 
> 
> Let's add some more description as to why these tracepoint are useful. Do
> they help us track latency? debugging information for us? for clients?

+1

> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
[..]
> > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> > index a21241cbeec7..dde8513641ae 100644
> > --- a/drivers/soc/qcom/smp2p.c
> > +++ b/drivers/soc/qcom/smp2p.c
> > @@ -20,6 +20,9 @@
> >   #include <linux/soc/qcom/smem_state.h>
> >   #include <linux/spinlock.h>
> > +#define CREATE_TRACE_POINTS
> > +#include "trace-smp2p.h"
> > +
> >   /*
> >    * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
> >    * of a single 32-bit value between two processors.  Each value has a single
> > @@ -191,6 +194,7 @@ static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
> >   	struct smp2p_smem_item *out = smp2p->out;
> >   	u32 val;
> > +	trace_smp2p_ssr_ack(smp2p->remote_pid);
> >   	smp2p->ssr_ack = !smp2p->ssr_ack;
> >   	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
> > @@ -213,6 +217,7 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
> >   			smp2p->ssr_ack_enabled = true;
> >   		smp2p->negotiation_done = true;
> > +		trace_smp2p_negotiate(smp2p->remote_pid, smp2p->ssr_ack_enabled);
> 
> since this tracepoint is for negotiating, maybe we should capture all of the
> features (out->features) instead of just the ssr_ack feature.
> 

Perhaps we can use __print_flags() in TP_printk() for that, it will
attempt to resolve the bits and if it fails include the numeric value.

> >   	}
> >   }
[..]
> > diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
[..]
> > +TRACE_EVENT(smp2p_ssr_ack,
> > +	TP_PROTO(unsigned int remote_pid),
> 
> Is there any way we can map the remote pid's to a string? I feel like the
> tracepoints would be more useful if they called out modem, adsp, etc instead
> of an integer value.
> 

And __print_symbolic() for this one.

Regards,
Bjorn