[PATCH V2 0/2] Add tracepoint support and remote name mapping to smp2p

[PATCH V2 0/2] Add tracepoint support and remote name mapping to smp2p

[Sudeepgoud Patil]

This commit introduces tracepoint support to smp2p module, enabling logging of communication
events between local and remote processors. The tracepoints capture essential details
such as remote processor ID, subsystem names, negotiation specifics, supported features,
bit changes, and subsystem restart (SSR) activity.
These tracepoints enhance debugging capabilities for inter-subsystem issues.

Addressing feedback from v1 to map remote PID (Process ID) along with the remote name
in tracepoints added new patch in V2 1/2, adding support to include the remote name
in the smp2p irq devname which fetches remote name from respective smp2p dtsi node,
which also makes the wakeup source distinguishable in irq wakeup prints.

Changes in v2:
- Added support to include the remote name in the smp2p IRQ devname, allowing for remote PID-name mapping
- Mapped the remote PID (Process ID) along with the remote name in tracepoints, as suggested by Chris
- Modified to capture all `out->features` instead of just the `ssr_ack`, following Chris's recommendation
- Expanded the commit description to provide additional context
- Link to v1: https://lore.kernel.org/all/20240429075528.1723133-1-quic_sudeepgo@quicinc.com

Sudeepgoud Patil (2):
  soc: qcom: smp2p: Add remote name into smp2p irq devname
  soc: qcom: smp2p: Introduce tracepoint support

 drivers/soc/qcom/Makefile      |   1 +
 drivers/soc/qcom/smp2p.c       |  26 +++++++-
 drivers/soc/qcom/trace-smp2p.h | 116 +++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/trace-smp2p.h

-- 

[PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Sudeepgoud Patil]

Add smp2p irq devname which fetches remote name from respective
smp2p dtsi node, which makes the wakeup source distinguishable
in irq wakeup prints.

Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
---
 drivers/soc/qcom/smp2p.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a21241cbeec7..a77fee048b38 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -122,6 +122,7 @@ struct smp2p_entry {
  * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
  * @ssr_ack: current cached state of the local ack bit
  * @negotiation_done: whether negotiating finished
+ * @irq_devname: poniter to the smp2p irq devname
  * @local_pid:	processor id of the inbound edge
  * @remote_pid:	processor id of the outbound edge
  * @ipc_regmap:	regmap for the outbound ipc
@@ -146,6 +147,7 @@ struct qcom_smp2p {
 	bool ssr_ack;
 	bool negotiation_done;
 
+	char *irq_devname;
 	unsigned local_pid;
 	unsigned remote_pid;
 
@@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 	/* Kick the outgoing edge after allocating entries */
 	qcom_smp2p_kick(smp2p);
 
+	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);
+	if (!smp2p->irq_devname) {
+		ret = -ENOMEM;
+		goto unwind_interfaces;
+	}
+
 	ret = devm_request_threaded_irq(&pdev->dev, irq,
 					NULL, qcom_smp2p_intr,
 					IRQF_ONESHOT,
-					"smp2p", (void *)smp2p);
+					smp2p->irq_devname, (void *)smp2p);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request interrupt\n");
 		goto unwind_interfaces;
@@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 	list_for_each_entry(entry, &smp2p->outbound, node)
 		qcom_smem_state_unregister(entry->state);
 
+	kfree(smp2p->irq_devname);
+
 	smp2p->out->valid_entries = 0;
 
 release_mbox:
@@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
 
 	mbox_free_channel(smp2p->mbox_chan);
 
+	kfree(smp2p->irq_devname);
+
 	smp2p->out->valid_entries = 0;
 }
 
-- 

Re: [PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Bjorn Andersson]

