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 <[email protected]>
---
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);
+ 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
};
--
2.20.1
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 <[email protected]>
> ---
> 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
> };
>
On Thu, 2020-02-20 at 19:07 +0200, Alexandru Tachici wrote:
> 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).
Please disregard this. Will send a V2.
> Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support,
> add support for direct register access")
> Signed-off-by: Alexandru Tachici <[email protected]>
> ---
> 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);
> + 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
> };
>