[PATCH v2 0/4] Add Per-transport SCMI debug statistics

[PATCH v2 0/4] Add Per-transport SCMI debug statistics

[Luke Parkin]

Hi,

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]
And in [Patch 4] a selection of new debugfs entries to present these statistics

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

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 (4):
  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

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

-- 
2.34.1

[PATCH v2 1/4] 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>
---
 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 v2 2/4] firmware: arm_scmi: Add support for tracking statistics

[Luke Parkin]

Add a new config option for statistic tracking in SCMI subsystem
Add a struct for tracking statistics
Add scmi_log_stats op/no-op function for incrementing statistics

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
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 | 18 ++++++++++++++++++
 3 files changed, 38 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..1f50c4a209d7 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 *atm)
+{
+	atomic_inc(atm);
+}
+#else
+static inline void scmi_log_stats(atomic_t *atm) {}
+#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..df3eb17cf439 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -125,6 +125,22 @@ struct scmi_debug_info {
 	bool is_atomic;
 };
 
+/**
+ * struct scmi_debug_stats - Debug statistics
+ * @sent_ok: Count of successful sends
+ * @sent_fail: Count of failed sends
+ * @response_ok: Count of successful responses
+ * @dlyd_response_ok: Count of successful delayed responses
+ * @xfers_response_timeout: Count of xfer response timeouts
+ */
+struct scmi_debug_stats {
+	atomic_t sent_ok;
+	atomic_t sent_fail;
+	atomic_t response_ok;
+	atomic_t dlyd_response_ok;
+	atomic_t xfers_response_timeout;
+};
+
 /**
  * struct scmi_info - Structure representing a SCMI instance
  *
@@ -161,6 +177,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)
+ * @stats: Contains several atomic_t's for tracking various statistics
  * @raw: An opaque reference handle used by SCMI Raw mode.
  */
 struct scmi_info {
@@ -187,6 +204,7 @@ struct scmi_info {
 	/* Serialize device creation process for this instance */
 	struct mutex devreq_mtx;
 	struct scmi_debug_info *dbg;
+	struct scmi_debug_stats stats;
 	void *raw;
 };
 
-- 
2.34.1

[PATCH v2 3/4] firmware: arm_scmi: Track basic SCMI statistics

[Luke Parkin]

Add tracking of 5 initial statistics

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

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index df3eb17cf439..937546397cf2 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1146,8 +1146,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->stats.dlyd_response_ok);
 	} else {
 		complete(&xfer->done);
+		scmi_log_stats(&info->stats.response_ok);
 	}
 
 	if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {
@@ -1231,6 +1233,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) {
 		/*
@@ -1251,13 +1254,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->stats.xfers_response_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
@@ -1291,6 +1293,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->stats.xfers_response_timeout);
 		}
 	}
 
@@ -1374,13 +1377,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->stats.sent_fail);
 		return -EINVAL;
 	}
 
 	cinfo = idr_find(&info->tx_idr, pi->proto->id);
-	if (unlikely(!cinfo))
+	if (unlikely(!cinfo)) {
+		scmi_log_stats(&info->stats.sent_fail);
 		return -EINVAL;
-
+	}
 	/* True ONLY if also supported by transport. */
 	if (is_polling_enabled(cinfo, info->desc))
 		xfer->hdr.poll_completion = true;
@@ -1412,6 +1417,7 @@ 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->stats.sent_fail);
 		return ret;
 	}
 
@@ -1420,8 +1426,12 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 			    xfer->hdr.status, xfer->tx.buf, xfer->tx.len);
 
 	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->stats.sent_fail);
+	} else {
+		scmi_log_stats(&info->stats.sent_ok);
+	}
 
 	if (info->desc->ops->mark_txdone)
 		info->desc->ops->mark_txdone(cinfo, ret, xfer);
-- 
2.34.1