On Tue, Jun 11, 2024 at 06:03:50PM +0530, Sudeepgoud Patil wrote:
> Add smp2p irq devname which fetches remote name from respective
> smp2p dtsi node, which makes the wakeup source distinguishable
> in irq wakeup prints.
> 
> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
> ---
>  drivers/soc/qcom/smp2p.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index a21241cbeec7..a77fee048b38 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -122,6 +122,7 @@ struct smp2p_entry {
>   * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
>   * @ssr_ack: current cached state of the local ack bit
>   * @negotiation_done: whether negotiating finished
> + * @irq_devname: poniter to the smp2p irq devname
>   * @local_pid:	processor id of the inbound edge
>   * @remote_pid:	processor id of the outbound edge
>   * @ipc_regmap:	regmap for the outbound ipc
> @@ -146,6 +147,7 @@ struct qcom_smp2p {
>  	bool ssr_ack;
>  	bool negotiation_done;
>  
> +	char *irq_devname;
>  	unsigned local_pid;
>  	unsigned remote_pid;
>  
> @@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  	/* Kick the outgoing edge after allocating entries */
>  	qcom_smp2p_kick(smp2p);
>  
> +	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);

That's a lot of extra instructions for copying a string, which doesn't
need to be copied because of_node->name is const char and the argument
to devm_request_threaded_irq() is const char.

So, kstrdup_const() is what you're looking for.

You can then go devm_kstrdup_const() and avoid the kfree() (then
kfree_const()) below.


That said, looking at /proc/interrupts, I think it would make sense to
make this devm_kasprintf(..., "smp2p-%s", name);

Regards,
Bjorn

> +	if (!smp2p->irq_devname) {
> +		ret = -ENOMEM;
> +		goto unwind_interfaces;
> +	}
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, irq,
>  					NULL, qcom_smp2p_intr,
>  					IRQF_ONESHOT,
> -					"smp2p", (void *)smp2p);
> +					smp2p->irq_devname, (void *)smp2p);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request interrupt\n");
>  		goto unwind_interfaces;
> @@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  	list_for_each_entry(entry, &smp2p->outbound, node)
>  		qcom_smem_state_unregister(entry->state);
>  
> +	kfree(smp2p->irq_devname);
> +
>  	smp2p->out->valid_entries = 0;
>  
>  release_mbox:
> @@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
>  
>  	mbox_free_channel(smp2p->mbox_chan);
>  
> +	kfree(smp2p->irq_devname);
> +
>  	smp2p->out->valid_entries = 0;
>  }
>  
> -- 
> 

Re: [PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Chris Lew]

On 6/11/2024 9:06 AM, Bjorn Andersson wrote:
> On Tue, Jun 11, 2024 at 06:03:50PM +0530, Sudeepgoud Patil wrote:
>> Add smp2p irq devname which fetches remote name from respective
>> smp2p dtsi node, which makes the wakeup source distinguishable
>> in irq wakeup prints.
>>
>> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
>> ---
>>   drivers/soc/qcom/smp2p.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
>> index a21241cbeec7..a77fee048b38 100644
>> --- a/drivers/soc/qcom/smp2p.c
>> +++ b/drivers/soc/qcom/smp2p.c
>> @@ -122,6 +122,7 @@ struct smp2p_entry {
>>    * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
>>    * @ssr_ack: current cached state of the local ack bit
>>    * @negotiation_done: whether negotiating finished
>> + * @irq_devname: poniter to the smp2p irq devname
>>    * @local_pid:	processor id of the inbound edge
>>    * @remote_pid:	processor id of the outbound edge
>>    * @ipc_regmap:	regmap for the outbound ipc
>> @@ -146,6 +147,7 @@ struct qcom_smp2p {
>>   	bool ssr_ack;
>>   	bool negotiation_done;
>>   
>> +	char *irq_devname;
>>   	unsigned local_pid;
>>   	unsigned remote_pid;
>>   
>> @@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>>   	/* Kick the outgoing edge after allocating entries */
>>   	qcom_smp2p_kick(smp2p);
>>   
>> +	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);
> 
> That's a lot of extra instructions for copying a string, which doesn't
> need to be copied because of_node->name is const char and the argument
> to devm_request_threaded_irq() is const char.
> 
> So, kstrdup_const() is what you're looking for.
> 
> You can then go devm_kstrdup_const() and avoid the kfree() (then
> kfree_const()) below.
> 
> 
> That said, looking at /proc/interrupts, I think it would make sense to
> make this devm_kasprintf(..., "smp2p-%s", name);
> 

Is it ok to rely on the "of_node->name"? I think device tree tends to 
always have the node name as "smp2p-%s" already, so ("smp2p-%s", name) 
would result in "smp2p-smp2p-adsp".

Also Sudeepgoud, I think this will update the irqname in 
/proc/interrupts for the ipcc irqchip entry. It would also be helpful if 
we could differentiate the instances of smp2p irqchips as well. That way 
we can see what processors the 'ready' and 'fatal' interrupts apply to 
in /proc/interrupts.

Can you refer to my internal patch that adds .irq_print_chip() and 
incorporate those changes here?

> Regards,
> Bjorn
> 
>> +	if (!smp2p->irq_devname) {
>> +		ret = -ENOMEM;
>> +		goto unwind_interfaces;
>> +	}
>> +
>>   	ret = devm_request_threaded_irq(&pdev->dev, irq,
>>   					NULL, qcom_smp2p_intr,
>>   					IRQF_ONESHOT,
>> -					"smp2p", (void *)smp2p);
>> +					smp2p->irq_devname, (void *)smp2p);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "failed to request interrupt\n");
>>   		goto unwind_interfaces;
>> @@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>>   	list_for_each_entry(entry, &smp2p->outbound, node)
>>   		qcom_smem_state_unregister(entry->state);
>>   
>> +	kfree(smp2p->irq_devname);
>> +
>>   	smp2p->out->valid_entries = 0;
>>   
>>   release_mbox:
>> @@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
>>   
>>   	mbox_free_channel(smp2p->mbox_chan);
>>   
>> +	kfree(smp2p->irq_devname);
>> +
>>   	smp2p->out->valid_entries = 0;
>>   }
>>   
>> -- 
>>

