[PATCH v4 0/5] Add Per-transport SCMI debug statistics

[PATCH v4 0/5] Add Per-transport SCMI debug statistics

[Luke Parkin]

This series adds support for tracking information about the SCMI [Patch 2/3]
A config option to enable this [Patch 2]
Cleans up a unneeded call to handle_scmi_info [Patch 1]
In [Patch 4] a selection of new debugfs entries to present these counters
Then finally in [Patch 5] enabled writing on the debugfs entries to reset counts

Based on v6.9, Tested on Arm Juno [1]

Thanks,
Luke

[1]: https://www.arm.com/products/development-tools/development-boards/juno-arm-dev-platform

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 tracking metrics
  firmware: arm_scmi: Track basic SCMI metrics
  firmware: arm_scmi: Create debugfs files for counts
  firmware: arm_scmi: Reset counters

 drivers/firmware/arm_scmi/Kconfig  | 11 ++++
 drivers/firmware/arm_scmi/common.h | 27 ++++++++++
 drivers/firmware/arm_scmi/driver.c | 87 +++++++++++++++++++++++++++---
 3 files changed, 117 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Luke Parkin]

Remove duplicate handle_to_scmi_info

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Luke Parkin <luke.parkin@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.34.1

[PATCH v4 2/5] firmware: arm_scmi: Add support for tracking metrics

[Luke Parkin]

Add a new optional config option for tracking, configurable
	at build.
Add methods for counting key metrics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
v3->v4
Rename to counters rather than statistic tracking
Move enum tracker to common.h
Move stats array into debug
v2->v3
Switch to an enum & array method of storing statistics
v1->v2
Config option now depends on DEBUG_FS
Add scmi_log_stats rather than if(IS_ENABLED)
Move location of scmi_debug_stats in the scmi_info struct
---
 drivers/firmware/arm_scmi/Kconfig  | 11 +++++++++++
 drivers/firmware/arm_scmi/common.h | 13 +++++++++++++
 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..b3ccd565b986 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -55,6 +55,17 @@ 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 debug tracking"
+	select ARM_SCMI_NEED_DEBUGFS
+	depends on DEBUG_FS
+	help
+	  Enables debug tracking for the SCMI subsystem.
+
+	  Enable this option to create a new debugfs directory which contains
+	  several useful debug counters. This can be helpful for debugging and
+	  SCMI monitoring. If unsure, say N.
+
 config ARM_SCMI_HAVE_TRANSPORT
 	bool
 	help
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index b5ac25dbc1ca..8f80bf0ddddd 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -301,6 +301,19 @@ 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
+};
+
+#ifdef CONFIG_ARM_SCMI_DEBUG_COUNTERS
+static inline void scmi_inc_count(atomic_t *arr, int stat)
+{
+	atomic_inc(&arr[stat]);
+}
+#else
+static inline void scmi_inc_count(atomic_t *arr, int stat) {}
+#endif
+
 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.34.1

[PATCH v4 3/5] firmware: arm_scmi: Track basic SCMI metrics

[Luke Parkin]

Add counting of initial metrics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
V3->V4
Use new names for functions
Use counter instead of stats for clarity
V2->V3
Add more statistics
Use new log_stats method.
V1->V2
Drop unneccesary atomic_set's
Use new 'scmi_log_stats' to simplify incrementing of atomics
Move scmi_log_stats to locations which mean no extra conditionals
	are needed
---
 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 8f80bf0ddddd..acec283f49de 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,
+	NOTIF_OK,
+	DLYD_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..5acd3d324def 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, NOTIF_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, DLYD_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.34.1

[PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Luke Parkin]

Create debugfs files for the metrics in the debug_counters array

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
v3->v4
Use new locations for debug array
Use counter instead of stats
v2->v3
Add extra statistics also added in v3
v1->v2
Only create stats pointer if stats are enabled
Move stats debugfs creation into a seperate helper function
---
 drivers/firmware/arm_scmi/driver.c | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 5acd3d324def..ec6434692d1a 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2847,6 +2847,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
