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 Ardelean <[email protected]>
Signed-off-by: Alexandru Tachici <[email protected]>
---
drivers/iio/industrialio-core.c | 15 +++++++++++----
include/linux/iio/iio.h | 2 ++
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 65ff0d067018..c4d5104934fc 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -301,11 +301,14 @@ 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;
- char buf[20];
unsigned val = 0;
- ssize_t len;
int ret;
+ if (*ppos > 0)
+ return simple_read_from_buffer(userbuf, count, ppos,
+ indio_dev->read_buf,
+ indio_dev->read_buf_len);
+
ret = indio_dev->info->debugfs_reg_access(indio_dev,
indio_dev->cached_reg_addr,
0, &val);
@@ -314,9 +317,13 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
return ret;
}
- len = snprintf(buf, sizeof(buf), "0x%X\n", val);
+ indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
+ sizeof(indio_dev->read_buf),
+ "0x%X\n", val);
- 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 Fri, 21 Feb 2020 14:06:55 +0200
Alexandru Tachici <[email protected]> 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).
>
> Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, add support for direct register access")
> Signed-off-by: Alexandru Ardelean <[email protected]>
> Signed-off-by: Alexandru Tachici <[email protected]>
I'm going to take this one via the slow path to give it a bit of time
to soak in linux-next. I think we are fine for unexpected side effects
but it's been there for a long time so we can take this slowly.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/iio/industrialio-core.c | 15 +++++++++++----
> include/linux/iio/iio.h | 2 ++
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 65ff0d067018..c4d5104934fc 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -301,11 +301,14 @@ 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;
> - char buf[20];
> unsigned val = 0;
> - ssize_t len;
> int ret;
>
> + if (*ppos > 0)
> + return simple_read_from_buffer(userbuf, count, ppos,
> + indio_dev->read_buf,
> + indio_dev->read_buf_len);
> +
> ret = indio_dev->info->debugfs_reg_access(indio_dev,
> indio_dev->cached_reg_addr,
> 0, &val);
> @@ -314,9 +317,13 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
> return ret;
> }
>
> - len = snprintf(buf, sizeof(buf), "0x%X\n", val);
> + indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
> + sizeof(indio_dev->read_buf),
> + "0x%X\n", val);
>
> - 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
> };
>
On Sun, 2020-03-01 at 19:56 +0000, Jonathan Cameron wrote:
> [External]
>
> On Fri, 21 Feb 2020 14:06:55 +0200
> Alexandru Tachici <[email protected]> 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).
> >
> > Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, add
> > support for direct register access")
> > Signed-off-by: Alexandru Ardelean <[email protected]>
> > Signed-off-by: Alexandru Tachici <[email protected]>
>
> I'm going to take this one via the slow path to give it a bit of time
> to soak in linux-next. I think we are fine for unexpected side effects
> but it's been there for a long time so we can take this slowly.
No hurry from us.
This one has been showing-up randomly when trying to use the debugfs interface
for (well...) debugging.
Most of the times the double-read [of regs] is fine; so this issue can go un-
noticed.
But in cases like reading a FIFO for debug purposes [I know this sounds a bit
like an abuse, but it is being read from a reg], the double-read drops some
values.
And that makes some support more difficult.
>
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/industrialio-core.c | 15 +++++++++++----
> > include/linux/iio/iio.h | 2 ++
> > 2 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index 65ff0d067018..c4d5104934fc 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -301,11 +301,14 @@ 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;
> > - char buf[20];
> > unsigned val = 0;
> > - ssize_t len;
> > int ret;
> >
> > + if (*ppos > 0)
> > + return simple_read_from_buffer(userbuf, count, ppos,
> > + indio_dev->read_buf,
> > + indio_dev->read_buf_len);
> > +
> > ret = indio_dev->info->debugfs_reg_access(indio_dev,
> > indio_dev->cached_reg_addr,
> > 0, &val);
> > @@ -314,9 +317,13 @@ static ssize_t iio_debugfs_read_reg(struct file *file,
> > char __user *userbuf,
> > return ret;
> > }
> >
> > - len = snprintf(buf, sizeof(buf), "0x%X\n", val);
> > + indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
> > + sizeof(indio_dev->read_buf),
> > + "0x%X\n", val);
> >
> > - 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
> > };
> >