[PATCH v5 0/5] Add per-transport SCMI communication debug statistics

[PATCH v5 0/5] Add per-transport SCMI communication debug statistics

[Sudeep Holla]

This series adds support for SCMI communication debug metrict tracking.
I am just sending on behalf of Luke with minor reworks in his absense.

Cristian,

I have retained your review tags, please shout if you disagree.

Regards,
Sudeep

V4->V5
- Updated/improved some patch titles and commit messages
- Moved all debugs strings to an array and changed the creations of the
  same into a for loop
- Minor changes to the debugfs file names itself to improve readability
V3->V4
- Rename to counters rather than statistics to reflect the scmi protocol better
- Use basic writing rather than custom function on debugfs in patch 5
V2->V3
- Switch statistic counters to an array to store statistics.
- Add more statistics
- Add the ability to reset statistics, both individually and all
V1->V2
- Add a minor fix removing an unneeded call to handle_to_scmi_info
- Use new scmi_log_stats op/no-op rather than if(IS_ENABLED)
- Drop unneeded atomic_set's
- Use a helper function for stats debugfs creation

Luke Parkin (5):
  firmware: arm_scmi: Remove superfluous handle_to_scmi_info
  firmware: arm_scmi: Add support for debug metrics at the interface
  firmware: arm_scmi: Track basic SCMI communication debug metrics
  firmware: arm_scmi: Create debugfs files for SCMI communication debug
    metrics
  firmware: arm_scmi: Add support to reset the debug metrics

 drivers/firmware/arm_scmi/Kconfig  | 14 +++++
 drivers/firmware/arm_scmi/common.h | 24 +++++++++
 drivers/firmware/arm_scmi/driver.c | 83 +++++++++++++++++++++++++++---
 3 files changed, 113 insertions(+), 8 deletions(-)

-- 
2.46.0

[PATCH v5 1/5] firmware: arm_scmi: Remove superfluous handle_to_scmi_info

[Sudeep Holla]

From: Luke Parkin <luke.parkin@arm.com>

Variable info is already defined in the outer code block and there is no
need to define the same again in the inner code block. Let us just remove
that duplicate definition of the variable info. No functional change.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6b6957f4743f..56a93d20bf23 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1261,9 +1261,6 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 					    xfer->rx.buf, xfer->rx.len);
 
 			if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
-				struct scmi_info *info =
-					handle_to_scmi_info(cinfo->handle);
-
 				scmi_raw_message_report(info->raw, xfer,
 							SCMI_RAW_REPLY_QUEUE,
 							cinfo->id);
-- 
2.46.0

[PATCH v5 2/5] firmware: arm_scmi: Add support for debug metrics at the interface

[Sudeep Holla]

From: Luke Parkin <luke.parkin@arm.com>

Since SCMI involves interaction with the entity(software, firmware and/or
hardware) providing services or features, it is quite useful to track
certain metrics(for pure debugging purposes) like how many messages were
sent or received, were there any failures, what kind of failures, ..etc.

Add a new optional config option for the above purpose and the initial
support for counting such key debug metrics.

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/Kconfig  | 14 ++++++++++++++
 drivers/firmware/arm_scmi/common.h | 10 ++++++++++
 drivers/firmware/arm_scmi/driver.c |  2 ++
 3 files changed, 26 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..f35784d0a8dd 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -55,6 +55,20 @@ config ARM_SCMI_RAW_MODE_SUPPORT_COEX
 	  operate normally, thing which could make an SCMI test suite using the
 	  SCMI Raw mode support unreliable. If unsure, say N.
 
+config ARM_SCMI_DEBUG_COUNTERS
+	bool "Enable SCMI communication debug metrics tracking"
+	select ARM_SCMI_NEED_DEBUGFS
+	depends on DEBUG_FS
+	default n
+	help
+	  Enables tracking of some key communication metrics for debug
+	  purposes. It may track metrics like how many messages were sent
+	  or received, were there any failures, what kind of failures, ..etc.
+
+	  Enable this option to create a new debugfs directory which contains
+	  such useful debug counters. This can be helpful for debugging and
+	  SCMI monitoring.
+
 config ARM_SCMI_HAVE_TRANSPORT
 	bool
 	help
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 4b8c5250cdb5..d414b0a43b3b 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -301,6 +301,16 @@ extern const struct scmi_desc scmi_optee_desc;
 
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
 