Re: [PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Bjorn Andersson]

On Tue, Jun 11, 2024 at 10:53:01AM -0700, Chris Lew wrote:
> 
> 
> On 6/11/2024 9:06 AM, Bjorn Andersson wrote:
> > On Tue, Jun 11, 2024 at 06:03:50PM +0530, Sudeepgoud Patil wrote:
> > > Add smp2p irq devname which fetches remote name from respective
> > > smp2p dtsi node, which makes the wakeup source distinguishable
> > > in irq wakeup prints.
> > > 
> > > Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
> > > ---
> > >   drivers/soc/qcom/smp2p.c | 14 +++++++++++++-
> > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> > > index a21241cbeec7..a77fee048b38 100644
> > > --- a/drivers/soc/qcom/smp2p.c
> > > +++ b/drivers/soc/qcom/smp2p.c
> > > @@ -122,6 +122,7 @@ struct smp2p_entry {
> > >    * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
> > >    * @ssr_ack: current cached state of the local ack bit
> > >    * @negotiation_done: whether negotiating finished
> > > + * @irq_devname: poniter to the smp2p irq devname
> > >    * @local_pid:	processor id of the inbound edge
> > >    * @remote_pid:	processor id of the outbound edge
> > >    * @ipc_regmap:	regmap for the outbound ipc
> > > @@ -146,6 +147,7 @@ struct qcom_smp2p {
> > >   	bool ssr_ack;
> > >   	bool negotiation_done;
> > > +	char *irq_devname;
> > >   	unsigned local_pid;
> > >   	unsigned remote_pid;
> > > @@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> > >   	/* Kick the outgoing edge after allocating entries */
> > >   	qcom_smp2p_kick(smp2p);
> > > +	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);
> > 
> > That's a lot of extra instructions for copying a string, which doesn't
> > need to be copied because of_node->name is const char and the argument
> > to devm_request_threaded_irq() is const char.
> > 
> > So, kstrdup_const() is what you're looking for.
> > 
> > You can then go devm_kstrdup_const() and avoid the kfree() (then
> > kfree_const()) below.
> > 
> > 
> > That said, looking at /proc/interrupts, I think it would make sense to
> > make this devm_kasprintf(..., "smp2p-%s", name);
> > 
> 
> Is it ok to rely on the "of_node->name"? I think device tree tends to always
> have the node name as "smp2p-%s" already, so ("smp2p-%s", name) would result
> in "smp2p-smp2p-adsp".
> 

You're right, I forgot about that.

This actually means that if we replace "smp2p" with NULL, we should get
the descriptive names we're looking for automagically (as the node name
is used to build dev_name()).

> Also Sudeepgoud, I think this will update the irqname in /proc/interrupts
> for the ipcc irqchip entry. It would also be helpful if we could
> differentiate the instances of smp2p irqchips as well. That way we can see
> what processors the 'ready' and 'fatal' interrupts apply to in
> /proc/interrupts.
> 

But this would be a change on the consumer side, right? To replace the
"q6v5" that we have hard coded for all the PAS remoteproc instances.

I'd be happy to see such change.

Regards,
Bjorn

> Can you refer to my internal patch that adds .irq_print_chip() and
> incorporate those changes here?
> 
> > Regards,
> > Bjorn
> > 
> > > +	if (!smp2p->irq_devname) {
> > > +		ret = -ENOMEM;
> > > +		goto unwind_interfaces;
> > > +	}
> > > +
> > >   	ret = devm_request_threaded_irq(&pdev->dev, irq,
> > >   					NULL, qcom_smp2p_intr,
> > >   					IRQF_ONESHOT,
> > > -					"smp2p", (void *)smp2p);
> > > +					smp2p->irq_devname, (void *)smp2p);
> > >   	if (ret) {
> > >   		dev_err(&pdev->dev, "failed to request interrupt\n");
> > >   		goto unwind_interfaces;
> > > @@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> > >   	list_for_each_entry(entry, &smp2p->outbound, node)
> > >   		qcom_smem_state_unregister(entry->state);
> > > +	kfree(smp2p->irq_devname);
> > > +
> > >   	smp2p->out->valid_entries = 0;
> > >   release_mbox:
> > > @@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
> > >   	mbox_free_channel(smp2p->mbox_chan);
> > > +	kfree(smp2p->irq_devname);
> > > +
> > >   	smp2p->out->valid_entries = 0;
> > >   }
> > > -- 
> > > 