+					struct dentry *trans)
+{
+	struct dentry *counters;
+
+	counters = debugfs_create_dir("counters", trans);
+
+	debugfs_create_atomic_t("sent_ok", 0400, counters,
+				&dbg->counters[SENT_OK]);
+	debugfs_create_atomic_t("sent_fail", 0400, counters,
+				&dbg->counters[SENT_FAIL]);
+	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
+				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
+	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
+				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
+	debugfs_create_atomic_t("response_ok", 0400, counters,
+				&dbg->counters[RESPONSE_OK]);
+	debugfs_create_atomic_t("notif_ok", 0400, counters,
+				&dbg->counters[NOTIF_OK]);
+	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
+				&dbg->counters[DLYD_RESPONSE_OK]);
+	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
+				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
+	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
+				&dbg->counters[RESPONSE_POLLED_OK]);
+	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
+				&dbg->counters[ERR_MSG_UNEXPECTED]);
+	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
+				&dbg->counters[ERR_MSG_INVALID]);
+	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
+				&dbg->counters[ERR_MSG_NOMEM]);
+	debugfs_create_atomic_t("err_protocol", 0400, counters,
+				&dbg->counters[ERR_PROTOCOL]);
+}
+
 static void scmi_debugfs_common_cleanup(void *d)
 {
 	struct scmi_debug_info *dbg = d;
@@ -2913,6 +2948,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.34.1

[PATCH v4 5/5] firmware: arm_scmi: Reset counters

[Luke Parkin]

Allow writing to atomics to reset
Create reset_all counters debugfs file to reset all counters

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
v3->v4
Use basic writing to allow any number to be set rather than forcing
	a set to 0
---
 drivers/firmware/arm_scmi/driver.c | 45 +++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ec6434692d1a..9e8720c27d51 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2847,6 +2847,24 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+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)
 {
@@ -2854,32 +2872,33 @@ static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
 
 	counters = debugfs_create_dir("counters", trans);
 
-	debugfs_create_atomic_t("sent_ok", 0400, counters,
+	debugfs_create_atomic_t("sent_ok", 0600, counters,
 				&dbg->counters[SENT_OK]);
-	debugfs_create_atomic_t("sent_fail", 0400, counters,
+	debugfs_create_atomic_t("sent_fail", 0600, counters,
 				&dbg->counters[SENT_FAIL]);
-	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
+	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0600, counters,
 				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
-	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
+	debugfs_create_atomic_t("sent_fail_channel_not_found", 0600, counters,
 				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
-	debugfs_create_atomic_t("response_ok", 0400, counters,
+	debugfs_create_atomic_t("response_ok", 0600, counters,
 				&dbg->counters[RESPONSE_OK]);
-	debugfs_create_atomic_t("notif_ok", 0400, counters,
+	debugfs_create_atomic_t("notif_ok", 0600, counters,
 				&dbg->counters[NOTIF_OK]);
-	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
+	debugfs_create_atomic_t("dlyd_resp_ok", 0600, counters,
 				&dbg->counters[DLYD_RESPONSE_OK]);
-	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
+	debugfs_create_atomic_t("xfers_resp_timeout", 0600, counters,
 				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
-	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
+	debugfs_create_atomic_t("response_polled_ok", 0600, counters,
 				&dbg->counters[RESPONSE_POLLED_OK]);
-	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
+	debugfs_create_atomic_t("err_msg_unexpected", 0600, counters,
 				&dbg->counters[ERR_MSG_UNEXPECTED]);
-	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
+	debugfs_create_atomic_t("err_msg_invalid", 0600, counters,
 				&dbg->counters[ERR_MSG_INVALID]);
-	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
+	debugfs_create_atomic_t("err_msg_nomem", 0600, counters,
 				&dbg->counters[ERR_MSG_NOMEM]);
-	debugfs_create_atomic_t("err_protocol", 0400, counters,
+	debugfs_create_atomic_t("err_protocol", 0600, counters,
 				&dbg->counters[ERR_PROTOCOL]);
+	debugfs_create_file("reset", 0200, counters, dbg, &fops_reset_counts);
 }
 
 static void scmi_debugfs_common_cleanup(void *d)
-- 
2.34.1

Re: [PATCH v4 2/5] firmware: arm_scmi: Add support for tracking metrics

[Cristian Marussi]

On Tue, Jul 30, 2024 at 10:33:39AM +0100, Luke Parkin wrote:
> Add a new optional config option for tracking, configurable
> 	at build.
> Add methods for counting key metrics

This commit message is a bit messed up, BUT it is fine, NO need to
respin a v5 just for this, we'll fix this on merge.

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

Thanks,
Cristian

Re: [PATCH v4 3/5] firmware: arm_scmi: Track basic SCMI metrics

[Cristian Marussi]

On Tue, Jul 30, 2024 at 10:33:40AM +0100, Luke Parkin wrote:
> Add counting of initial metrics
> 

LGTM.

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

Thanks,
Cristian

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Cristian Marussi]

On Tue, Jul 30, 2024 at 10:33:41AM +0100, Luke Parkin wrote:
> Create debugfs files for the metrics in the debug_counters array
>
 
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v4 5/5] firmware: arm_scmi: Reset counters

[Cristian Marussi]

On Tue, Jul 30, 2024 at 10:33:42AM +0100, Luke Parkin wrote:
> Allow writing to atomics to reset
> Create reset_all counters debugfs file to reset all counters
> 

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

Thanks,
Cristian

Re: [PATCH v4 0/5] Add Per-transport SCMI debug statistics

[Cristian Marussi]

On Tue, Jul 30, 2024 at 10:33:37AM +0100, Luke Parkin wrote:
> This series adds support for tracking information about the SCMI [Patch 2/3]
> A config option to enable this [Patch 2]
> Cleans up a unneeded call to handle_scmi_info [Patch 1]
> In [Patch 4] a selection of new debugfs entries to present these counters
> Then finally in [Patch 5] enabled writing on the debugfs entries to reset counts
> 
> Based on v6.9, Tested on Arm Juno [1]

Hi,

seems good to me. (...you have NOT rebased on v6.11-rc1 ... but it does
apply cleanly anyway apparently...or you dont have updated the above :D)

... anyway...let's see if Sudeep has any remarks...if not this will go
straight into -next soon so that bots can keep on bothering you on my behalf :P

Thanks,
Cristian

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Sudeep Holla]

On Tue, Jul 30, 2024 at 10:33:41AM +0100, Luke Parkin wrote:
> Create debugfs files for the metrics in the debug_counters array
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
> v3->v4
> Use new locations for debug array
> Use counter instead of stats
> v2->v3
> Add extra statistics also added in v3
> v1->v2
> Only create stats pointer if stats are enabled
> Move stats debugfs creation into a seperate helper function
> ---
>  drivers/firmware/arm_scmi/driver.c | 38 ++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 5acd3d324def..ec6434692d1a 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2847,6 +2847,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
> +					struct dentry *trans)
> +{
> +	struct dentry *counters;
> +
> +	counters = debugfs_create_dir("counters", trans);
> +
> +	debugfs_create_atomic_t("sent_ok", 0400, counters,
> +				&dbg->counters[SENT_OK]);
> +	debugfs_create_atomic_t("sent_fail", 0400, counters,
> +				&dbg->counters[SENT_FAIL]);
> +	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
> +				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
> +	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
> +				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
> +	debugfs_create_atomic_t("response_ok", 0400, counters,
> +				&dbg->counters[RESPONSE_OK]);
> +	debugfs_create_atomic_t("notif_ok", 0400, counters,
> +				&dbg->counters[NOTIF_OK]);
> +	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
> +				&dbg->counters[DLYD_RESPONSE_OK]);
> +	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
> +				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
> +	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
> +				&dbg->counters[RESPONSE_POLLED_OK]);
> +	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
> +				&dbg->counters[ERR_MSG_UNEXPECTED]);
> +	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
> +				&dbg->counters[ERR_MSG_INVALID]);
> +	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
> +				&dbg->counters[ERR_MSG_NOMEM]);
> +	debugfs_create_atomic_t("err_protocol", 0400, counters,
> +				&dbg->counters[ERR_PROTOCOL]);