+enum debug_counters {
+	SCMI_DEBUG_COUNTERS_LAST
+};
+
+static inline void scmi_inc_count(atomic_t *arr, int stat)
+{
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_COUNTERS))
+		atomic_inc(&arr[stat]);
+}
+
 enum scmi_bad_msg {
 	MSG_UNEXPECTED = -1,
 	MSG_INVALID = -2,
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 56a93d20bf23..958b2ac92050 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -117,12 +117,14 @@ struct scmi_protocol_instance {
  * @name: Name of this SCMI instance
  * @type: Type of this SCMI instance
  * @is_atomic: Flag to state if the transport of this instance is atomic
+ * @counters: An array of atomic_c's used for tracking statistics (if enabled)
  */
 struct scmi_debug_info {
 	struct dentry *top_dentry;
 	const char *name;
 	const char *type;
 	bool is_atomic;
+	atomic_t counters[SCMI_DEBUG_COUNTERS_LAST];
 };
 
 /**
-- 
2.46.0

[PATCH v5 3/5] firmware: arm_scmi: Track basic SCMI communication debug metrics

[Sudeep Holla]

From: Luke Parkin <luke.parkin@arm.com>

Add the support for counting some of the SCMI communication debug metrics
like how many were sent successfully or with some errors, responses
received, notifications and delayed responses, transfer timeouts and
errors from the firmware/platform.

In many cases, the traces exists. But the traces are not always necessarily
enabled and getting such cumulative SCMI communication debug metrics helps
in understanding if there are any possible improvements that can be made
on either side of SCMI communication.

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/common.h | 14 ++++++++++++++
 drivers/firmware/arm_scmi/driver.c | 25 ++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index d414b0a43b3b..21c94deaf614 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -302,6 +302,20 @@ extern const struct scmi_desc scmi_optee_desc;
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
 
 enum debug_counters {
+	SENT_OK,
+	SENT_FAIL,
+	SENT_FAIL_POLLING_UNSUPPORTED,
+	SENT_FAIL_CHANNEL_NOT_FOUND,
+	RESPONSE_OK,
+	NOTIFICATION_OK,
+	DELAYED_RESPONSE_OK,
+	XFERS_RESPONSE_TIMEOUT,
+	XFERS_RESPONSE_POLLED_TIMEOUT,
+	RESPONSE_POLLED_OK,
+	ERR_MSG_UNEXPECTED,
+	ERR_MSG_INVALID,
+	ERR_MSG_NOMEM,
+	ERR_PROTOCOL,
 	SCMI_DEBUG_COUNTERS_LAST
 };
 
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 958b2ac92050..943ba03315de 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -990,6 +990,7 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
 		spin_unlock_irqrestore(&minfo->xfer_lock, flags);
 
 		scmi_bad_message_trace(cinfo, msg_hdr, MSG_UNEXPECTED);
+		scmi_inc_count(info->dbg->counters, ERR_MSG_UNEXPECTED);
 
 		return xfer;
 	}
@@ -1017,6 +1018,8 @@ scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
 			msg_type, xfer_id, msg_hdr, xfer->state);
 
 		scmi_bad_message_trace(cinfo, msg_hdr, MSG_INVALID);
+		scmi_inc_count(info->dbg->counters, ERR_MSG_INVALID);
+
 
 		/* On error the refcount incremented above has to be dropped */
 		__scmi_xfer_put(minfo, xfer);
@@ -1056,6 +1059,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
 			PTR_ERR(xfer));
 
 		scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
+		scmi_inc_count(info->dbg->counters, ERR_MSG_NOMEM);
 
 		scmi_clear_channel(info, cinfo);
 		return;
@@ -1071,6 +1075,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
 	trace_scmi_msg_dump(info->id, cinfo->id, xfer->hdr.protocol_id,
 			    xfer->hdr.id, "NOTI", xfer->hdr.seq,
 			    xfer->hdr.status, xfer->rx.buf, xfer->rx.len);
+	scmi_inc_count(info->dbg->counters, NOTIFICATION_OK);
 
 	scmi_notify(cinfo->handle, xfer->hdr.protocol_id,
 		    xfer->hdr.id, xfer->rx.buf, xfer->rx.len, ts);
@@ -1130,8 +1135,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 	if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) {
 		scmi_clear_channel(info, cinfo);
 		complete(xfer->async_done);
+		scmi_inc_count(info->dbg->counters, DELAYED_RESPONSE_OK);
 	} else {
 		complete(&xfer->done);
+		scmi_inc_count(info->dbg->counters, RESPONSE_OK);
 	}
 
 	if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