[PATCH V2 2/2] soc: qcom: smp2p: Introduce tracepoint support

[Sudeepgoud Patil]

This commit introduces tracepoint support for smp2p,
enabling logging of communication between local and remote processors.
The tracepoints include information about the remote processor ID,
remote subsystem name, negotiation details, supported features,
bit change notifications, and ssr activity.
These tracepoints are valuable for debugging issues between subsystems.

Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
---
 drivers/soc/qcom/Makefile      |   1 +
 drivers/soc/qcom/smp2p.c       |  12 ++++
 drivers/soc/qcom/trace-smp2p.h | 116 +++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/soc/qcom/trace-smp2p.h

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ca0bece0dfff..30c1bf645501 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -23,6 +23,7 @@ qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= rpm-proc.o smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
+CFLAGS_smp2p.o := -I$(src)
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a77fee048b38..6eab8ff55691 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
@@ -193,6 +196,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->irq_devname);
 	smp2p->ssr_ack = !smp2p->ssr_ack;
 
 	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
@@ -215,6 +219,8 @@ 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->irq_devname,
+				out->features);
 	}
 }
 
@@ -253,6 +259,9 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 		status = val ^ entry->last_value;
 		entry->last_value = val;
 
+		trace_smp2p_notify_in(smp2p->remote_pid, smp2p->irq_devname,
+				entry->name, status, val);
+
 		/* No changes of this entry? */
 		if (!status)
 			continue;
@@ -408,6 +417,9 @@ static int smp2p_update_bits(void *data, u32 mask, u32 value)
 	writel(val, entry->value);
 	spin_unlock_irqrestore(&entry->lock, flags);
 
+	trace_smp2p_update_bits(entry->smp2p->remote_pid, entry->smp2p->irq_devname,
+		entry->name, orig, val);
+
 	if (val != orig)
 		qcom_smp2p_kick(entry->smp2p);
 
diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
new file mode 100644
index 000000000000..833782460b57
--- /dev/null
+++ b/drivers/soc/qcom/trace-smp2p.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qcom_smp2p
+
+#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __QCOM_SMP2P_TRACE_H__
+
+#include <linux/tracepoint.h>
+
+#define SMP2P_FEATURE_SSR_ACK 0x1
+
+TRACE_EVENT(smp2p_ssr_ack,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname),
+	TP_ARGS(remote_pid, irq_devname),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+	),
+	TP_printk("%d: %s: SSR detected, doing SSR Handshake",
+		__entry->remote_pid,
+		__get_str(irq_devname)
+	)
+);
+
+TRACE_EVENT(smp2p_negotiate,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname, unsigned int features),
+	TP_ARGS(remote_pid, irq_devname, features),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__field(u32, out_features)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__entry->out_features = features;
+	),
+	TP_printk("%d: %s: state=open out_features=%s",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__print_flags(__entry->out_features, "|",
+			{SMP2P_FEATURE_SSR_ACK, "SMP2P_FEATURE_SSR_ACK"})
+	)
+);
+
+TRACE_EVENT(smp2p_notify_in,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname,
+		 const char *name, unsigned long status, u32 val),
+	TP_ARGS(remote_pid, irq_devname, name, status, val),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__string(name, name)
+		__field(unsigned long, status)
+		__field(u32, val)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__assign_str(name, name);
+		__entry->status = status;
+		__entry->val = val;
+	),
+	TP_printk("%d: %s: %s: status:0x%0lx val:0x%0x",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__get_str(name),
+		__entry->status,
+		__entry->val
+	)
+);
+
+TRACE_EVENT(smp2p_update_bits,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname,
+		 const char *name, u32 orig, u32 val),
+	TP_ARGS(remote_pid, irq_devname, name, orig, val),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__string(name, name)
+		__field(u32, orig)
+		__field(u32, val)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__assign_str(name, name);
+		__entry->orig = orig;
+		__entry->val = val;
+	),
+	TP_printk("%d: %s: %s: orig:0x%0x new:0x%0x",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__get_str(name),
+		__entry->orig,
+		__entry->val
+	)
+);
+
+#endif /* __QCOM_SMP2P_TRACE_H__ */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace-smp2p
+
+#include <trace/define_trace.h>
-- 