Just curious and wonder if we can keep all these read-only and have another
debugfs file which is write only to just reset the counters ?

Cristian,

Thoughts ? Or am I missing somthing ?

-- 
Regards,
Sudeep

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Cristian Marussi]

On Wed, Jul 31, 2024 at 01:11:22PM +0100, Sudeep Holla wrote:
> On Tue, Jul 30, 2024 at 10:33:41AM +0100, Luke Parkin wrote:
> > Create debugfs files for the metrics in the debug_counters array
> > 
> > Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> > ---
> > v3->v4
> > Use new locations for debug array
> > Use counter instead of stats
> > v2->v3
> > Add extra statistics also added in v3
> > v1->v2
> > Only create stats pointer if stats are enabled
> > Move stats debugfs creation into a seperate helper function
> > ---
> >  drivers/firmware/arm_scmi/driver.c | 38 ++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 5acd3d324def..ec6434692d1a 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2847,6 +2847,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
> >  	return NOTIFY_OK;
> >  }
> >  
> > +static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
> > +					struct dentry *trans)
> > +{
> > +	struct dentry *counters;
> > +
> > +	counters = debugfs_create_dir("counters", trans);
> > +
> > +	debugfs_create_atomic_t("sent_ok", 0400, counters,
> > +				&dbg->counters[SENT_OK]);
> > +	debugfs_create_atomic_t("sent_fail", 0400, counters,
> > +				&dbg->counters[SENT_FAIL]);
> > +	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
> > +				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
> > +	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
> > +				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
> > +	debugfs_create_atomic_t("response_ok", 0400, counters,
> > +				&dbg->counters[RESPONSE_OK]);
> > +	debugfs_create_atomic_t("notif_ok", 0400, counters,
> > +				&dbg->counters[NOTIF_OK]);
> > +	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
> > +				&dbg->counters[DLYD_RESPONSE_OK]);
> > +	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
> > +				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
> > +	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
> > +				&dbg->counters[RESPONSE_POLLED_OK]);
> > +	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
> > +				&dbg->counters[ERR_MSG_UNEXPECTED]);
> > +	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
> > +				&dbg->counters[ERR_MSG_INVALID]);
> > +	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
> > +				&dbg->counters[ERR_MSG_NOMEM]);
> > +	debugfs_create_atomic_t("err_protocol", 0400, counters,
> > +				&dbg->counters[ERR_PROTOCOL]);
> 
> Just curious and wonder if we can keep all these read-only and have another
> debugfs file which is write only to just reset the counters ?
> 
> Cristian,
> 
> Thoughts ? Or am I missing somthing ?

