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

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

[Luke Parkin]

Hi,

This series adds support for tracking Arm SCMI statistics, [Patch 2]
A config option to enable this, [Patch 1] 
And in [Patch 3] a selection of new debugfs entries to present these statistics

These statistics are per transport instance, and will be a helpful resource when
debugging the SCMI and running tests.

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

Luke Parkin (3):
  Add Kconfig option for scmi debug statistics
  Track basic SCMI statistics
  Create debugfs files for statistics

 drivers/firmware/arm_scmi/Kconfig  | 10 +++++
 drivers/firmware/arm_scmi/driver.c | 61 +++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/3] Add Kconfig option for scmi debug statistics

[Luke Parkin]

Add a new config option for statistic tracking in SCMI subsystem.

Signed-off-by: Luke Parkin <luke.parkin@arm.com>
---
 drivers/firmware/arm_scmi/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..fac50fd0be72 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -55,6 +55,16 @@ 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
+	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
-- 
2.34.1

Re: [PATCH 1/3] Add Kconfig option for scmi debug statistics

[Cristian Marussi]

On Mon, Jul 01, 2024 at 03:28:49PM +0100, Luke Parkin wrote:
> Add a new config option for statistic tracking in SCMI subsystem.
> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
>  drivers/firmware/arm_scmi/Kconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Hi,

> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..fac50fd0be72 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -55,6 +55,16 @@ 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"

I would add also a depends on DEBUG_FS like in RAW...and maybe in the
future move such all of such depends on ARM_SCMI_NEED_DEBUGFS
instead...but for now it is easier and less potentially disruptive to
the build to just add a depends here.

> +	select ARM_SCMI_NEED_DEBUGFS
> +	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.
> +

Moreover you should move this Kconfig patch after the patches in the
series that includes the logic underneath this...here really you are
defining some option that, if enabled, at this point it really still
does NOT enable anything.

Thanks,
Cristian

[PATCH 2/3] Track basic SCMI statistics

[Luke Parkin]

Add scmi_debug_stats struct with atomic_t types to prevent racing.

Add tracking of 5 initial statistics
- sent_ok & sent_fail
- response_ok & dlyd_response_ok
- xfers_response_timeout

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

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6b6957f4743f..f69dff699d48 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
  *
@@ -141,6 +157,7 @@ struct scmi_debug_info {
  * @protocols: IDR for protocols' instance descriptors initialized for
  *	       this SCMI instance: populated on protocol's first attempted
  *	       usage.
+ * @stats: Contains several atomic_t's for tracking various statistics
  * @protocols_mtx: A mutex to protect protocols instances initialization.
  * @protocols_imp: List of protocols implemented, currently maximum of
  *		   scmi_revision_info.num_protocols elements allocated by the
@@ -174,6 +191,7 @@ struct scmi_info {
 	struct idr tx_idr;
 	struct idr rx_idr;
 	struct idr protocols;
+	struct scmi_debug_stats stats;
 	/* Ensure mutual exclusive access to protocols instance array */
 	struct mutex protocols_mtx;
 	u8 *protocols_imp;
@@ -1143,7 +1161,12 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 						SCMI_RAW_REPLY_QUEUE,
 						cinfo->id);
 	}
-
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
+		if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP)
+			atomic_inc(&info->stats.dlyd_response_ok);
+		else
+			atomic_inc(&info->stats.response_ok);
+	}
 	scmi_xfer_command_release(info, xfer);
 }
 
@@ -1279,6 +1302,12 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
 		}
 	}
 
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS) && ret == -ETIMEDOUT) {
+		struct scmi_info *info =
+					handle_to_scmi_info(cinfo->handle);
+		atomic_inc(&info->stats.xfers_response_timeout);
+	}
+
 	return ret;
 }
 
@@ -1414,6 +1443,13 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id,
 			    xfer->hdr.protocol_id, xfer->hdr.seq, ret);
 
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
+		if (ret == 0)
+			atomic_inc(&info->stats.sent_ok);
+		else
+			atomic_inc(&info->stats.sent_fail);
+	}
+
 	return ret;
 }
 
