| Message ID | 20200221120655.20252-1-alexandru.tachici@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: industrialio-core: Fix debugfs read | expand |
On Fri, 21 Feb 2020 14:06:55 +0200 Alexandru Tachici <alexandru.tachici@analog.com> 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 <alexandru.ardelean@analog.com> > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> 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 <alexandru.tachici@analog.com> 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 <alexandru.ardelean@analog.com> > > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> > > 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 > > }; > >
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 };
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 <alexandru.ardelean@analog.com> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> --- drivers/iio/industrialio-core.c | 15 +++++++++++---- include/linux/iio/iio.h | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-)