@@ -1215,6 +1222,7 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 			       struct scmi_xfer *xfer, unsigned int timeout_ms)
 {
 	int ret = 0;
+	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
 
 	if (xfer->hdr.poll_completion) {
 		/*
@@ -1235,13 +1243,12 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 					"timed out in resp(caller: %pS) - polling\n",
 					(void *)_RET_IP_);
 				ret = -ETIMEDOUT;
+				scmi_inc_count(info->dbg->counters, XFERS_RESPONSE_POLLED_TIMEOUT);
 			}
 		}
 
 		if (!ret) {
 			unsigned long flags;
-			struct scmi_info *info =
-				handle_to_scmi_info(cinfo->handle);
 
 			/*
 			 * Do not fetch_response if an out-of-order delayed
@@ -1261,6 +1268,7 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 					    "RESP" : "resp",
 					    xfer->hdr.seq, xfer->hdr.status,
 					    xfer->rx.buf, xfer->rx.len);
+			scmi_inc_count(info->dbg->counters, RESPONSE_POLLED_OK);
 
 			if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
 				scmi_raw_message_report(info->raw, xfer,
@@ -1275,6 +1283,7 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 			dev_err(dev, "timed out in resp(caller: %pS)\n",
 				(void *)_RET_IP_);
 			ret = -ETIMEDOUT;
+			scmi_inc_count(info->dbg->counters, XFERS_RESPONSE_TIMEOUT);
 		}
 	}
 
@@ -1358,13 +1367,15 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	    !is_transport_polling_capable(info->desc)) {
 		dev_warn_once(dev,
 			      "Polling mode is not supported by transport.\n");
+		scmi_inc_count(info->dbg->counters, SENT_FAIL_POLLING_UNSUPPORTED);
 		return -EINVAL;
 	}
 
 	cinfo = idr_find(&info->tx_idr, pi->proto->id);
-	if (unlikely(!cinfo))
+	if (unlikely(!cinfo)) {
+		scmi_inc_count(info->dbg->counters, SENT_FAIL_CHANNEL_NOT_FOUND);
 		return -EINVAL;
-
+	}
 	/* True ONLY if also supported by transport. */
 	if (is_polling_enabled(cinfo, info->desc))
 		xfer->hdr.poll_completion = true;
@@ -1396,16 +1407,20 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	ret = info->desc->ops->send_message(cinfo, xfer);
 	if (ret < 0) {
 		dev_dbg(dev, "Failed to send message %d\n", ret);
+		scmi_inc_count(info->dbg->counters, SENT_FAIL);
 		return ret;
 	}
 
 	trace_scmi_msg_dump(info->id, cinfo->id, xfer->hdr.protocol_id,
 			    xfer->hdr.id, "CMND", xfer->hdr.seq,
 			    xfer->hdr.status, xfer->tx.buf, xfer->tx.len);
+	scmi_inc_count(info->dbg->counters, SENT_OK);
 
 	ret = scmi_wait_for_message_response(cinfo, xfer);
-	if (!ret && xfer->hdr.status)
+	if (!ret && xfer->hdr.status) {
 		ret = scmi_to_linux_errno(xfer->hdr.status);
+		scmi_inc_count(info->dbg->counters, ERR_PROTOCOL);
+	}
 
 	if (info->desc->ops->mark_txdone)
 		info->desc->ops->mark_txdone(cinfo, ret, xfer);
-- 
2.46.0

[PATCH v5 4/5] firmware: arm_scmi: Create debugfs files for SCMI communication debug metrics

[Sudeep Holla]

From: Luke Parkin <luke.parkin@arm.com>

Now that the basic support to collect the SCMI communication debug
metrics is in place, let us create debugfs files for the same so
that they are accessible to the users/debuggers.

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 943ba03315de..ade32a67ab63 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2847,6 +2847,36 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static const char * const dbg_counter_strs[] = {
+	"sent_ok",
+	"sent_fail",
+	"sent_fail_polling_unsupported",
+	"sent_fail_channel_not_found",
+	"response_ok",
+	"notification_ok",
+	"delayed_response_ok",
+	"xfers_response_timeout",
+	"xfers_response_polled_timeout",
+	"response_polled_ok",
+	"err_msg_unexpected",
+	"err_msg_invalid",
+	"err_msg_nomem",
+	"err_protocol",
+};
+
+static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
+					struct dentry *trans)
+{
+	struct dentry *counters;
+	int idx;
+
+	counters = debugfs_create_dir("counters", trans);
+
+	for (idx = 0; idx < SCMI_DEBUG_COUNTERS_LAST; idx++)
+		debugfs_create_atomic_t(dbg_counter_strs[idx], 0400, counters,
+					&dbg->counters[idx]);
+}
+
 static void scmi_debugfs_common_cleanup(void *d)
 {
 	struct scmi_debug_info *dbg = d;
@@ -2913,6 +2943,9 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
 	debugfs_create_u32("rx_max_msg", 0400, trans,
 			   (u32 *)&info->rx_minfo.max_msg);
 
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_COUNTERS))
+		scmi_debugfs_counters_setup(dbg, trans);
+
 	dbg->top_dentry = top_dentry;
 
 	if (devm_add_action_or_reset(info->dev,
-- 
2.46.0

[PATCH v5 5/5] firmware: arm_scmi: Add support to reset the debug metrics

[Sudeep Holla]

From: Luke Parkin <luke.parkin@arm.com>

It is sometimes useful to reset all these SCMI communication debug
metrics especially when we are interested in analysing these metrics
during a particular workload or for a fixed time duration. Let us
add the capability to reset all these metrics as once so that they
can be counted during the period of interest.

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ade32a67ab63..ca910079d718 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2864,6 +2864,24 @@ static const char * const dbg_counter_strs[] = {
 	"err_protocol",
 };
 
+static ssize_t reset_all_on_write(struct file *filp, const char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct scmi_debug_info *dbg = filp->private_data;
+
+	for (int i = 0; i < SCMI_DEBUG_COUNTERS_LAST; i++)
+		atomic_set(&dbg->counters[i], 0);
+
+	return count;
+}
+
+static const struct file_operations fops_reset_counts = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.llseek = no_llseek,
+	.write = reset_all_on_write,
+};
+
 static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
 					struct dentry *trans)
 {
@@ -2873,8 +2891,10 @@ static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
 	counters = debugfs_create_dir("counters", trans);
 
 	for (idx = 0; idx < SCMI_DEBUG_COUNTERS_LAST; idx++)
-		debugfs_create_atomic_t(dbg_counter_strs[idx], 0400, counters,
+		debugfs_create_atomic_t(dbg_counter_strs[idx], 0600, counters,
 					&dbg->counters[idx]);
+
+	debugfs_create_file("reset", 0200, counters, dbg, &fops_reset_counts);
 }
 
 static void scmi_debugfs_common_cleanup(void *d)
-- 
2.46.0

Re: [PATCH v5 0/5] Add per-transport SCMI communication debug statistics

[Cristian Marussi]

On Mon, Aug 05, 2024 at 02:10:07PM +0100, Sudeep Holla wrote:
> This series adds support for SCMI communication debug metrict tracking.
> I am just sending on behalf of Luke with minor reworks in his absense.
> 
> Cristian,
> 
> I have retained your review tags, please shout if you disagree.
> 
> Regards,
> Sudeep
> 

Hi,

LGTM.

If this is what was on Fri on your next, I gave it a go on my setup
too.

Tested-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v5 0/5] Add per-transport SCMI communication debug statistics