@@ -2994,6 +3030,14 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->devm_protocol_get = scmi_devm_protocol_get;
 	handle->devm_protocol_put = scmi_devm_protocol_put;
 
+	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
+		atomic_set(&info->stats.response_ok, 0);
+		atomic_set(&info->stats.sent_fail, 0);
+		atomic_set(&info->stats.sent_ok, 0);
+		atomic_set(&info->stats.dlyd_response_ok, 0);
+		atomic_set(&info->stats.xfers_response_timeout, 0);
+	}
+
 	/* System wide atomic threshold for atomic ops .. if any */
 	if (!of_property_read_u32(np, "atomic-threshold-us",
 				  &info->atomic_threshold))
-- 
2.34.1

Re: [PATCH 2/3] Track basic SCMI statistics

[Cristian Marussi]

On Mon, Jul 01, 2024 at 03:28:50PM +0100, Luke Parkin wrote:
> Add scmi_debug_stats struct with atomic_t types to prevent racing.
> 

Hi

"Add SCMI debug statisticts based on atomic types to prevent races."

if you want to specify why you did it this way in the commit message,
but this seems really more something for a comment in the doxygen that
on the commit log.

> Add tracking of 5 initial statistics
> - sent_ok & sent_fail
> - response_ok & dlyd_response_ok
> - xfers_response_timeout

Morover I would not specify here in the log what you added, you can see
it from the code. "Add some initial stats counter" if you want.

> 
> Signed-off-by: Luke Parkin <luke.parkin@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 46 +++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 6b6957f4743f..f69dff699d48 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
>   *
> @@ -141,6 +157,7 @@ struct scmi_debug_info {
>   * @protocols: IDR for protocols' instance descriptors initialized for
>   *	       this SCMI instance: populated on protocol's first attempted
>   *	       usage.
> + * @stats: Contains several atomic_t's for tracking various statistics
>   * @protocols_mtx: A mutex to protect protocols instances initialization.
>   * @protocols_imp: List of protocols implemented, currently maximum of
>   *		   scmi_revision_info.num_protocols elements allocated by the
> @@ -174,6 +191,7 @@ struct scmi_info {
>  	struct idr tx_idr;
>  	struct idr rx_idr;
>  	struct idr protocols;
> +	struct scmi_debug_stats stats;

Pleaase move this field right after scmi_debug_info down below, so that
we keep all debug stuff together.

Also this is an embedded structure, not a bare pointer to dynamically
allocated stuff (and this is fine...) so you should probably ifdef
ARM_SCMI_DEBUG_STATISTICS at compile time the presence of this field itself
so as not to waste memory for something you never use...but this is
opinable because it will also, on the other side, polllute a bit the code
with unpleasant ifdeffery... so maybe Sudeep wont like it...the other option
would be to make this a pointer and conditionaly devm_kzalloc a struct to this
pointer (like scmi_debug_info)

>  	/* Ensure mutual exclusive access to protocols instance array */
>  	struct mutex protocols_mtx;
>  	u8 *protocols_imp;
> @@ -1143,7 +1161,12 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
>  						SCMI_RAW_REPLY_QUEUE,
>  						cinfo->id);
>  	}
> -
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
> +		if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP)
> +			atomic_inc(&info->stats.dlyd_response_ok);
> +		else
> +			atomic_inc(&info->stats.response_ok);
> +	}

Ok, so IMO, this is the main core thing to rework in this series: the
"counting" operation/block should be defined as a macro so that it can
be fully compiled out when STATS=n, because these are counters
incremented on the hot path for each message, not just once in a while,
so the above if(IS_ENABELD()) now will be there and evaluated even when
STATS=n.

Something like:

	#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
	#define SCMI_LOG_STATS(counter)			\
		<your magic here> 			\
	#else
	#define SCMI_LOG_STATS(counter)
	#endif

.... I have not thought it through eh...so it could be radically
different...the point is ... the counting machinery should just
disappear at compile time when STATS=n

