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

From: Deepak Kumar Singh
Date: Wed Jun 12 2024 - 05:13:01 EST




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