[PATCH v2 4/4] 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>
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 | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 937546397cf2..10cd9a319ffb 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2858,6 +2858,24 @@ 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("response_ok", 0400, stats,
+				&info->stats.response_ok);
+	debugfs_create_atomic_t("dlyd_response_ok", 0400, stats,
+				&info->stats.dlyd_response_ok);
+	debugfs_create_atomic_t("sent_ok", 0400, stats,
+				&info->stats.sent_ok);
+	debugfs_create_atomic_t("sent_fail", 0400, stats,
+				&info->stats.sent_fail);
+	debugfs_create_atomic_t("xfers_response_timeout", 0400, stats,
+				&info->stats.xfers_response_timeout);
+}
+
 static void scmi_debugfs_common_cleanup(void *d)
 {
 	struct scmi_debug_info *dbg = d;
@@ -2924,6 +2942,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 v2 1/4] firmware: arm_scmi: Remove superfluous handle_to_scmi_info

[Cristian Marussi]

On Wed, Jul 03, 2024 at 03:37:35PM +0100, Luke Parkin wrote:
> Remove duplicate handle_to_scmi_info

Good catch.

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

Thanks,
Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add support for tracking statistics

[Cristian Marussi]

On Wed, Jul 03, 2024 at 03:37:36PM +0100, Luke Parkin wrote:
> Add a new config option for statistic tracking in SCMI subsystem
> Add a struct for tracking statistics
> Add scmi_log_stats op/no-op function for incrementing statistics
> 

Hi Luke,

good that you keep a summary of viersion changes in the series you
are posting to aid in review....but...

> Signed-off-by: Luke Parkin <luke.parkin@arm.com>

...you need a 

---

...separator here before the summary

> 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

...if not this sumamry info will be kept in the final merged commit
message, which is not what we want...this is only for the sake of
reviews...