Moreover please define this macros magic here in this patch BUT split
out in a distinct patch all the places where you make use of it, so that
this patch contains only definitions of mechanisms and struct and
another patch will contain all the places where stats are monitored.

>  	scmi_xfer_command_release(info, xfer);
>  }
>  
> @@ -1279,6 +1302,12 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc,
>  		}
>  	}
>  
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS) && ret == -ETIMEDOUT) {
> +		struct scmi_info *info =
> +					handle_to_scmi_info(cinfo->handle);
> +		atomic_inc(&info->stats.xfers_response_timeout);
> +	}
> +

Ditto.

>  	return ret;
>  }
>  
> @@ -1414,6 +1443,13 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>  	trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id,
>  			    xfer->hdr.protocol_id, xfer->hdr.seq, ret);
>  
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
> +		if (ret == 0)
> +			atomic_inc(&info->stats.sent_ok);
> +		else
> +			atomic_inc(&info->stats.sent_fail);
> +	}
> +

Ditto.

>  	return ret;
>  }
>  
> @@ -2994,6 +3030,14 @@ static int scmi_probe(struct platform_device *pdev)
>  	handle->devm_protocol_get = scmi_devm_protocol_get;
>  	handle->devm_protocol_put = scmi_devm_protocol_put;
>  
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) {
> +		atomic_set(&info->stats.response_ok, 0);
> +		atomic_set(&info->stats.sent_fail, 0);
> +		atomic_set(&info->stats.sent_ok, 0);
> +		atomic_set(&info->stats.dlyd_response_ok, 0);
> +		atomic_set(&info->stats.xfers_response_timeout, 0);
> +	}
> +

I think that you can just drop this zero initializations because the
stats are part of the info structure which is created devm_kzalloc'ed...
so zerod at creation time AND indeed atomic_t is just a structure containing
an int counter which will be already zeroed too...I dont think that any
other special init is done, it is already zero, and nothing more is done
by your atomic_set(v, 0)...but feel free to verify I am not missing
something, please.

Thanks,
Cristian

Re: [PATCH 2/3] Track basic SCMI statistics

[Luke Parkin]

> Ok, so IMO, this is the main core thing to rework in this series: the
> "counting" operation/block should be defined as a macro so that it can
> be fully compiled out when STATS=n, because these are counters
> incremented on the hot path for each message, not just once in a while,
> so the above if(IS_ENABELD()) now will be there and evaluated even when
> STATS=n.
>
> Something like:
>
> 	#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
> 	#define SCMI_LOG_STATS(counter)			\
> 		<your magic here> 			\
> 	#else
> 	#define SCMI_LOG_STATS(counter)
> 	#endif
>
> .... I have not thought it through eh...so it could be radically
> different...the point is ... the counting machinery should just
> disappear at compile time when STATS=n

Hey Cristian, Unless I've missed something, It looks like IS_ENABLED() does do
what you ask for.
In Documentation/process/coding-style.rst:1168 it reccomends using IS_ENABLED
for conditional compilation over #if and #ifdef, saying that the compiler will
constant-fold the conditional away.

Thanks,
Luke

Re: [PATCH 2/3] Track basic SCMI statistics

[Cristian Marussi]

On Tue, Jul 02, 2024 at 10:57:23AM +0100, Luke Parkin wrote:
> > Ok, so IMO, this is the main core thing to rework in this series: the
> > "counting" operation/block should be defined as a macro so that it can
> > be fully compiled out when STATS=n, because these are counters
> > incremented on the hot path for each message, not just once in a while,
> > so the above if(IS_ENABELD()) now will be there and evaluated even when
> > STATS=n.
> >
> > Something like:
> >
> > 	#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
> > 	#define SCMI_LOG_STATS(counter)			\
> > 		<your magic here> 			\
> > 	#else
> > 	#define SCMI_LOG_STATS(counter)
> > 	#endif
> >
> > .... I have not thought it through eh...so it could be radically
> > different...the point is ... the counting machinery should just
> > disappear at compile time when STATS=n
> 
> Hey Cristian, Unless I've missed something, It looks like IS_ENABLED() does do
> what you ask for.
> In Documentation/process/coding-style.rst:1168 it reccomends using IS_ENABLED
> for conditional compilation over #if and #ifdef, saying that the compiler will
> constant-fold the conditional away.