Do you mean creating a bunch of additional reset_sent_ok
reset_<your_counters> that are just WO and used to reset each single
counter ?
(...I suppose because a global WO reset-all is already there...)

...I think it should be doable easily...but with a lot of duplication so
my next questions would be why ? what would be the gain ?
is it worth the effort being debug feats ?

Thanks,
Cristian

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Sudeep Holla]

On Wed, Jul 31, 2024 at 01:32:00PM +0100, Cristian Marussi wrote:
> On Wed, Jul 31, 2024 at 01:11:22PM +0100, Sudeep Holla wrote:
> > On Tue, Jul 30, 2024 at 10:33:41AM +0100, Luke Parkin wrote:
> > > Create debugfs files for the metrics in the debug_counters array
> > > 
> > > Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> > > ---
> > > v3->v4
> > > Use new locations for debug array
> > > Use counter instead of stats
> > > v2->v3
> > > Add extra statistics also added in v3
> > > v1->v2
> > > Only create stats pointer if stats are enabled
> > > Move stats debugfs creation into a seperate helper function
> > > ---
> > >  drivers/firmware/arm_scmi/driver.c | 38 ++++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index 5acd3d324def..ec6434692d1a 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > @@ -2847,6 +2847,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
> > >  	return NOTIFY_OK;
> > >  }
> > >  
> > > +static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
> > > +					struct dentry *trans)
> > > +{
> > > +	struct dentry *counters;
> > > +
> > > +	counters = debugfs_create_dir("counters", trans);
> > > +
> > > +	debugfs_create_atomic_t("sent_ok", 0400, counters,
> > > +				&dbg->counters[SENT_OK]);
> > > +	debugfs_create_atomic_t("sent_fail", 0400, counters,
> > > +				&dbg->counters[SENT_FAIL]);
> > > +	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
> > > +				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
> > > +	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
> > > +				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
> > > +	debugfs_create_atomic_t("response_ok", 0400, counters,
> > > +				&dbg->counters[RESPONSE_OK]);
> > > +	debugfs_create_atomic_t("notif_ok", 0400, counters,
> > > +				&dbg->counters[NOTIF_OK]);
> > > +	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
> > > +				&dbg->counters[DLYD_RESPONSE_OK]);
> > > +	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
> > > +				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
> > > +	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
> > > +				&dbg->counters[RESPONSE_POLLED_OK]);
> > > +	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
> > > +				&dbg->counters[ERR_MSG_UNEXPECTED]);
> > > +	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
> > > +				&dbg->counters[ERR_MSG_INVALID]);
> > > +	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
> > > +				&dbg->counters[ERR_MSG_NOMEM]);
> > > +	debugfs_create_atomic_t("err_protocol", 0400, counters,
> > > +				&dbg->counters[ERR_PROTOCOL]);
> > 
> > Just curious and wonder if we can keep all these read-only and have another
> > debugfs file which is write only to just reset the counters ?
> > 
> > Cristian,
> > 
> > Thoughts ? Or am I missing somthing ?
> 
> Do you mean creating a bunch of additional reset_sent_ok
> reset_<your_counters> that are just WO and used to reset each single
> counter ?

