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