Yes indeed, it will be compiled out anyway, forgot that despite having
it used myself a few lines below :D .... but from the readability standpoint,
given that we will sprinkle this all over the code, wont be much clearer to
conditionally define once for all an inline function (like mentioned at the
start of that coding-style.rst paragraph) or a macro in a header (like common.h)
to wrap the atomic_inc

#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
static inline void scmi_log_stats(atomic_t *cnt)
{
	atomic_inc(cnt);
}
#else
static inline void scmi_log_stats(atomic_t *cnt) { }
#endif

and then just call it plainly wherever it needs, knowing that the compiler
will equally compile it out all-over when STATS=n.

ifdeffery is discouraged in the code flow but it is acceptable to define
alternative nop fucntions in a header.

Also because in some of the callsite you handle 2 stats with some ifcond
(conditional on the IS_ENABLED that is good) and that could be a problem,
but those calls can be split and placed alone where that some condition is
already check normally as in as an example in scmi_handle_response():

	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);
        }                                    

...what do you think, am I missing something else ?

Thanks,
Cristian

Re: [PATCH 2/3] Track basic SCMI statistics

[Luke Parkin]

> #ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
> static inline void scmi_log_stats(atomic_t *cnt)
> {
> 	atomic_inc(cnt);
> }
> #else
> static inline void scmi_log_stats(atomic_t *cnt) { }
> #endif
> but those calls can be split and placed alone where that some condition is
> already check normally as in as an example in scmi_handle_response():
>
> 	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);
>         }
>
> ...what do you think, am I missing something else ?

Ah yeah, that looks better to me. I'll use that.

Thanks!
Luke


On 02/07/2024 12:10, Cristian Marussi wrote:
> On Tue, Jul 02, 2024 at 10:57:23AM +0100, Luke Parkin wrote:
>>> Ok, so IMO, this is the main core thing to rework in this series: the
>>> "counting" operation/block should be defined as a macro so that it can
>>> be fully compiled out when STATS=n, because these are counters
>>> incremented on the hot path for each message, not just once in a while,
>>> so the above if(IS_ENABELD()) now will be there and evaluated even when
>>> STATS=n.
>>>
>>> Something like:
>>>
>>> 	#ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
>>> 	#define SCMI_LOG_STATS(counter)			\
>>> 		<your magic here> 			\
>>> 	#else
>>> 	#define SCMI_LOG_STATS(counter)
>>> 	#endif
>>>
>>> .... I have not thought it through eh...so it could be radically
>>> different...the point is ... the counting machinery should just
>>> disappear at compile time when STATS=n
>>
>> Hey Cristian, Unless I've missed something, It looks like IS_ENABLED() does do
>> what you ask for.
>> In Documentation/process/coding-style.rst:1168 it reccomends using IS_ENABLED
>> for conditional compilation over #if and #ifdef, saying that the compiler will
>> constant-fold the conditional away.
> 
> Yes indeed, it will be compiled out anyway, forgot that despite having
> it used myself a few lines below :D .... but from the readability standpoint,
> given that we will sprinkle this all over the code, wont be much clearer to
> conditionally define once for all an inline function (like mentioned at the
> start of that coding-style.rst paragraph) or a macro in a header (like common.h)
> to wrap the atomic_inc
> 
> #ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS
> static inline void scmi_log_stats(atomic_t *cnt)
> {
> 	atomic_inc(cnt);
> }
> #else
> static inline void scmi_log_stats(atomic_t *cnt) { }
> #endif
> 
> and then just call it plainly wherever it needs, knowing that the compiler
> will equally compile it out all-over when STATS=n.
> 
> ifdeffery is discouraged in the code flow but it is acceptable to define
> alternative nop fucntions in a header.
> 
> Also because in some of the callsite you handle 2 stats with some ifcond
> (conditional on the IS_ENABLED that is good) and that could be a problem,
> but those calls can be split and placed alone where that some condition is
> already check normally as in as an example in scmi_handle_response():
> 
> 	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);
>         }                                    
> 
> ...what do you think, am I missing something else ?
> 
> Thanks,
> Cristian