Re: [PATCH V2 2/2] soc: qcom: smp2p: Introduce tracepoint support

[Chris Lew]

On 6/11/2024 5:33 AM, Sudeepgoud Patil wrote:
> This commit introduces tracepoint support for smp2p,
> enabling logging of communication between local and remote processors.
> The tracepoints include information about the remote processor ID,
> remote subsystem name, negotiation details, supported features,
> bit change notifications, and ssr activity.
> These tracepoints are valuable for debugging issues between subsystems.
> 
> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
> ---
...
> diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
> new file mode 100644
> index 000000000000..833782460b57
> --- /dev/null
> +++ b/drivers/soc/qcom/trace-smp2p.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM qcom_smp2p
> +
> +#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
> +#define __QCOM_SMP2P_TRACE_H__
> +
> +#include <linux/tracepoint.h>
> +
> +#define SMP2P_FEATURE_SSR_ACK 0x1

Now that I see it, redefining the the feature flag here seems a bit out 
of place. I'm not sure if it's worth kicking off a header file for this 
single define though.

> +
> +TRACE_EVENT(smp2p_ssr_ack,
> +	TP_PROTO(unsigned int remote_pid, char *irq_devname),
> +	TP_ARGS(remote_pid, irq_devname),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +		__string(irq_devname, irq_devname)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +		__assign_str(irq_devname, irq_devname);
> +	),
> +	TP_printk("%d: %s: SSR detected, doing SSR Handshake",
> +		__entry->remote_pid,
> +		__get_str(irq_devname)
> +	)
> +);
> +

I don't think we need to pass remote_pid into all of the traces if we 
have a unique name "irq_devname" to identify the remote now. We could 
remove remote_pid from all the trace event arguments.

We can probably drop the "doing SSR Handshake" part of this print. I 
think it can be assumed that we're doing the handshake once we've 
detected SSR.

Re: [PATCH V2 2/2] soc: qcom: smp2p: Introduce tracepoint support

[Deepak Kumar Singh]

On 6/12/2024 4:35 AM, Chris Lew wrote:
> 
> 
> On 6/11/2024 5:33 AM, Sudeepgoud Patil wrote:
>> This commit introduces tracepoint support for smp2p,
>> enabling logging of communication between local and remote processors.
>> The tracepoints include information about the remote processor ID,
>> remote subsystem name, negotiation details, supported features,
>> bit change notifications, and ssr activity.
>> These tracepoints are valuable for debugging issues between subsystems.
>>
>> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
>> ---
> ...
>> diff --git a/drivers/soc/qcom/trace-smp2p.h 
>> b/drivers/soc/qcom/trace-smp2p.h
>> new file mode 100644
>> index 000000000000..833782460b57
>> --- /dev/null
>> +++ b/drivers/soc/qcom/trace-smp2p.h
>> @@ -0,0 +1,116 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM qcom_smp2p
>> +
>> +#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
>> +#define __QCOM_SMP2P_TRACE_H__
>> +
>> +#include <linux/tracepoint.h>
>> +
>> +#define SMP2P_FEATURE_SSR_ACK 0x1
> 
> Now that I see it, redefining the the feature flag here seems a bit out 
> of place. I'm not sure if it's worth kicking off a header file for this 
> single define though.
> 
I think it is ok to have this define in smp2p.c, as that is the only 
place where it is being used.
>> +
>> +TRACE_EVENT(smp2p_ssr_ack,
>> +    TP_PROTO(unsigned int remote_pid, char *irq_devname),
>> +    TP_ARGS(remote_pid, irq_devname),
>> +    TP_STRUCT__entry(
>> +        __field(u32, remote_pid)
>> +        __string(irq_devname, irq_devname)
>> +    ),
>> +    TP_fast_assign(
>> +        __entry->remote_pid = remote_pid;
>> +        __assign_str(irq_devname, irq_devname);
>> +    ),
>> +    TP_printk("%d: %s: SSR detected, doing SSR Handshake",
>> +        __entry->remote_pid,
>> +        __get_str(irq_devname)
>> +    )
>> +);
>> +
> 
> I don't think we need to pass remote_pid into all of the traces if we 
> have a unique name "irq_devname" to identify the remote now. We could 
> remove remote_pid from all the trace event arguments.
> 
> We can probably drop the "doing SSR Handshake" part of this print. I 
> think it can be assumed that we're doing the handshake once we've 
> detected SSR.