No, not at all. Sorry if my response meant that.

> (...I suppose because a global WO reset-all is already there...)
>

Indeed, I hadn't seen it carefully. Do we really need per counter reset ?
For me one global reset all with all files read-only should suffice.
Let me know if and why you think otherwise.

--
Regards,
Sudeep

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Cristian Marussi]

On Wed, Jul 31, 2024 at 01:40:49PM +0100, Sudeep Holla wrote:
> On Wed, Jul 31, 2024 at 01:32:00PM +0100, Cristian Marussi wrote:
> > On Wed, Jul 31, 2024 at 01:11:22PM +0100, Sudeep Holla wrote:
> > > On Tue, Jul 30, 2024 at 10:33:41AM +0100, Luke Parkin wrote:
> > > > Create debugfs files for the metrics in the debug_counters array
> > > > 
> > > > Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> > > > ---
> > > > v3->v4
> > > > Use new locations for debug array
> > > > Use counter instead of stats
> > > > v2->v3
> > > > Add extra statistics also added in v3
> > > > v1->v2
> > > > Only create stats pointer if stats are enabled
> > > > Move stats debugfs creation into a seperate helper function
> > > > ---
> > > >  drivers/firmware/arm_scmi/driver.c | 38 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 38 insertions(+)
> > > > 
> > > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > > index 5acd3d324def..ec6434692d1a 100644
> > > > --- a/drivers/firmware/arm_scmi/driver.c
> > > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > > @@ -2847,6 +2847,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
> > > >  	return NOTIFY_OK;
> > > >  }
> > > >  
> > > > +static void scmi_debugfs_counters_setup(struct scmi_debug_info *dbg,
> > > > +					struct dentry *trans)
> > > > +{
> > > > +	struct dentry *counters;
> > > > +
> > > > +	counters = debugfs_create_dir("counters", trans);
> > > > +
> > > > +	debugfs_create_atomic_t("sent_ok", 0400, counters,
> > > > +				&dbg->counters[SENT_OK]);
> > > > +	debugfs_create_atomic_t("sent_fail", 0400, counters,
> > > > +				&dbg->counters[SENT_FAIL]);
> > > > +	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, counters,
> > > > +				&dbg->counters[SENT_FAIL_POLLING_UNSUPPORTED]);
> > > > +	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, counters,
> > > > +				&dbg->counters[SENT_FAIL_CHANNEL_NOT_FOUND]);
> > > > +	debugfs_create_atomic_t("response_ok", 0400, counters,
> > > > +				&dbg->counters[RESPONSE_OK]);
> > > > +	debugfs_create_atomic_t("notif_ok", 0400, counters,
> > > > +				&dbg->counters[NOTIF_OK]);
> > > > +	debugfs_create_atomic_t("dlyd_resp_ok", 0400, counters,
> > > > +				&dbg->counters[DLYD_RESPONSE_OK]);
> > > > +	debugfs_create_atomic_t("xfers_resp_timeout", 0400, counters,
> > > > +				&dbg->counters[XFERS_RESPONSE_TIMEOUT]);
> > > > +	debugfs_create_atomic_t("response_polled_ok", 0400, counters,
> > > > +				&dbg->counters[RESPONSE_POLLED_OK]);
> > > > +	debugfs_create_atomic_t("err_msg_unexpected", 0400, counters,
> > > > +				&dbg->counters[ERR_MSG_UNEXPECTED]);
> > > > +	debugfs_create_atomic_t("err_msg_invalid", 0400, counters,
> > > > +				&dbg->counters[ERR_MSG_INVALID]);
> > > > +	debugfs_create_atomic_t("err_msg_nomem", 0400, counters,
> > > > +				&dbg->counters[ERR_MSG_NOMEM]);
> > > > +	debugfs_create_atomic_t("err_protocol", 0400, counters,
> > > > +				&dbg->counters[ERR_PROTOCOL]);
> > > 
> > > Just curious and wonder if we can keep all these read-only and have another
> > > debugfs file which is write only to just reset the counters ?
> > > 
> > > Cristian,
> > > 
> > > Thoughts ? Or am I missing somthing ?
> > 
> > Do you mean creating a bunch of additional reset_sent_ok
> > reset_<your_counters> that are just WO and used to reset each single
> > counter ?
> 
> No, not at all. Sorry if my response meant that.
> 
> > (...I suppose because a global WO reset-all is already there...)
> >
> 
> Indeed, I hadn't seen it carefully. Do we really need per counter reset ?
> For me one global reset all with all files read-only should suffice.
> Let me know if and why you think otherwise.

Well we don't need strictly anything since it is a debug/devel feat, it
just seemed handy to have a way to just reset one of the counters you are
actively working on, if you think about a testing or live devel
scenario...but it is not really needed and I have no strong opinion
about this...it was just a nice to have I asked Luke to add last minute...

...patch 5 indeed already adds also a global WO reset, beside switching all
the above counters to RW...it is just a matter to drop those RO->RW perms
changes in patch 5.

Thanks,
Cristian

Re: [PATCH v4 4/5] firmware: arm_scmi: Create debugfs files for counts

[Luke Parkin]

>> Indeed, I hadn't seen it carefully. Do we really need per counter reset ?
>> For me one global reset all with all files read-only should suffice.
>> Let me know if and why you think otherwise.
>
> Well we don't need strictly anything since it is a debug/devel feat, it
> just seemed handy to have a way to just reset one of the counters you are
> actively working on, if you think about a testing or live devel
> scenario...but it is not really needed and I have no strong opinion
> about this...it was just a nice to have I asked Luke to add last minute...
>
From my perspective, there isn't really any drawbacks to allowing writes to the
per counter reset? It's just a nice-to-have without much drawback, unless I'm
missing something.

Thanks,
Luke