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