[Sudeep Holla]

On Mon, Aug 05, 2024 at 04:12:33PM +0100, Cristian Marussi wrote:
> On Mon, Aug 05, 2024 at 02:10:07PM +0100, Sudeep Holla wrote:
> > This series adds support for SCMI communication debug metrict tracking.
> > I am just sending on behalf of Luke with minor reworks in his absense.
> > 
> > Cristian,
> > 
> > I have retained your review tags, please shout if you disagree.
> > 
> > Regards,
> > Sudeep
> > 
> 
> Hi,
> 
> LGTM.
> 
> If this is what was on Fri on your next, I gave it a go on my setup
> too.
> 
> Tested-by: Cristian Marussi <cristian.marussi@arm.com>
> 

Thanks, I think I also modified some Kconfig text and scmi_inc_count()
but nothing that affects testing 😉.

-- 
Regards,
Sudeep

Re: [PATCH v5 0/5] Add per-transport SCMI communication debug statistics

[Sudeep Holla]

On Mon, 05 Aug 2024 14:10:07 +0100, Sudeep Holla wrote:
> This series adds support for SCMI communication debug metrict tracking.
> I am just sending on behalf of Luke with minor reworks in his absense.
>
> Cristian,
>
> I have retained your review tags, please shout if you disagree.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/5] firmware: arm_scmi: Remove superfluous handle_to_scmi_info
      https://git.kernel.org/sudeep.holla/c/20c9234d623c
[2/5] firmware: arm_scmi: Add support for debug metrics at the interface
      https://git.kernel.org/sudeep.holla/c/1b18d4295f9d
[3/5] firmware: arm_scmi: Track basic SCMI communication debug metrics
      https://git.kernel.org/sudeep.holla/c/0b3d48c4726e
[4/5] firmware: arm_scmi: Create debugfs files for SCMI communication debug metrics
      https://git.kernel.org/sudeep.holla/c/f6a905eaf6bf
[5/5] firmware: arm_scmi: Add support to reset the debug metrics
      https://git.kernel.org/sudeep.holla/c/bd02b0737f38
--
Regards,
Sudeep