[PATCH 3/3] 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>
---
 drivers/firmware/arm_scmi/driver.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f69dff699d48..509ea42d17bf 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2884,7 +2884,7 @@ static void scmi_debugfs_common_cleanup(void *d)
 static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
 {
 	char top_dir[16];
-	struct dentry *trans, *top_dentry;
+	struct dentry *trans, *top_dentry, *stats;
 	struct scmi_debug_info *dbg;
 	const char *c_ptr = NULL;
 
@@ -2935,6 +2935,19 @@ 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)) {
+		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);
+	}
 	dbg->top_dentry = top_dentry;
 
 	if (devm_add_action_or_reset(info->dev,
-- 
2.34.1

Re: [PATCH 3/3] Create debugfs files for statistics

[Cristian Marussi]

On Mon, Jul 01, 2024 at 03:28:51PM +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>
> ---
>  drivers/firmware/arm_scmi/driver.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index f69dff699d48..509ea42d17bf 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2884,7 +2884,7 @@ static void scmi_debugfs_common_cleanup(void *d)
>  static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
>  {
>  	char top_dir[16];
> -	struct dentry *trans, *top_dentry;
> +	struct dentry *trans, *top_dentry, *stats;

stats are conditional...so...

>  	struct scmi_debug_info *dbg;
>  	const char *c_ptr = NULL;
>  
> @@ -2935,6 +2935,19 @@ 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)) {
> +		stats = debugfs_create_dir("stats", trans);

you can put the above local *stats var instead so that it is NOT even
defined when STATS=n

> +		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);
> +	}

... moreover...this is not always enabled BUT certainly will be extend
sooner with more counters...so please split this setup out in a local helper
function.

Thanks,
Cristian

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

[Cristian Marussi]

On Mon, Jul 01, 2024 at 03:28:48PM +0100, Luke Parkin wrote:
> Hi,
> 

Hi Luke,

thanks for taking a shot at this, a few comments down below and along
this series.

> This series adds support for tracking Arm SCMI statistics, [Patch 2]
> A config option to enable this, [Patch 1] 
> And in [Patch 3] a selection of new debugfs entries to present these statistics
> 
> These statistics are per transport instance, and will be a helpful resource when
> debugging the SCMI and running tests.
> 
> Based on v6.9, Tested on Arm Juno [1]
> 

First of all a nitpick on commit messages themselves...when posting patches on a specific
subsystem, the commit message "style" should align with the subsystem conventions:
IOW all the patches in your series (beside this cover-letter) should be titled as:

  firmware: arm_scmi: My patch starting with a capital letter

as you can guess having a look at git log --oneline drivers/firmware/arm_scmi

So please do this in V2 together with other reviews.

Thanks,
Cristian

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

[Cristian Marussi]

On Mon, Jul 01, 2024 at 03:28:48PM +0100, Luke Parkin wrote:
> Hi,
> 
> This series adds support for tracking Arm SCMI statistics, [Patch 2]
> A config option to enable this, [Patch 1] 
> And in [Patch 3] a selection of new debugfs entries to present these statistics
> 
> These statistics are per transport instance, and will be a helpful resource when
> debugging the SCMI and running tests.
> 
> 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
> 
> Luke Parkin (3):
>   Add Kconfig option for scmi debug statistics
>   Track basic SCMI statistics
>   Create debugfs files for statistics
> 
>  drivers/firmware/arm_scmi/Kconfig  | 10 +++++
>  drivers/firmware/arm_scmi/driver.c | 61 +++++++++++++++++++++++++++++-
>  2 files changed, 69 insertions(+), 2 deletions(-)
> 

All in all, seems to me a good start, please address or dispute my (and
possibly other reviewers) observations in V2.

Thanks,
Cristian