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

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

[Luke Parkin]

This series adds support for tracking Arm SCMI statistics [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 statistics
Then finally in [Patch 5] enabled writing on the debugfs entries to reset stats

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

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 statistics
  firmware: arm_scmi: Track basic SCMI statistics
  firmware: arm_scmi: Create debugfs files for statistics
  firmware: arm_scmi: Reset statistics

 drivers/firmware/arm_scmi/Kconfig  |  11 +++
 drivers/firmware/arm_scmi/common.h |   9 ++
 drivers/firmware/arm_scmi/driver.c | 138 +++++++++++++++++++++++++++--
 3 files changed, 150 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Luke Parkin]

Remove duplicate handle_to_scmi_info

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
Reviewed-by: Cristian Marussi <cristian.marussi@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 v3 2/5] firmware: arm_scmi: Add support for tracking statistics

[Luke Parkin]

Add a new config option for statistic tracking in SCMI subsystem
Add an array and enum for tracking statistics
Add scmi_log_stats op/no-op function for incrementing statistics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
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 |  9 +++++++++
 drivers/firmware/arm_scmi/driver.c |  6 ++++++
 3 files changed, 26 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..45e8e7df927e 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_STATISTICS
+	bool "Enable SCMI Raw mode statistic tracking"
+	select ARM_SCMI_NEED_DEBUGFS
+	depends on DEBUG_FS
+	help
+	  Enables statistic tracking for the SCMI subsystem.
+
+	  Enable this option to create a new debugfs directory which contains
+	  several useful statistics on various SCMI features. This can be useful
+	  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..157df695aeb1 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -301,6 +301,15 @@ extern const struct scmi_desc scmi_optee_desc;
 
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
 
+#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
+static inline void scmi_log_stats(atomic_t *arr, int stat)
+{
+	atomic_inc(&arr[stat]);
+}
+#else
+static inline void scmi_log_stats(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..6edec6ec912d 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -125,6 +125,10 @@ struct scmi_debug_info {
 	bool is_atomic;
 };
 
+enum debug_stat_counters {
+	LAST
+};
+
 /**
  * struct scmi_info - Structure representing a SCMI instance
  *
@@ -161,6 +165,7 @@ struct scmi_debug_info {
  *		bus
  * @devreq_mtx: A mutex to serialize device creation for this SCMI instance
  * @dbg: A pointer to debugfs related data (if any)
+ * @dbg_stats: An array of atomic_c's used for tracking statistics (if enabled)
  * @raw: An opaque reference handle used by SCMI Raw mode.
  */
 struct scmi_info {
@@ -187,6 +192,7 @@ struct scmi_info {
 	/* Serialize device creation process for this instance */
 	struct mutex devreq_mtx;
 	struct scmi_debug_info *dbg;
+	atomic_t dbg_stats[LAST];
 	void *raw;
 };
 
-- 
2.34.1

[PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Luke Parkin]

Add tracking of initial statistics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
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/driver.c | 39 ++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6edec6ec912d..b22f104cda36 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -126,6 +126,20 @@ struct scmi_debug_info {
 };
 
 enum debug_stat_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,
 	LAST
 };
 
@@ -994,6 +1008,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_log_stats(info->dbg_stats, ERR_MSG_UNEXPECTED);
 
 		return xfer;
 	}
@@ -1021,6 +1036,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_log_stats(info->dbg_stats, ERR_MSG_INVALID);
+
 
 		/* On error the refcount incremented above has to be dropped */
 		__scmi_xfer_put(minfo, xfer);
@@ -1060,6 +1077,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo,
 			PTR_ERR(xfer));
 
 		scmi_bad_message_trace(cinfo, msg_hdr, MSG_NOMEM);
+		scmi_log_stats(info->dbg_stats, ERR_MSG_NOMEM);
 
 		scmi_clear_channel(info, cinfo);
 		return;