> ---
>  drivers/firmware/arm_scmi/Kconfig  | 11 +++++++++++
>  drivers/firmware/arm_scmi/common.h |  9 +++++++++
>  drivers/firmware/arm_scmi/driver.c | 18 ++++++++++++++++++
>  3 files changed, 38 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..1f50c4a209d7 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 *atm)
> +{
> +	atomic_inc(atm);
> +}
> +#else
> +static inline void scmi_log_stats(atomic_t *atm) {}
> +#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..df3eb17cf439 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -125,6 +125,22 @@ struct scmi_debug_info {
>  	bool is_atomic;
>  };
>  
> +/**
> + * struct scmi_debug_stats - Debug statistics
> + * @sent_ok: Count of successful sends
> + * @sent_fail: Count of failed sends
> + * @response_ok: Count of successful responses
> + * @dlyd_response_ok: Count of successful delayed responses
> + * @xfers_response_timeout: Count of xfer response timeouts
> + */
> +struct scmi_debug_stats {
> +	atomic_t sent_ok;
> +	atomic_t sent_fail;
> +	atomic_t response_ok;
> +	atomic_t dlyd_response_ok;
> +	atomic_t xfers_response_timeout;
> +};
> +
>  /**
>   * struct scmi_info - Structure representing a SCMI instance
>   *
> @@ -161,6 +177,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)
> + * @stats: Contains several atomic_t's for tracking various statistics
>   * @raw: An opaque reference handle used by SCMI Raw mode.
>   */
>  struct scmi_info {
> @@ -187,6 +204,7 @@ struct scmi_info {
>  	/* Serialize device creation process for this instance */
>  	struct mutex devreq_mtx;
>  	struct scmi_debug_info *dbg;
> +	struct scmi_debug_stats stats;
>  	void *raw;
>  };

Once the above nitpicks are fixed, I think this is good enough for our
purposes for now; optionally we could quickly move to a macro-based machinery
to avoid carrying the stats field in full even when STATS=n if someone
prefers that way... 

.... but...I will ask you anyway to add a few more stats in the next patch
by splitting a few of the existing ones...

Thanks,
Cristian

Re: [PATCH v2 3/4] firmware: arm_scmi: Track basic SCMI statistics

[Cristian Marussi]

On Wed, Jul 03, 2024 at 03:37:37PM +0100, Luke Parkin wrote:
> Add tracking of 5 initial statistics
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>

Missing

	---

> 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 | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index df3eb17cf439..937546397cf2 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1146,8 +1146,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->stats.dlyd_response_ok);
>  	} else {
>  		complete(&xfer->done);
> +		scmi_log_stats(&info->stats.response_ok);
>  	}
>  
>  	if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT)) {

Up above here in scmi_handle_respnonse() I would take care to track a
few interesting stats about when reponses that went bad...if you look at
scmi_xfer_command_acquire() you will notice that a whole class of errors
there, and  spread all around the core, are handled by scmi_bad_message_trace():
please handle all of these errors based on the type of the enum scmi_bad_msg...

....which means, IOW, add a counter for each the enum scmi_bad_msg and handle
them in scmi_bad_message_trace(), so that they will get counted everywhere....
and... :P ... while doing this please consider as usual the fact that we will
like the log_stats call to vanish completely at compile time if STATS=n

...then please look also into handle_notification() and try to add a couple
of counters there too...

... we can discuss all of these new counters in the next review...

> @@ -1231,6 +1233,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) {
>  		/*
> @@ -1251,13 +1254,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->stats.xfers_response_timeout);

I would rename this as 'xfer_response_polled_timeout' (so adding one more
stats to the structs and debugfs) in order to discern this timeout case
for the one below.

>  			}
>  		}
>  
>  		if (!ret) {
>  			unsigned long flags;
> -			struct scmi_info *info =
> -				handle_to_scmi_info(cinfo->handle);
>  

..in this same code-block, down below right before the trace_scmi_msg, I
would add a:

		scmi_log_stats(&info->stats.response_polled_ok);

...if not, you will end-up counting only the successfull reply on the IRQ
path, i.e. only the one that are served by handle_response.

>  			/*
>  			 * Do not fetch_response if an out-of-order delayed
> @@ -1291,6 +1293,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->stats.xfers_response_timeout);

This is a different kind of timeout...on non-polled request, lets keep
it named as it is now...

>  		}
>  	}
>  
> @@ -1374,13 +1377,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->stats.sent_fail);

Same here ... you have different reason for failures in the following,
so you could try to count them separately...

	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->stats.sent_fail);

	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;
> @@ -1412,6 +1417,7 @@ 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->stats.sent_fail);

this is the basic, original send failure...it is a generic failure at the transport layer,
so lets keep it named like it is now...

>  		return ret;
>  	}
>  

...here you can just place your (instead of the one down below)

	scmi_log_stats(&info->stats.sent_ok);

...becasue now, right here, you are already sure that the message has been sent ...

> @@ -1420,8 +1426,12 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>  			    xfer->hdr.status, xfer->tx.buf, xfer->tx.len);
>  
>  	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->stats.sent_fail);

...this is NOT a sent_fail...you DID send successfully the message AND received
successfully a response (ret==0)  BUT there was an error of some kind at the protocol
layer...i.e. the server sent you an error code in the reply...so I would count here
instead something named like

	stats.protocol_errs

> +	} else {
> +		scmi_log_stats(&info->stats.sent_ok);

and this is really late to be counted as sent, so it would be clearer to
count it above (where I mentiones), so please drop the else.

Thanks,
Cristian

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

[Cristian Marussi]

On Wed, Jul 03, 2024 at 03:37:38PM +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>

Missing ---

> 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 | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 937546397cf2..10cd9a319ffb 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2858,6 +2858,24 @@ 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("response_ok", 0400, stats,
> +				&info->stats.response_ok);
> +	debugfs_create_atomic_t("dlyd_response_ok", 0400, stats,
> +				&info->stats.dlyd_response_ok);
> +	debugfs_create_atomic_t("sent_ok", 0400, stats,
> +				&info->stats.sent_ok);
> +	debugfs_create_atomic_t("sent_fail", 0400, stats,
> +				&info->stats.sent_fail);
> +	debugfs_create_atomic_t("xfers_response_timeout", 0400, stats,
> +				&info->stats.xfers_response_timeout);
> +}
> +
>  static void scmi_debugfs_common_cleanup(void *d)
>  {
>  	struct scmi_debug_info *dbg = d;
> @@ -2924,6 +2942,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);
> +

Nothing to say here if not that more entries will need to be added as said.

Moreover, You could take the chance in V3 to add in this patch the
support to handle resetting each single counter (hint...this is a low hanging
fruit :D) and also to support something like:

	transports/stats/reset

which will be a WOnly booolean entry that will reset ALL the counters in
one go.

Thanks,
Cristian