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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZqEEYfLGshkKyM-x@pluto>
Date: Wed, 24 Jul 2024 14:40:49 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Luke Parkin <luke.parkin@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	arm-scmi@...r.kernel.org, sudeep.holla@....com,
	cristian.marussi@....com
Subject: Re: [PATCH v3 2/5] firmware: arm_scmi: Add support for tracking
 statistics

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@....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