@@ -1075,6 +1093,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_log_stats(info->dbg_stats, NOTIF_OK);
 
 	scmi_notify(cinfo->handle, xfer->hdr.protocol_id,
 		    xfer->hdr.id, xfer->rx.buf, xfer->rx.len, ts);
@@ -1134,8 +1153,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_log_stats(info->dbg_stats, DLYD_RESPONSE_OK);
 	} else {
 		complete(&xfer->done);
+		scmi_log_stats(info->dbg_stats, RESPONSE_OK);
 	}
 
 	if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
@@ -1219,6 +1240,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) {
 		/*
@@ -1239,13 +1261,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_log_stats(info->dbg_stats, 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
@@ -1265,6 +1286,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_log_stats(info->dbg_stats, RESPONSE_POLLED_OK);
 
 			if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
 				scmi_raw_message_report(info->raw, xfer,
@@ -1279,6 +1301,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_log_stats(info->dbg_stats, XFERS_RESPONSE_TIMEOUT);
 		}
 	}
 
@@ -1362,13 +1385,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_log_stats(info->dbg_stats, SENT_FAIL_POLLING_UNSUPPORTED);
 		return -EINVAL;
 	}
 
 	cinfo = idr_find(&info->tx_idr, pi->proto->id);
-	if (unlikely(!cinfo))
+	if (unlikely(!cinfo)) {
+		scmi_log_stats(info->dbg_stats, 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;
@@ -1400,16 +1425,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_log_stats(info->dbg_stats, 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_log_stats(info->dbg_stats, 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_log_stats(info->dbg_stats, ERR_PROTOCOL);
+	}
 
 	if (info->desc->ops->mark_txdone)
 		info->desc->ops->mark_txdone(cinfo, ret, xfer);
-- 
2.34.1

[PATCH v3 4/5] firmware: arm_scmi: Create debugfs files for statistics

[Luke Parkin]

Create debugfs files for the statistics in the scmi_debug_stats struct

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
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 b22f104cda36..9378e2d8af4f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2865,6 +2865,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static void scmi_debugfs_stats_setup(struct scmi_info *info,
+				     struct dentry *trans)
+{
+	struct dentry *stats;
+
+	stats = debugfs_create_dir("stats", trans);
+
+	debugfs_create_atomic_t("sent_ok", 0400, stats,
+				&info->dbg_stats[SENT_OK]);
+	debugfs_create_atomic_t("sent_fail", 0400, stats,
+				&info->dbg_stats[SENT_FAIL]);
+	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, stats,
+				&info->dbg_stats[SENT_FAIL_POLLING_UNSUPPORTED]);
+	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, stats,
+				&info->dbg_stats[SENT_FAIL_CHANNEL_NOT_FOUND]);
+	debugfs_create_atomic_t("response_ok", 0400, stats,
+				&info->dbg_stats[RESPONSE_OK]);
+	debugfs_create_atomic_t("notif_ok", 0400, stats,
+				&info->dbg_stats[NOTIF_OK]);
+	debugfs_create_atomic_t("dlyd_resp_ok", 0400, stats,
+				&info->dbg_stats[DLYD_RESPONSE_OK]);
+	debugfs_create_atomic_t("xfers_resp_timeout", 0400, stats,
+				&info->dbg_stats[XFERS_RESPONSE_TIMEOUT]);
+	debugfs_create_atomic_t("response_polled_ok", 0400, stats,
+				&info->dbg_stats[RESPONSE_POLLED_OK]);
+	debugfs_create_atomic_t("err_msg_unexpected", 0400, stats,
+				&info->dbg_stats[ERR_MSG_UNEXPECTED]);
+	debugfs_create_atomic_t("err_msg_invalid", 0400, stats,
+				&info->dbg_stats[ERR_MSG_INVALID]);
+	debugfs_create_atomic_t("err_msg_nomem", 0400, stats,
+				&info->dbg_stats[ERR_MSG_NOMEM]);
+	debugfs_create_atomic_t("err_protocol", 0400, stats,
+				&info->dbg_stats[ERR_PROTOCOL]);
+}
+
 static void scmi_debugfs_common_cleanup(void *d)
 {
 	struct scmi_debug_info *dbg = d;
@@ -2931,6 +2966,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_STATISTICS))
+		scmi_debugfs_stats_setup(info, trans);
+
 	dbg->top_dentry = top_dentry;
 
 	if (devm_add_action_or_reset(info->dev,
-- 
2.34.1

[PATCH v3 5/5] firmware: arm_scmi: Reset statistics

[Luke Parkin]

Create write function to reset individual statistics on write
Create reset_all stats debugfs file to reset all statistics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 104 +++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 9378e2d8af4f..6a90311f764d 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2865,6 +2865,47 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static int read_atomic(void *atomic, u64 *val)
+{
+	atomic_t *atm = (atomic_t *)atomic;
+
+	*val = atomic_read(atm);
+	return 0;
+}
+
+static int reset_single(void *atomic, u64 val)
+{
+	atomic_t *atm = (atomic_t *)atomic;
+
+	atomic_set(atm, 0);
+	return 0;
+}
+
+static void reset_all_stats(struct scmi_info *info)
+{
+	for (int i = 0; i < LAST; i++)
+		atomic_set(&info->dbg_stats[i], 0);
+}
+
+static ssize_t reset_all_on_write(struct file *filp,
+				  const char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct scmi_info *info = filp->private_data;
+
+	reset_all_stats(info);
+	return count;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_reset_on_write, read_atomic, reset_single, "%llu\n");
+
+static const struct file_operations fops_reset_stats = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.llseek = no_llseek,
+	.write = reset_all_on_write,
+};
+
 static void scmi_debugfs_stats_setup(struct scmi_info *info,
 				     struct dentry *trans)
 {
@@ -2872,32 +2913,43 @@ static void scmi_debugfs_stats_setup(struct scmi_info *info,
 
 	stats = debugfs_create_dir("stats", trans);
 
-	debugfs_create_atomic_t("sent_ok", 0400, stats,
-				&info->dbg_stats[SENT_OK]);
-	debugfs_create_atomic_t("sent_fail", 0400, stats,
-				&info->dbg_stats[SENT_FAIL]);
-	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, stats,
-				&info->dbg_stats[SENT_FAIL_POLLING_UNSUPPORTED]);
-	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, stats,
-				&info->dbg_stats[SENT_FAIL_CHANNEL_NOT_FOUND]);
-	debugfs_create_atomic_t("response_ok", 0400, stats,
-				&info->dbg_stats[RESPONSE_OK]);
-	debugfs_create_atomic_t("notif_ok", 0400, stats,
-				&info->dbg_stats[NOTIF_OK]);
-	debugfs_create_atomic_t("dlyd_resp_ok", 0400, stats,
-				&info->dbg_stats[DLYD_RESPONSE_OK]);
-	debugfs_create_atomic_t("xfers_resp_timeout", 0400, stats,
-				&info->dbg_stats[XFERS_RESPONSE_TIMEOUT]);
-	debugfs_create_atomic_t("response_polled_ok", 0400, stats,
-				&info->dbg_stats[RESPONSE_POLLED_OK]);
-	debugfs_create_atomic_t("err_msg_unexpected", 0400, stats,
-				&info->dbg_stats[ERR_MSG_UNEXPECTED]);
-	debugfs_create_atomic_t("err_msg_invalid", 0400, stats,
-				&info->dbg_stats[ERR_MSG_INVALID]);
-	debugfs_create_atomic_t("err_msg_nomem", 0400, stats,
-				&info->dbg_stats[ERR_MSG_NOMEM]);
-	debugfs_create_atomic_t("err_protocol", 0400, stats,
-				&info->dbg_stats[ERR_PROTOCOL]);
+	debugfs_create_file("sent_ok", 0400, stats, &info->dbg_stats[SENT_OK],
+			    &fops_reset_on_write);
+	debugfs_create_file("sent_fail", 0400, stats,
+			    &info->dbg_stats[SENT_FAIL], &fops_reset_on_write);
+	debugfs_create_file("sent_fail_polling_unsupported", 0400, stats,
+			    &info->dbg_stats[SENT_FAIL_POLLING_UNSUPPORTED],
+			    &fops_reset_on_write);
+	debugfs_create_file("sent_fail_channel_not_found", 0400, stats,
+			    &info->dbg_stats[SENT_FAIL_CHANNEL_NOT_FOUND],
+			    &fops_reset_on_write);
+	debugfs_create_file("response_ok", 0400, stats,
+			    &info->dbg_stats[RESPONSE_OK],
+			    &fops_reset_on_write);
+	debugfs_create_file("notif_ok", 0400, stats, &info->dbg_stats[NOTIF_OK],
+			    &fops_reset_on_write);
+	debugfs_create_file("dlyd_resp_ok", 0400, stats,
+			    &info->dbg_stats[DLYD_RESPONSE_OK],
+			    &fops_reset_on_write);
+	debugfs_create_file("xfers_resp_timeout", 0400, stats,
+			    &info->dbg_stats[XFERS_RESPONSE_TIMEOUT],
+			    &fops_reset_on_write);
+	debugfs_create_file("response_polled_ok", 0400, stats,
+			    &info->dbg_stats[RESPONSE_POLLED_OK],
+			    &fops_reset_on_write);
+	debugfs_create_file("err_msg_unexpected", 0400, stats,
+			    &info->dbg_stats[ERR_MSG_UNEXPECTED],
+			    &fops_reset_on_write);
+	debugfs_create_file("err_msg_invalid", 0400, stats,
+			    &info->dbg_stats[ERR_MSG_INVALID],
+			    &fops_reset_on_write);
+	debugfs_create_file("err_msg_nomem", 0400, stats,
+			    &info->dbg_stats[ERR_MSG_NOMEM],
+			    &fops_reset_on_write);
+	debugfs_create_file("err_protocol", 0400, stats,
+			    &info->dbg_stats[ERR_PROTOCOL],
+			    &fops_reset_on_write);
+	debugfs_create_file("reset", 0200, stats, info, &fops_reset_stats);
 }
 
 static void scmi_debugfs_common_cleanup(void *d)
-- 
2.34.1

RE: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Peng Fan]

> Subject: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI
> statistics
> 
> Add tracking of initial statistics
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>

Just wonder that since ftrace is there, why need this?

Thanks,
Peng.

> ---
> 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/driver.c | 39
> ++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c
> b/drivers/firmware/arm_scmi/driver.c
> index 6edec6ec912d..b22f104cda36 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -126,6 +126,20 @@ struct scmi_debug_info {  };
> 
>  enum debug_stat_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,
>  	LAST
>  };
> 
> @@ -994,6 +1008,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_log_stats(info->dbg_stats,
> ERR_MSG_UNEXPECTED);
> 
>  		return xfer;
>  	}
> @@ -1021,6 +1036,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_log_stats(info->dbg_stats, ERR_MSG_INVALID);
> +
> 
>  		/* On error the refcount incremented above has to be
> dropped */
>  		__scmi_xfer_put(minfo, xfer);
> @@ -1060,6 +1077,7 @@ static void scmi_handle_notification(struct
> scmi_chan_info *cinfo,
>  			PTR_ERR(xfer));
> 
>  		scmi_bad_message_trace(cinfo, msg_hdr,
> MSG_NOMEM);
> +		scmi_log_stats(info->dbg_stats, ERR_MSG_NOMEM);
> 
>  		scmi_clear_channel(info, cinfo);
>  		return;
> @@ -1075,6 +1093,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_log_stats(info->dbg_stats, NOTIF_OK);
> 
>  	scmi_notify(cinfo->handle, xfer->hdr.protocol_id,
>  		    xfer->hdr.id, xfer->rx.buf, xfer->rx.len, ts); @@ -
> 1134,8 +1153,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_log_stats(info->dbg_stats, DLYD_RESPONSE_OK);
>  	} else {
>  		complete(&xfer->done);
> +		scmi_log_stats(info->dbg_stats, RESPONSE_OK);
>  	}
> 
>  	if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT))
> { @@ -1219,6 +1240,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) {
>  		/*
> @@ -1239,13 +1261,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_log_stats(info->dbg_stats,
> 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 @@ -1265,6 +1286,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_log_stats(info->dbg_stats,
> RESPONSE_POLLED_OK);
> 
>  			if
> (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
>  				scmi_raw_message_report(info->raw,
> xfer, @@ -1279,6 +1301,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_log_stats(info->dbg_stats,
> XFERS_RESPONSE_TIMEOUT);
>  		}
>  	}
> 
> @@ -1362,13 +1385,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_log_stats(info->dbg_stats,
> SENT_FAIL_POLLING_UNSUPPORTED);
>  		return -EINVAL;
>  	}
> 
>  	cinfo = idr_find(&info->tx_idr, pi->proto->id);
> -	if (unlikely(!cinfo))
> +	if (unlikely(!cinfo)) {
> +		scmi_log_stats(info->dbg_stats,
> 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;
> @@ -1400,16 +1425,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_log_stats(info->dbg_stats, 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_log_stats(info->dbg_stats, 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_log_stats(info->dbg_stats, ERR_PROTOCOL);
> +	}
> 
>  	if (info->desc->ops->mark_txdone)
>  		info->desc->ops->mark_txdone(cinfo, ret, xfer);
> --
> 2.34.1
> 

Re: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Luke Parkin]

 > Just wonder that since ftrace is there, why need this?

Hi, this series aims to provide a useful debug aid for running tests on the SCMI
subsystem. As you mentioned, ftrace does report some of these statistics,
however DEBUG_STATISTICS will allow a programatic interface to access the count
of failures, successes and an extended variety of other specific
errors/statistics for automated (or manual) testing.

Hope this clears things up,
Luke

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

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:47PM +0100, Luke Parkin wrote:
> Remove duplicate handle_to_scmi_info
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Your Signed-off-by should always come last after any Reviewd-by, CC or
any other tag...

Thanks,
Cristian

Re: [PATCH v3 2/5] firmware: arm_scmi: Add support for tracking statistics

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:48PM +0100, Luke Parkin wrote:
> Add a new config option for statistic tracking in SCMI subsystem
> Add an array and enum for tracking statistics
> Add scmi_log_stats op/no-op function for incrementing statistics
> 

You should terminate your sentences with a period, and I would stay more
generic, no need to detail the changes in the commit log.

Some times there is not so much to say more than what is expressed in
the title :D .... I would only stress that new support for SCMI statistics is
added and that such support is optional, configurable at build-time and
OFF by default...

> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
> 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 |  9 +++++++++
>  drivers/firmware/arm_scmi/driver.c |  6 ++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..45e8e7df927e 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_STATISTICS
> +	bool "Enable SCMI Raw mode statistic tracking"

" SCMI Raw mode" .... this is unrelated to Raw mode...probably just a leftover
from V2 ... to be dropped...

> +	select ARM_SCMI_NEED_DEBUGFS
> +	depends on DEBUG_FS
> +	help
> +	  Enables statistic tracking for the SCMI subsystem.
> +
> +	  Enable this option to create a new debugfs directory which contains
> +	  several useful statistics on various SCMI features. This can be useful
> +	  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..157df695aeb1 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -301,6 +301,15 @@ extern const struct scmi_desc scmi_optee_desc;
>  
>  void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
>  
> +#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
> +static inline void scmi_log_stats(atomic_t *arr, int stat)
> +{
> +	atomic_inc(&arr[stat]);
> +}
> +#else
> +static inline void scmi_log_stats(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..6edec6ec912d 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -125,6 +125,10 @@ struct scmi_debug_info {
>  	bool is_atomic;
>  };
>  
> +enum debug_stat_counters {
> +	LAST

You should definitely call this something more specific like SCMI_DEBUG_COUNTERS_LAST.

Also, I would move this enum definitions to common.h since in the near
future we could want to track more stats from other area of the stack...
(i.e. outside this driver.c file)..and also because scmi_log_stats is
already defined in common.h

> +};
> +
>  /**
>   * struct scmi_info - Structure representing a SCMI instance
>   *
> @@ -161,6 +165,7 @@ struct scmi_debug_info {
>   *		bus
>   * @devreq_mtx: A mutex to serialize device creation for this SCMI instance
>   * @dbg: A pointer to debugfs related data (if any)
> + * @dbg_stats: An array of atomic_c's used for tracking statistics (if enabled)
>   * @raw: An opaque reference handle used by SCMI Raw mode.
>   */
>  struct scmi_info {
> @@ -187,6 +192,7 @@ struct scmi_info {
>  	/* Serialize device creation process for this instance */
>  	struct mutex devreq_mtx;
>  	struct scmi_debug_info *dbg;
> +	atomic_t dbg_stats[LAST];

I would also move this dbg_stats inside scmi_debug_info and name it just
plain as 'stats' so that you will then access it as 

	info->dbg.stats

...unless I miss something about scoping and it is problematic...

...while at that, I would also place the new stats[] field as last in the
scni_debug_info structure...this is because, even though you are a building
a real array of atomic, up until now in the seris you are indeed introducing
(apparently to static analyzers) a zero-length array (since LAST is zero)....
...and I think gcc/llvm or static analyzers would complain if thet see an
arr[0] placed NOT as the last elemnt of a struct

Thanks,
Cristian

Re: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:49PM +0100, Luke Parkin wrote:
> Add tracking of initial statistics
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
> 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/driver.c | 39 ++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 6edec6ec912d..b22f104cda36 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -126,6 +126,20 @@ struct scmi_debug_info {
>  };
>  
>  enum debug_stat_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,
>  	LAST
>  };
>  
> @@ -994,6 +1008,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_log_stats(info->dbg_stats, ERR_MSG_UNEXPECTED);

I'd be tempted to say why dont you wrap these scmi_log_stats() inside the
scmi_bad_message_trace() ... BUT in order to avoid an additional
conditional inside the scmi_bad_message_trace() you will need to somehow
remap the MSG_UNEXPECTED to ERR_MSG_UNEXPECTED inside scmi_bad_message_trace (lets say
with a local onstack array indexed by -err)...AND that would mean committing to keep
such mapping in-sync with the the above enum, so as to avoid that adding
a new definition into scmi_bad_msg BUT not to debug_stat_counters will
end up in a buffer overflow....so at the end probably better/safer to keep it
this way...

Thanks,
Cristian

Re: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Cristian Marussi]

On Mon, Jul 22, 2024 at 01:21:31PM +0100, Luke Parkin wrote:
>  > Just wonder that since ftrace is there, why need this?
> 
> Hi, this series aims to provide a useful debug aid for running tests on the SCMI
> subsystem. As you mentioned, ftrace does report some of these statistics,
> however DEBUG_STATISTICS will allow a programatic interface to access the count
> of failures, successes and an extended variety of other specific
> errors/statistics for automated (or manual) testing.
> 
> Hope this clears things up,
> Luke

Yes, the idea was to have some continuosuly running counters that can be looked
up (when compiled-in, NOT in production) to check for anomalies...ftrace
are certainly more detailed in these regards BUT you have to enable them
at runtime and then dig into the log, and moreover do not cover all of
the cases like these stats...

Thanks,
Cristian

Re: [PATCH v3 4/5] firmware: arm_scmi: Create debugfs files for statistics

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:50PM +0100, Luke Parkin wrote:
> Create debugfs files for the statistics in the scmi_debug_stats struct
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>

Here you missed the seprator

---

before the changelog...so this will go into the final commit.

Thanks,
Cristian

> 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 b22f104cda36..9378e2d8af4f 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2865,6 +2865,41 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static void scmi_debugfs_stats_setup(struct scmi_info *info,
> +				     struct dentry *trans)
> +{
> +	struct dentry *stats;
> +
> +	stats = debugfs_create_dir("stats", trans);
> +
> +	debugfs_create_atomic_t("sent_ok", 0400, stats,
> +				&info->dbg_stats[SENT_OK]);
> +	debugfs_create_atomic_t("sent_fail", 0400, stats,
> +				&info->dbg_stats[SENT_FAIL]);
> +	debugfs_create_atomic_t("sent_fail_polling_unsupported", 0400, stats,
> +				&info->dbg_stats[SENT_FAIL_POLLING_UNSUPPORTED]);
> +	debugfs_create_atomic_t("sent_fail_channel_not_found", 0400, stats,
> +				&info->dbg_stats[SENT_FAIL_CHANNEL_NOT_FOUND]);
> +	debugfs_create_atomic_t("response_ok", 0400, stats,
> +				&info->dbg_stats[RESPONSE_OK]);
> +	debugfs_create_atomic_t("notif_ok", 0400, stats,
> +				&info->dbg_stats[NOTIF_OK]);
> +	debugfs_create_atomic_t("dlyd_resp_ok", 0400, stats,
> +				&info->dbg_stats[DLYD_RESPONSE_OK]);
> +	debugfs_create_atomic_t("xfers_resp_timeout", 0400, stats,
> +				&info->dbg_stats[XFERS_RESPONSE_TIMEOUT]);
> +	debugfs_create_atomic_t("response_polled_ok", 0400, stats,
> +				&info->dbg_stats[RESPONSE_POLLED_OK]);
> +	debugfs_create_atomic_t("err_msg_unexpected", 0400, stats,
> +				&info->dbg_stats[ERR_MSG_UNEXPECTED]);
> +	debugfs_create_atomic_t("err_msg_invalid", 0400, stats,
> +				&info->dbg_stats[ERR_MSG_INVALID]);
> +	debugfs_create_atomic_t("err_msg_nomem", 0400, stats,
> +				&info->dbg_stats[ERR_MSG_NOMEM]);
> +	debugfs_create_atomic_t("err_protocol", 0400, stats,
> +				&info->dbg_stats[ERR_PROTOCOL]);
> +}
> +
>  static void scmi_debugfs_common_cleanup(void *d)
>  {
>  	struct scmi_debug_info *dbg = d;
> @@ -2931,6 +2966,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_STATISTICS))
> +		scmi_debugfs_stats_setup(info, trans);
> +
>  	dbg->top_dentry = top_dentry;
>  
>  	if (devm_add_action_or_reset(info->dev,
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 3/5] firmware: arm_scmi: Track basic SCMI statistics

[Luke Parkin]

>> @@ -994,6 +1008,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_log_stats(info->dbg_stats, ERR_MSG_UNEXPECTED);
> 
> I'd be tempted to say why dont you wrap these scmi_log_stats() inside the
> scmi_bad_message_trace() ... BUT in order to avoid an additional
> conditional inside the scmi_bad_message_trace() you will need to somehow
> remap the MSG_UNEXPECTED to ERR_MSG_UNEXPECTED inside scmi_bad_message_trace (lets say
> with a local onstack array indexed by -err)...AND that would mean committing to keep
> such mapping in-sync with the the above enum, so as to avoid that adding
> a new definition into scmi_bad_msg BUT not to debug_stat_counters will
> end up in a buffer overflow....so at the end probably better/safer to keep it
> this way...

Yeah, I had the same thoughts and came to the same conclusion. As far as I could
determine there weren't enough scmi_bad_message_trace()'s to warrant the extra
complexity (both code and processing) of centralising it.

Thanks,
Luke

Re: [PATCH v3 5/5] firmware: arm_scmi: Reset statistics

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:51PM +0100, Luke Parkin wrote:
> Create write function to reset individual statistics on write
> Create reset_all stats debugfs file to reset all statistics
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 104 +++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9378e2d8af4f..6a90311f764d 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2865,6 +2865,47 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static int read_atomic(void *atomic, u64 *val)
> +{
> +	atomic_t *atm = (atomic_t *)atomic;
> +
> +	*val = atomic_read(atm);
> +	return 0;
> +}
> +
> +static int reset_single(void *atomic, u64 val)
> +{
> +	atomic_t *atm = (atomic_t *)atomic;
> +
> +	atomic_set(atm, 0);
> +	return 0;
> +}
> +

So, you rightly built a fops_reset_on_write to handle any write to a
single stats field and zero teh counter...BUT...for the sake of
simplicity, we could relax a bit the requirement and just think about
handling generically the writes...because if you look at the definition
of debugfs_create_atomic_t:

https://elixir.bootlin.com/linux/v6.10/source/fs/debugfs/file.c#L867

you will see that if you declare the mode as RW 0600, the debugfs core
code will automagically provide you with RW debugfs atomic fops...so
that you wont need all of this and you can still keep using
debugfs_create_atomic_t....the only limitation is that the user will be
able to reset the counter to any value, NOT only to zero...BUT being a
debug/test feats seems to me acceptable.


> +static void reset_all_stats(struct scmi_info *info)
> +{
> +	for (int i = 0; i < LAST; i++)
> +		atomic_set(&info->dbg_stats[i], 0);
> +}

You can drop this function and just nest the for loop inside the
function below...

> +
> +static ssize_t reset_all_on_write(struct file *filp,
> +				  const char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct scmi_info *info = filp->private_data;
> +
> +	reset_all_stats(info);
> +	return count;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_reset_on_write, read_atomic, reset_single, "%llu\n");
> +

..and drop this as said above to keep using debugfs_create_atomic_t

> +static const struct file_operations fops_reset_stats = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.llseek = no_llseek,
> +	.write = reset_all_on_write,
> +};
> +

This is good instead for a quick general reset...

>  static void scmi_debugfs_stats_setup(struct scmi_info *info,
>  				     struct dentry *trans)
>  {
> @@ -2872,32 +2913,43 @@ static void scmi_debugfs_stats_setup(struct scmi_info *info,
>  
>  	stats = debugfs_create_dir("stats", trans);
>  
> -	debugfs_create_atomic_t("sent_ok", 0400, stats,

using 0600 here made the trick...

> -				&info->dbg_stats[SENT_OK]);

Thanks,
Cristian

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

[Cristian Marussi]

On Mon, Jul 15, 2024 at 02:37:46PM +0100, Luke Parkin wrote:
> This series adds support for tracking Arm SCMI statistics [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 statistics
> Then finally in [Patch 5] enabled writing on the debugfs entries to reset stats
> 
> Based on v6.9, Tested on Arm Juno [1]
> 
> Thanks,
> Luke

Beside the specific patch review comments, I have been just reminded by
ATG that using SCMI stats is a bit ambigous since there are statistics
in the protocol...so please in v4 take care to rename all the
code/commmemts and commit msg to something else ... that could be...
"counters" ? instead of stats...so you will have debug counters available
under transport/counters instead of /stats....etc etc

Thanks,
Cristian