Re: [PATCH] iio: industrialio-core: Fix debugfs read

[Ardelean, Alexandru]

On Thu, 2020-02-20 at 19:07 +0200, Alexandru Tachici wrote:
> [External]
> 
> Currently iio_debugfs_read_reg calls debugfs_reg_access
> every time it is ran. Reading the same hardware register
> multiple times during the same reading of a debugfs file
> can cause unintended effects.
> 
> For example for each: cat iio:device0/direct_reg_access
> the file_operations.read function will be called at least
> twice. First will return the full length of the string in
> bytes  and the second will return 0.
> 
> This patch makes iio_debugfs_read_reg to call debugfs_reg_access
> only when the user's buffer position (*ppos) is 0. (meaning
> it is the beginning of a new reading of the debugfs file).
> 
> Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, add
> support for direct register access")
> Signed-off-by: Alexandru Tachici <alexandru.tachici@xxxxxxxxxx>
> ---
>  drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++----------
>  include/linux/iio/iio.h         |  2 ++
>  2 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 65ff0d067018..637cea14afdb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -297,26 +297,40 @@ static void __exit iio_exit(void)
>  }
>  
>  #if defined(CONFIG_DEBUG_FS)
> -static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
> -			      size_t count, loff_t *ppos)
> +static int iio_debugfs_get_reg_string(struct iio_dev *indio_dev)
>  {
> -	struct iio_dev *indio_dev = file->private_data;
> -	char buf[20];
> +	const struct iio_info *info = indio_dev->info;
>  	unsigned val = 0;
> -	ssize_t len;
>  	int ret;
>  
> -	ret = indio_dev->info->debugfs_reg_access(indio_dev,
> -						  indio_dev->cached_reg_addr,
> -						  0, &val);
> +	ret = info->debugfs_reg_access(indio_dev, indio_dev->cached_reg_addr,
> +				       0, &val);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__);
>  		return ret;
>  	}
> +	indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
> +					   sizeof(indio_dev->read_buf),
> +					   "0x%X\n", val);
> +	return 0;
> +}
>  
> -	len = snprintf(buf, sizeof(buf), "0x%X\n", val);
> +static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
> +				    size_t count, loff_t *ppos)
> +{
> +	struct iio_dev *indio_dev = file->private_data;
> +	loff_t pos = *ppos;
> +	int ret;
> +
> +	if (pos == 0) {
> +		ret = iio_debugfs_get_reg_string(indio_dev);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	return simple_read_from_buffer(userbuf, count, ppos, buf, len);

I'm wondering now, if it would be sufficient here to just do:

if (len == simple_read_from_buffer(userbuf, count, ppos, buf, len))
    return 0;


> +	return simple_read_from_buffer(userbuf, count, ppos,
> +				       indio_dev->read_buf,
> +				       indio_dev->read_buf_len);
>  }
>  
>  static ssize_t iio_debugfs_write_reg(struct file *file,
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 862ce0019eba..eed58ed2f368 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -568,6 +568,8 @@ struct iio_dev {
>  #if defined(CONFIG_DEBUG_FS)
>  	struct dentry			*debugfs_dentry;
>  	unsigned			cached_reg_addr;
> +	char				read_buf[20];
> +	unsigned int			read_buf_len;
>  #endif
>  };
>