Received-SPF: pass (google.com: domain of linux-kernel+bounces-164882-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 30 Apr 2024 19:59:26 -0700
From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Chris Lew <quic_clew@quicinc.com>
CC: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>, <andersson@kernel.org>,
        <mathieu.poirier@linaro.org>, <linux-kernel@vger.kernel.org>,
        <quic_deesin@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>
Subject: Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support
Message-ID: <ZjGwDhqqe/MCKcEF@hu-bjorande-lv.qualcomm.com>
References: <20240429075528.1723133-1-quic_sudeepgo@quicinc.com>
 <172ab19b-4643-6ac0-53ab-d2d1e217171c@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <172ab19b-4643-6ac0-53ab-d2d1e217171c@quicinc.com>

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